From owner-freebsd-bugs@FreeBSD.ORG Thu Aug 11 12:40:31 2005 Return-Path: X-Original-To: freebsd-bugs@hub.freebsd.org Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 9D4CB16A41F for ; Thu, 11 Aug 2005 12:40:31 +0000 (GMT) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [216.136.204.21]) by mx1.FreeBSD.org (Postfix) with ESMTP id 93C3943D55 for ; Thu, 11 Aug 2005 12:40:22 +0000 (GMT) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.13.3/8.13.3) with ESMTP id j7BCeLHb013232 for ; Thu, 11 Aug 2005 12:40:21 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.13.3/8.13.1/Submit) id j7BCeLkb013231; Thu, 11 Aug 2005 12:40:21 GMT (envelope-from gnats) Resent-Date: Thu, 11 Aug 2005 12:40:21 GMT Resent-Message-Id: <200508111240.j7BCeLkb013231@freefall.freebsd.org> Resent-From: FreeBSD-gnats-submit@FreeBSD.org (GNATS Filer) Resent-To: freebsd-bugs@FreeBSD.org Resent-Reply-To: FreeBSD-gnats-submit@FreeBSD.org, Spencer Minear Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 6B22416A41F for ; Thu, 11 Aug 2005 12:37:32 +0000 (GMT) (envelope-from nobody@FreeBSD.org) Received: from www.freebsd.org (www.freebsd.org [216.136.204.117]) by mx1.FreeBSD.org (Postfix) with ESMTP id 236C043D55 for ; Thu, 11 Aug 2005 12:37:32 +0000 (GMT) (envelope-from nobody@FreeBSD.org) Received: from www.freebsd.org (localhost [127.0.0.1]) by www.freebsd.org (8.13.1/8.13.1) with ESMTP id j7BCbVx9028780 for ; Thu, 11 Aug 2005 12:37:31 GMT (envelope-from nobody@www.freebsd.org) Received: (from nobody@localhost) by www.freebsd.org (8.13.1/8.13.1/Submit) id j7BCbVTW028779; Thu, 11 Aug 2005 12:37:31 GMT (envelope-from nobody) Message-Id: <200508111237.j7BCbVTW028779@www.freebsd.org> Date: Thu, 11 Aug 2005 12:37:31 GMT From: Spencer Minear To: freebsd-gnats-submit@FreeBSD.org X-Send-Pr-Version: www-2.3 Cc: Subject: misc/84797: State engine in the libutils properties_read is not safe over incremental reads X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Aug 2005 12:40:31 -0000 >Number: 84797 >Category: misc >Synopsis: State engine in the libutils properties_read is not safe over incremental reads >Confidential: no >Severity: non-critical >Priority: low >Responsible: freebsd-bugs >State: open >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Thu Aug 11 12:40:20 GMT 2005 >Closed-Date: >Last-Modified: >Originator: Spencer Minear >Release: 5.4 >Organization: Securecomputing Corp. >Environment: FreeBSD nowind2.scur.com 5.4-RELEASE FreeBSD 5.4-RELEASE #0: Fri Jun 24 07:43:40 CDT 2005 mkarels@nowind2.scur.com:/usr/src/sys/i386/compile/SMP i386 >Description: If the data fead to the properties_read function in libustil is coming over a network, and results in a partial read, the parsing state engine will loose information from the last partial line when a subsequent read is done. >How-To-Repeat: Build build a program that uses the property funtions to parse a property file like a FTP dist.inf file and feed the information in "chunks" with a shell script like the following, where base.inf.1 and base.inf.2 are a base.inf file that gets cut in the middle of a line. #!/bin/sh ( cat base.inf.1; sleep 2; cat base.inf.2 ) | ./proptest >Fix: The following is the context diff of the changes I made that appear to work. The approach is to separate the buffer read state from the parsing state, and not change the parse state just becuase the next block of data is read. Index: property.c =================================================================== RCS file: /projects/fusion/cvs/src/freebsd/lib/libutil/property.c,v retrieving revision 1.2 diff -c -r1.2 property.c *** property.c 2005/01/04 18:14:22 1.2 --- property.c 2005/08/11 11:40:56 *************** *** 75,218 **** 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; int ch = 0, blevel = 0; n = v = bp = max = 0; head = ptr = NULL; ! state = LOOK; ! while (state != STOP) { ! if (state != COMMIT) { if (bp == max) ! state = FILL; else ch = buf[bp++]; } ! switch(state) { case FILL: if ((max = read(fd, buf, sizeof buf)) < 0) { properties_free(head); return (NULL); } if (max == 0) { ! state = STOP; break; } else { ! state = LOOK; ch = buf[0]; bp = 1; } ! /* FALLTHROUGH deliberately since we already have a character and st ate == LOOK */ ! case LOOK: ! if (isspace((unsigned char)ch)) ! continue; ! /* Allow shell or lisp style comments */ ! else if (ch == '#' || ch == ';') { ! state = COMMENT; ! continue; ! } ! else if (isalnum((unsigned char)ch) || ch == '_') { ! if (n >= PROPERTY_MAX_NAME) { ! n = 0; ! state = COMMENT; } ! else { ! hold_n[n++] = ch; ! state = NAME; } ! } ! else ! state = COMMENT; /* Ignore the rest of the line */ ! break; ! case COMMENT: ! if (ch == '\n') ! state = LOOK; ! break; ! ! case NAME: ! if (ch == '\n' || !ch) { ! hold_n[n] = '\0'; ! hold_v[0] = '\0'; ! v = n = 0; ! state = COMMIT; ! } ! else if (isspace((unsigned char)ch)) ! continue; ! else if (ch == '=') { ! hold_n[n] = '\0'; ! v = n = 0; ! state = VALUE; ! } ! else ! hold_n[n++] = ch; ! break; ! case VALUE: ! if (v == 0 && ch == '\n') { ! hold_v[v] = '\0'; ! v = n = 0; ! state = COMMIT; ! } ! else if (v == 0 && isspace((unsigned char)ch)) ! continue; ! else if (ch == '{') { ! state = MVALUE; ! ++blevel; ! } ! else if (ch == '\n' || !ch) { ! hold_v[v] = '\0'; ! v = n = 0; ! state = COMMIT; ! } ! else { if (v >= PROPERTY_MAX_VALUE) { ! state = COMMENT; v = n = 0; ! break; } ! else hold_v[v++] = ch; ! } ! break; ! case MVALUE: ! /* multiline value */ ! if (v >= PROPERTY_MAX_VALUE) { ! warn("properties_read: value exceeds max length"); ! state = COMMENT; ! n = v = 0; ! } ! else if (ch == '}' && !--blevel) { ! hold_v[v] = '\0'; v = n = 0; ! state = COMMIT; ! } ! else { ! hold_v[v++] = ch; ! if (ch == '{') ! ++blevel; ! } ! break; - case COMMIT: - if (head == NULL) { - if ((head = ptr = property_alloc(hold_n, hold_v)) == NULL) - return (NULL); - } else { - if ((ptr->next = property_alloc(hold_n, hold_v)) == NULL) { - properties_free(head); - return (NULL); - } - ptr = ptr->next; } - state = LOOK; - v = n = 0; - break; - case STOP: ! /* we don't handle this here, but this prevents warnings */ break; } } --- 75,225 ---- char hold_v[PROPERTY_MAX_VALUE + 1]; char buf[BUFSIZ * 4]; int bp, n, v, max; ! enum { LOOK, COMMENT, NAME, VALUE, MVALUE, COMMIT } parse_state; ! enum { FILL, DATA, STOP } buffer_state; int ch = 0, blevel = 0; n = v = bp = max = 0; head = ptr = NULL; ! buffer_state = FILL; ! parse_state = LOOK; ! ! while (buffer_state != STOP) { ! ! if (parse_state != COMMIT) { if (bp == max) ! buffer_state = FILL; else ch = buf[bp++]; } ! ! switch(buffer_state) { case FILL: if ((max = read(fd, buf, sizeof buf)) < 0) { properties_free(head); return (NULL); } if (max == 0) { ! buffer_state = STOP; break; } else { ! buffer_state = DATA; ch = buf[0]; bp = 1; } ! /* Fall through since we now have data to process */ ! case DATA: ! switch(parse_state) { ! case LOOK: ! if (isspace((unsigned char)ch)) ! continue; ! /* Allow shell or lisp style comments */ ! else if (ch == '#' || ch == ';') { ! parse_state = COMMENT; ! continue; ! } ! else if (isalnum((unsigned char)ch) || ch == '_') { ! if (n >= PROPERTY_MAX_NAME) { ! n = 0; ! parse_state = COMMENT; ! } ! else { ! hold_n[n++] = ch; ! parse_state = NAME; ! } ! } ! else ! parse_state = COMMENT; /* Ignore the rest of the line */ ! break; ! case COMMENT: ! if (ch == '\n') ! parse_state = LOOK; ! break; ! ! case NAME: ! if (ch == '\n' || !ch) { ! hold_n[n] = '\0'; ! hold_v[0] = '\0'; ! v = n = 0; ! parse_state = COMMIT; } ! else if (isspace((unsigned char)ch)) ! continue; ! else if (ch == '=') { ! hold_n[n] = '\0'; ! v = n = 0; ! parse_state = VALUE; } ! else ! hold_n[n++] = ch; ! break; ! case VALUE: ! if (v == 0 && ch == '\n') { ! hold_v[v] = '\0'; ! v = n = 0; ! parse_state = COMMIT; ! } ! else if (v == 0 && isspace((unsigned char)ch)) ! continue; ! else if (ch == '{') { ! parse_state = MVALUE; ! ++blevel; ! } ! else if (ch == '\n' || !ch) { ! hold_v[v] = '\0'; ! v = n = 0; ! parse_state = COMMIT; ! } ! else { ! if (v >= PROPERTY_MAX_VALUE) { ! parse_state = COMMENT; ! v = n = 0; ! break; ! } ! else ! hold_v[v++] = ch; ! } ! break; ! case MVALUE: ! /* multiline value */ if (v >= PROPERTY_MAX_VALUE) { ! warn("properties_read: value exceeds max length"); ! parse_state = COMMENT; ! n = v = 0; ! } ! else if (ch == '}' && !--blevel) { ! hold_v[v] = '\0'; v = n = 0; ! parse_state = COMMIT; } ! else { hold_v[v++] = ch; ! if (ch == '{') ! ++blevel; ! } ! break; ! case COMMIT: ! if (head == NULL) { ! if ((head = ptr = property_alloc(hold_n, hold_v)) == NULL) ! return (NULL); ! } else { ! if ((ptr->next = property_alloc(hold_n, hold_v)) == NULL) { ! properties_free(head); ! return (NULL); ! } ! ptr = ptr->next; ! } ! parse_state = LOOK; v = n = 0; ! break; } case STOP: ! /* we don't handle these here, but this prevents warnings */ break; } } >Release-Note: >Audit-Trail: >Unformatted: