Date: Thu, 17 Nov 2005 11:44:34 +1100 (EST) From: Antony Mawer <gnats@mawer.org> To: FreeBSD-gnats-submit@FreeBSD.org Subject: kern/89181: [patch] lib/libutil properties_read() bug with files > 4096 bytes Message-ID: <20051117004434.EE3597A@scooby.enchanted.net> Resent-Message-ID: <200511170050.jAH0oEs1078495@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>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@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:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20051117004434.EE3597A>