kern/89181: [patch] lib/libutil properties_read() bug with files > 4096 bytes

[Antony Mawer]

>Number:         89181
>Category:       kern
>Synopsis:       [patch] lib/libutil properties_read() bug with files > 4096 bytes
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Thu Nov 17 00:50:14 GMT 2005
>Closed-Date:
>Last-Modified:
>Originator:     Antony Mawer
>Release:        FreeBSD 6.0-BETA2 i386
>Organization:
GP Technology Solutions
>Environment:
System: FreeBSD scooby.enchanted.net 6.0-BETA2 FreeBSD 6.0-BETA2 #1: Mon Aug 8 09:56:30 EST 2005 root at scooby.enchanted.net:/usr/obj/usr/src/sys/SCOOBY i386

>Description:
	While developing a custom sysinstall-based 6.0 installation CD, I hit upon
	a "Invalid realloc size of 0!" error during testing. This problem occurs
	every time at exactly the same place during the installation, and happens
	when installing a (relatively) large local distribution (~200mb).

	Looking at the code, I suspected it was a problem with the property
	functions in libutil, and wrote a test program that parsed the local.inf
	file. The source for this test program can be found at the following URL,
	allong with the local.inf that demonstrates the problem:

		http://people.gptech.com.au/~ajmawer/freebsd/proptest.c
		http://people.gptech.com.au/~ajmawer/freebsd/local.inf

	The test program indicated that in the middle of the property values read
	from the file, one was returning NULL, while subsequent properties were
	able to be read without a problem:

		Processing chunk 137 of 163
		Looking for cksum.fh
		Read property '(null)'

	Tracing through properties_read() in src/lib/libutil/property.c showed
	that the first 4096 bytes were being into a buffer and processed, then
	the function would read another 4096 and begin in a LOOK state again,
	regardless of where it was up to prior to the buffer running out. As a
	result, the property that was half-way through being read was discarded
	and properties_read() started work on the next one.
>How-To-Repeat:
	1. Download the proptest.c and local.inf files and place them in the
	   same directory.
	2. Compile the proptest.c file:
		gcc -o proptest -lutil proptest.c
	3. Run the command:
		./proptest | less
	4. Observe that Read property for 'cksum.fh' returns NULL
>Fix:
	The problem is that when properties_read() reaches the end of its, 
	it sets the state = FILL, discarding whatever the previous state
	was, re-fills the buffer, and then launches straight into a LOOK state
	by allowing the switch statement to fall through to the next case
	statement. So if, as in this instance, the buffer runs out while it
	is in the middle of a state = VALUE operation, this state is lost and
	a fresh search for a name=value pair begins.

	The attached patch teaches properties_read() to remember its current
	state prior to setting state = FILL, and restore it upon refilling
	the buffer. In order to do this, it was necessary to remove the
	fallthrough to the LOOK case, and instead re-start the while loop
	and re-evaluate the state.

	After rebuilding with the attached patch, re-running the proptest.c
	shows that the cksum.fh is now read correctly.

--- property.c.diff begins here ---
index: lib/libutil/property.c
===================================================================
RCS file: /home/ncvs/src/lib/libutil/property.c,v
retrieving revision 1.13
diff -u -r1.13 property.c
--- lib/libutil/property.c	14 Jun 2003 18:42:37 -0000	1.13
+++ lib/libutil/property.c	17 Nov 2005 00:03:41 -0000
@@ -75,17 +75,18 @@
     char hold_v[PROPERTY_MAX_VALUE + 1];
     char buf[BUFSIZ * 4];
     int bp, n, v, max;
-    enum { LOOK, COMMENT, NAME, VALUE, MVALUE, COMMIT, FILL, STOP } state;
+    enum { LOOK, COMMENT, NAME, VALUE, MVALUE, COMMIT, FILL, STOP } state, last_state;
     int ch = 0, blevel = 0;
 
     n = v = bp = max = 0;
     head = ptr = NULL;
-    state = LOOK;
+    state = last_state = LOOK;
     while (state != STOP) {
 	if (state != COMMIT) {
-	    if (bp == max)
+	    if (bp == max) {
+		last_state = state;
 		state = FILL;
-	    else
+	    } else
 		ch = buf[bp++];
 	}
 	switch(state) {
@@ -96,13 +97,17 @@
 	    }
 	    if (max == 0) {
 		state = STOP;
-		break;
 	    } else {
-		state = LOOK;
+		/* Restore the state as to before the fill (which will be
+		 * initialised to LOOK for the first FILL). This ensures that
+		 * if we were part-way through, eg. a VALUE state, when the
+		 * buffer ran out, that the previous operation will be allowed
+		 * to complete. */
+		state = last_state;
 		ch = buf[0];
-		bp = 1;
+		bp = 0;
 	    }
-	    /* FALLTHROUGH deliberately since we already have a character and state == LOOK */
+	    continue;
 
 	case LOOK:
 	    if (isspace((unsigned char)ch))
--- property.c.diff ends here ---

>Release-Note:
>Audit-Trail:
>Unformatted: