PERFORCE change 178863 for review

[Garrett Cooper]

http://p4web.freebsd.org/@@178863?ac=10

Change 178863 by gcooper at gcooper-bayonetta on 2010/05/27 10:05:35

	
	1. Fix the linereading by using the proper variable. Properly cap the
	value for now by erroring out quickly noting the cause of failure, as
	opposed to silently failing.
	2. Remove some debug strings.
	3. Add some comments.

Affected files ...

.. //depot/projects/soc2007/gcooper-pkg_install-enhancements-simplified/lib/libpkg/plist.c#6 edit

Differences ...

==== //depot/projects/soc2007/gcooper-pkg_install-enhancements-simplified/lib/libpkg/plist.c#6 (text+ko) ====

@@ -196,11 +196,21 @@
 int
 plist_cmd(const char *s, char **arg)
 {
+    /* XXX (gcooper): this can blow up really quickly with the recent
+     * modifications made to read_plist, provided a sufficiently large list. */
     char cmd[FILENAME_MAX + 20];	/* 20 == fudge for max cmd len */
     char *cp;
     const char *sp;
 
-    strlcpy(cmd, s, sizeof(cmd));
+    /* 
+     * FIXME (gcooper): this should be dynamic according to whatever's passed
+     * in.
+     */
+    if (strlcpy(cmd, s, sizeof(cmd)) >= sizeof(cmd)) {
+	warnx("%s: line '%s' exceeds set limits", __func__, s);
+	errno = EINVAL;
+	return -1;
+    }
     str_lowercase(cmd);
     cp = cmd;
     sp = s;
@@ -279,7 +289,7 @@
 	int major;
 	int minor;
 	int rc = -1;
-	off_t off;
+	off_t end_off;
 	size_t len;
 
 	pkg->fmtver_maj = 1;
@@ -292,15 +302,11 @@
 	    (contents_map = mmap(NULL, sb.st_size, PROT_READ, MAP_SHARED, fd,
 	    0)) != NULL) {
 
-		off = sb.st_size;
+		end_off = sb.st_size;
 		rc = 0;
 		start = contents_map;
 
-		/* 
-		 * XXX (gcooper): BAD BAD BAD -- this can be longer than
-		 * FILENAME_MAX
-		 */
-		while (rc == 0 && 0 < off) {
+		while (rc == 0 && 0 < end_off) {
 
 			end = strchr(start, '\n');
 			/* No trailing newlines -- look for '\0'. */
@@ -308,7 +314,7 @@
 				end = strchr(start, '\0');
 			/* Don't forget we're eating newlines.. om nom nom... */
 			else
-				off--;
+				end_off--;
 			/* 
 			 * This is bad if this fails -- a non-NUL terminated
 			 * string is in our midst!
@@ -323,12 +329,10 @@
 
 				strlcpy(cmd_buf, start, end-start+1);
 
-				warnx("cmd_buf: %s", cmd_buf);
-
 				len = strlen(cmd_buf);
-				off -= len;
-
-				/* Trim off trailing whitespace. */
+				end_off -= len;
+				
+				/* Trim end_off trailing whitespace. */
 				while (0 < len && isspace(cmd_buf[len]))
 					cmd_buf[len--] = '\0';
 
@@ -344,7 +348,7 @@
 			/* A plist command directive */
 			if (rc == 0 && *start == CMD_CHAR) {
 
-				cmd = plist_cmd(start+1, &cp);
+				cmd = plist_cmd(cmd_buf+1, &cp);
 
 				if (cmd == -1) {
 					warnx("%s: unknown command '%s' "
@@ -394,6 +398,11 @@
 			/* A file manifest item */
 			else if (rc == 0)
 				cmd = PLIST_FILE;
+
+			/* 
+			 * Winner, winner, chicken dinner.. we have a working
+			 * command!
+			 */
 			if (rc == 0) {
 
 				add_plist(pkg, cmd, cp);
@@ -404,6 +413,12 @@
 
 			}
 
+			/* 
+			 * XXX (gcooper): using more intelligent pointer
+			 * arithmetic and proper NUL termination, there's no
+			 * reason why this needs to be freed automatically in
+			 * each iteration.
+			 */
 			if (cmd_buf != NULL) {
 				free (cmd_buf);
 				cmd_buf = NULL;
@@ -573,7 +588,6 @@
 	case PLIST_CWD:
 	    if (prefix == NULL)
 		prefix = p->name;
-	    warnx("prefix: %s", prefix);
 	    Where = (p->name == NULL) ? prefix : p->name;
 	    if (Verbose)
 		printf("Change working directory to %s\n", Where);