> > On Sat, Sep 17, 2011 at 06:58:18PM +0200, v...@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 <v...@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++] = '#'; _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel