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

[vdb at picaros.org]

> Am 31.08.2011 07:06, schrieb vdb at picaros.org:
> > [PATCH xserver/hw/xfree86/parser/scan.c] potential buffer overflow
> > 
> > The patch below fixes a potential buffer overflow in xf86addComment().  
> > This occurs if  curlen > 0 && eol_seen == 0 && iscomment == 0 , as 
> > follows from the code:
> > 
> > char *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)
> >                 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");
> > 
> > Signed-off-by: Servaas Vandenberghe
> > diff --git a/hw/xfree86/parser/scan.c b/hw/xfree86/parser/scan.c
> > index 1cff3bc..99b3257 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;
> > @@ -1118,14 +1118,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++] = '#';
> 
> 
> So far i understand is the code adding a commentstring ("#fusel") to a string ("blah")
> (stripping \n before .. ).

The code is actually inserting characters.  Simplifying, 

  if cur[], the current comment buffer, is not \n terminated ==> append \n

  if add[], the new comment, doesn't start with # ==> append # to cur[]

  then append add[] to cur[] via strcpy()

  if add is not \n terminated ==> strcat(cur, "\n")

The result cur[] thus looks like "#curmsg\n#addmsg\n".

> A more easy way could be using asprintf() like that
> 
> result=asprintf(&buf,"%s#%s\n",cur,add);
> free(cur); cur=result;

Yes, something like

  int err;
  const char *insnl, *inshash, *termnl;

  insnl = insnewline ? "\n" : "";
  inshash = iscomment ? "" : "#";
  termnl = endnewline ? "" : "\n";

  err = asprintf(&result, "%s%s%s%s%s", cur, insnl, inshash, add, termnl);
  if (err >= 0) {
    free(cur); 
    cur = result;
  }

would work.  But since I am not the author of this code I chose to stay as
close to the original work as possible.  Moreover, the code in parser/ 
seems to predate the use of asprintf() in Xorg.  IIRC asprintf() started 
appearing in 2010.

Just my opinion, Servaas Vandenberghe.