Bug 41660 - xserver/hw/xfree86/parser/scan.c potential buffer overrun in xorg.conf parser
Summary: xserver/hw/xfree86/parser/scan.c potential buffer overrun in xorg.conf parser
Alias: None
Product: xorg
Classification: Unclassified
Component: Server/General (show other bugs)
Version: git
Hardware: All All
: medium normal
Assignee: Xorg Project Team
QA Contact: Xorg Project Team
Whiteboard: 2011BRB_Reviewed
Keywords: patch
Depends on:
Reported: 2011-10-10 11:10 UTC by vdb128
Modified: 2011-10-31 04:37 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:

Coherent buffer size calculation and buffer use. (1.26 KB, patch)
2011-10-10 11:10 UTC, vdb128
no flags Details | Splinter Review

Description vdb128 2011-10-10 11:10:55 UTC
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')
->	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:


The bug hasn't surfaced yet since all calls to xf86addComment() use a 
string add[] with a leading '#'.
Comment 1 vdb128 2011-10-10 11:27:37 UTC
Suggested commit log message:

This patch fixes a potential buffer overrun in xf86addComment().  The 
overrun occurs if the comment string to add doesn't start with a 
leading '#'.
Comment 2 Jeremy Huddleston Sequoia 2011-10-25 18:11:58 UTC
Please send this to xorg-devel for review.
Comment 3 vdb128 2011-10-31 04:37:18 UTC
On Mon, 31 Oct 2011 09:41:03 +1000, Peter Hutterer <peter.hutterer@who-t.net> wrote:
> Dave Airlie (1):
>       test: fix two more failing FP3232 tests
> Peter Hutterer (2):
>       xfree86: reduce calls to input_option_get_key/value
>       dix: block signals when closing all devices
> Servaas Vandenberghe (1):
>       xfree86: fix potential buffer overflow
   d0c6732..132545f  master -> master

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.