[PATCH xserver/hw/xfree86/parser/scan.c] potential buffer overrun

[vdb at picaros.org]

> 
> On Sat, Sep 17, 2011 at 06:58:18PM +0200, vdb at picaros.org wrote:
> > This patch fixes a potential buffer overrun in xf86addComment().  The 
> > overrun occurs if the comment string to add doesn't start with a 
> > leading '#'.  
> 
> out of interest - how do you trigger this? Presumably xf86addComment should
> only be called for comments, which would require the leading '#'

The code which calculates the new buffer size is incorrect.  In the 
normal case, a leading '#', the reallocated buffer is one byte too 
long.  In the other case the buffer will be one byte short and an 
overrun occurs.

The code has explicit support for adding comments without a leading 
hash.  It is this support which causes the bug.  

It seems not good to have support for a feature which causes an 
overrun when used.  But indeed, no bug is seen since all calls to 
xf86addComment() include the leading '#' as read from xorg.conf.  

Just my view, Servaas Vandenberghe.

> > The bug can be seen from the code assuming 
> >  curlen > 0 && eol_seen == 0 && iscomment == 0 :
> > 
> > char *
> > xf86addComment(char *cur, char *add)
> > 
> > <...>
> > 
> > 	str = add;
> > ->	iscomment = 0;
> > 	while (*str) {
> > 	    if (*str != ' ' && *str != '\t')
> > 		break;
> > 	    ++str;
> > 	}
> > ->	iscomment = (*str == '#');
> > 
> >         len = strlen(add);
> >         endnewline = add[len - 1] == '\n';
> >         len +=  1 + iscomment + (!hasnewline) + (!endnewline) + eol_seen;
> >                     ^^^^^^^^^
> >         if ((str = realloc(cur, len + curlen)) == NULL)
> >                 return cur;
> > 
> >         cur = str;
> > 
> >         if (eol_seen || (curlen && !hasnewline))
> >                 cur[curlen++] = '\n';
> > ->      if (!iscomment)
> > ->              cur[curlen++] = '#';
> >         strcpy(cur + curlen, add);
> >         if (!endnewline)
> >                 strcat(cur, "\n");
> > 
> > The patch below is just a bugfix and tries to stay as close as 
> > possible to the original code.  See also: 
> >  http://lists.x.org/archives/xorg-devel/2011-August/024775.html .
> > 
> > Signed-off-by: Servaas Vandenberghe <vdb at picaros.org>
> > diff --git a/hw/xfree86/parser/scan.c b/hw/xfree86/parser/scan.c
> > index 1cff3bc..b188503 100644
> > --- a/hw/xfree86/parser/scan.c
> > +++ b/hw/xfree86/parser/scan.c
> > @@ -1093,7 +1093,7 @@ char *
> >  xf86addComment(char *cur, char *add)
> >  {
> >  	char *str;
> > -	int len, curlen, iscomment, hasnewline = 0, endnewline;
> > +	int len, curlen, iscomment, hasnewline = 0, insnewline, endnewline;
> >  
> >  	if (add == NULL || add[0] == '\0')
> >  		return cur;
> > @@ -1108,7 +1108,6 @@ xf86addComment(char *cur, char *add)
> >  		curlen = 0;
> >  
> >  	str = add;
> > -	iscomment = 0;
> >  	while (*str) {
> >  	    if (*str != ' ' && *str != '\t')
> >  		break;
> > @@ -1118,14 +1117,23 @@ xf86addComment(char *cur, char *add)
> >  
> >  	len = strlen(add);
> >  	endnewline = add[len - 1] == '\n';
> > -	len +=  1 + iscomment + (!hasnewline) + (!endnewline) + eol_seen;
> >  
> > -	if ((str = realloc(cur, len + curlen)) == NULL)
> > +	insnewline = eol_seen || (curlen && !hasnewline);
> > +	if (insnewline)
> > +		len++;
> > +	if (!iscomment)
> > +		len++;
> > +	if (!endnewline)
> > +		len++;
> > +
> > +	/* Allocate + 1 char for '\0' terminator. */
> > +	str = realloc(cur, curlen + len + 1);
> > +	if (!str)
> >  		return cur;
> >  
> >  	cur = str;
> >  
> > -	if (eol_seen || (curlen && !hasnewline))
> > +	if (insnewline)
> >  		cur[curlen++] = '\n';
> >  	if (!iscomment)
> >  		cur[curlen++] = '#';