Bug 41660

Created attachment 52193 [details] [review]
Coherent buffer size calculation and buffer use.

The text below describes a lurking bug in the xorg.conf parser code.  
Although a bit long, the description and bugfix are self-contained 
and straightforward.  

The bug is in xf86addComment(char *cur, char *add) which returns a 
string of the form cur[] = "#curmsg\n#addmsg\n" such that 
fprintf(fp, "%s", cur); yields a valid comment section.  This form 
is guaranteed by inserting characters if required.  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").

Thus the result cur[] always looks like "#curmsg\n#addmsg\n".  

The code calculates the new buffer size and then appends the new 
data.  It is this size calculation which is incorrect.  The bug can be 
seen from the code assuming there is already a comment in the current 
buffer.  Then  curlen > 0 && eol_seen == 0  such that

  add with leading #  ==>  iscomment == 1  ==>  size is one byte too many, 
  otherwise           ==>  iscomment == 0  ==>  size is one byte too short, 

as follows from the code:

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 is just a bugfix and tries to stay as close as possible to 
the original code.  It removes the TRUE == (int )1 assumption and 
adds the auxiliary variable 

  insnewline = eol_seen || (curlen && !hasnewline);

such that the new size calculation and actual append operation consist 
of the same three if() blocks.  References:

http://lists.x.org/archives/xorg-devel/2011-August/024775.html
http://lists.x.org/archives/xorg-devel/2011-September/025266.html

The bug hasn't surfaced yet since all calls to xf86addComment() use a 
string add[] with a leading '#'.