On Sun, Oct 08, 2023 at 02:50:10PM -0300, Crystal Kolipe wrote: > Hi, > > On Sun, Oct 08, 2023 at 07:07:27PM +0200, Matthieu Herrb wrote: > > I can confirm that there's an issue here. However in my tests, I don't > > see a garbled error message. > > If I set MALLOC_OPTIONS=F then a double free is reported, which I > > tracked down to the realloc() calls in getNextLine() that will make > > the xf68_lex_val.str pointer point to already free()d memory. > > > > So the following patch, which invalidates this pointer before reading > > more data from the config file fixes the issue I'm seeing. Can you > > confirm that it also fixes it for you ? > > Unfortunately it doesn't :-). > > Or rather, yes and no... Your patch does fix most cases, but I'm > still able to read free'd memory with a specially crafted config > file that contains bare 0x0d bytes as line separators instead of 0x0a. >
Ok, after some more attempts, I have to agree that copying the token like you proposed originally is the only way to completely fix the issue. Here's a patch that also free()s the copy after it's been consumed, to avoid the (minor) leaks. Also submitted upstreams: https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1176 Comments, ok? Index: hw/xfree86/parser/DRI.c =================================================================== RCS file: /cvs/OpenBSD/xenocara/xserver/hw/xfree86/parser/DRI.c,v retrieving revision 1.8 diff -u -p -u -r1.8 DRI.c --- hw/xfree86/parser/DRI.c 8 Dec 2017 15:02:01 -0000 1.8 +++ hw/xfree86/parser/DRI.c 9 Oct 2023 05:56:22 -0000 @@ -77,6 +77,8 @@ xf86parseDRISection(void) break; case COMMENT: ptr->dri_comment = xf86addComment(ptr->dri_comment, xf86_lex_val.str); + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; break; default: Error(INVALID_KEYWORD_MSG, xf86tokenString()); Index: hw/xfree86/parser/Device.c =================================================================== RCS file: /cvs/OpenBSD/xenocara/xserver/hw/xfree86/parser/Device.c,v retrieving revision 1.10 diff -u -p -u -r1.10 Device.c --- hw/xfree86/parser/Device.c 27 Jul 2019 07:57:18 -0000 1.10 +++ hw/xfree86/parser/Device.c 9 Oct 2023 05:56:22 -0000 @@ -106,6 +106,8 @@ xf86parseDeviceSection(void) switch (token) { case COMMENT: ptr->dev_comment = xf86addComment(ptr->dev_comment, xf86_lex_val.str); + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; break; case IDENTIFIER: if (xf86getSubToken(&(ptr->dev_comment)) != STRING) Index: hw/xfree86/parser/Extensions.c =================================================================== RCS file: /cvs/OpenBSD/xenocara/xserver/hw/xfree86/parser/Extensions.c,v retrieving revision 1.5 diff -u -p -u -r1.5 Extensions.c --- hw/xfree86/parser/Extensions.c 8 Dec 2017 15:02:01 -0000 1.5 +++ hw/xfree86/parser/Extensions.c 9 Oct 2023 05:56:22 -0000 @@ -67,6 +67,8 @@ xf86parseExtensionsSection(void) case COMMENT: ptr->extensions_comment = xf86addComment(ptr->extensions_comment, xf86_lex_val.str); + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; break; default: Error(INVALID_KEYWORD_MSG, xf86tokenString()); Index: hw/xfree86/parser/Files.c =================================================================== RCS file: /cvs/OpenBSD/xenocara/xserver/hw/xfree86/parser/Files.c,v retrieving revision 1.9 diff -u -p -u -r1.9 Files.c --- hw/xfree86/parser/Files.c 8 Dec 2017 15:02:01 -0000 1.9 +++ hw/xfree86/parser/Files.c 9 Oct 2023 05:56:22 -0000 @@ -89,6 +89,8 @@ xf86parseFilesSection(void) switch (token) { case COMMENT: ptr->file_comment = xf86addComment(ptr->file_comment, xf86_lex_val.str); + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; break; case FONTPATH: if (xf86getSubToken(&(ptr->file_comment)) != STRING) Index: hw/xfree86/parser/Flags.c =================================================================== RCS file: /cvs/OpenBSD/xenocara/xserver/hw/xfree86/parser/Flags.c,v retrieving revision 1.11 diff -u -p -u -r1.11 Flags.c --- hw/xfree86/parser/Flags.c 11 Nov 2021 09:03:09 -0000 1.11 +++ hw/xfree86/parser/Flags.c 9 Oct 2023 05:56:22 -0000 @@ -98,6 +98,8 @@ xf86parseFlagsSection(void) switch (token) { case COMMENT: ptr->flg_comment = xf86addComment(ptr->flg_comment, xf86_lex_val.str); + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; break; /* * these old keywords are turned into standard generic options. @@ -436,17 +438,21 @@ xf86parseOption(XF86OptionPtr head) if ((token = xf86getSubToken(&comment)) == STRING) { option = xf86newOption(name, xf86_lex_val.str); option->opt_comment = comment; - if ((token = xf86getToken(NULL)) == COMMENT) + if ((token = xf86getToken(NULL)) == COMMENT) { option->opt_comment = xf86addComment(option->opt_comment, xf86_lex_val.str); - else + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; + } else xf86unGetToken(token); } else { option = xf86newOption(name, NULL); option->opt_comment = comment; - if (token == COMMENT) + if (token == COMMENT) { option->opt_comment = xf86addComment(option->opt_comment, xf86_lex_val.str); - else + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; + } else xf86unGetToken(token); } Index: hw/xfree86/parser/Input.c =================================================================== RCS file: /cvs/OpenBSD/xenocara/xserver/hw/xfree86/parser/Input.c,v retrieving revision 1.8 diff -u -p -u -r1.8 Input.c --- hw/xfree86/parser/Input.c 8 Dec 2017 15:02:01 -0000 1.8 +++ hw/xfree86/parser/Input.c 9 Oct 2023 05:56:22 -0000 @@ -84,6 +84,8 @@ xf86parseInputSection(void) switch (token) { case COMMENT: ptr->inp_comment = xf86addComment(ptr->inp_comment, xf86_lex_val.str); + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; break; case IDENTIFIER: if (xf86getSubToken(&(ptr->inp_comment)) != STRING) Index: hw/xfree86/parser/InputClass.c =================================================================== RCS file: /cvs/OpenBSD/xenocara/xserver/hw/xfree86/parser/InputClass.c,v retrieving revision 1.8 diff -u -p -u -r1.8 InputClass.c --- hw/xfree86/parser/InputClass.c 8 Dec 2017 15:02:01 -0000 1.8 +++ hw/xfree86/parser/InputClass.c 9 Oct 2023 05:56:22 -0000 @@ -191,6 +191,8 @@ xf86parseInputClassSection(void) switch (token) { case COMMENT: ptr->comment = xf86addComment(ptr->comment, xf86_lex_val.str); + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; break; case IDENTIFIER: if (xf86getSubToken(&(ptr->comment)) != STRING) Index: hw/xfree86/parser/Layout.c =================================================================== RCS file: /cvs/OpenBSD/xenocara/xserver/hw/xfree86/parser/Layout.c,v retrieving revision 1.8 diff -u -p -u -r1.8 Layout.c --- hw/xfree86/parser/Layout.c 8 Dec 2017 15:02:01 -0000 1.8 +++ hw/xfree86/parser/Layout.c 9 Oct 2023 05:56:22 -0000 @@ -101,6 +101,8 @@ xf86parseLayoutSection(void) switch (token) { case COMMENT: ptr->lay_comment = xf86addComment(ptr->lay_comment, xf86_lex_val.str); + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; break; case IDENTIFIER: if (xf86getSubToken(&(ptr->lay_comment)) != STRING) Index: hw/xfree86/parser/Module.c =================================================================== RCS file: /cvs/OpenBSD/xenocara/xserver/hw/xfree86/parser/Module.c,v retrieving revision 1.7 diff -u -p -u -r1.7 Module.c --- hw/xfree86/parser/Module.c 8 Dec 2017 15:02:01 -0000 1.7 +++ hw/xfree86/parser/Module.c 9 Oct 2023 05:56:22 -0000 @@ -95,6 +95,8 @@ xf86parseModuleSubSection(XF86LoadPtr he switch (token) { case COMMENT: ptr->load_comment = xf86addComment(ptr->load_comment, xf86_lex_val.str); + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; break; case OPTION: ptr->load_opt = xf86parseOption(ptr->load_opt); @@ -126,6 +128,8 @@ xf86parseModuleSection(void) switch (token) { case COMMENT: ptr->mod_comment = xf86addComment(ptr->mod_comment, xf86_lex_val.str); + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; break; case LOAD: if (xf86getSubToken(&(ptr->mod_comment)) != STRING) @@ -230,9 +234,11 @@ xf86addNewLoadDirective(XF86LoadPtr head new->ignore = 0; new->list.next = NULL; - if ((token = xf86getToken(NULL)) == COMMENT) + if ((token = xf86getToken(NULL)) == COMMENT) { new->load_comment = xf86addComment(new->load_comment, xf86_lex_val.str); - else + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; + } else xf86unGetToken(token); return ((XF86LoadPtr) xf86addListItem((glp) head, (glp) new)); Index: hw/xfree86/parser/Monitor.c =================================================================== RCS file: /cvs/OpenBSD/xenocara/xserver/hw/xfree86/parser/Monitor.c,v retrieving revision 1.11 diff -u -p -u -r1.11 Monitor.c --- hw/xfree86/parser/Monitor.c 11 Nov 2021 09:03:09 -0000 1.11 +++ hw/xfree86/parser/Monitor.c 9 Oct 2023 05:56:22 -0000 @@ -269,6 +269,8 @@ xf86parseVerboseMode(void) switch (token) { case COMMENT: ptr->ml_comment = xf86addComment(ptr->ml_comment, xf86_lex_val.str); + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; break; case DOTCLOCK: if ((token = xf86getSubToken(&(ptr->ml_comment))) != NUMBER) @@ -413,6 +415,8 @@ xf86parseMonitorSection(void) switch (token) { case COMMENT: ptr->mon_comment = xf86addComment(ptr->mon_comment, xf86_lex_val.str); + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; break; case IDENTIFIER: if (xf86getSubToken(&(ptr->mon_comment)) != STRING) @@ -599,6 +603,8 @@ xf86parseModesSection(void) switch (token) { case COMMENT: ptr->modes_comment = xf86addComment(ptr->modes_comment, xf86_lex_val.str); + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; break; case IDENTIFIER: if (xf86getSubToken(&(ptr->modes_comment)) != STRING) Index: hw/xfree86/parser/OutputClass.c =================================================================== RCS file: /cvs/OpenBSD/xenocara/xserver/hw/xfree86/parser/OutputClass.c,v retrieving revision 1.4 diff -u -p -u -r1.4 OutputClass.c --- hw/xfree86/parser/OutputClass.c 27 Jul 2019 07:57:18 -0000 1.4 +++ hw/xfree86/parser/OutputClass.c 9 Oct 2023 05:56:22 -0000 @@ -102,6 +102,8 @@ xf86parseOutputClassSection(void) switch (token) { case COMMENT: ptr->comment = xf86addComment(ptr->comment, xf86_lex_val.str); + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; break; case IDENTIFIER: if (xf86getSubToken(&(ptr->comment)) != STRING) Index: hw/xfree86/parser/Pointer.c =================================================================== RCS file: /cvs/OpenBSD/xenocara/xserver/hw/xfree86/parser/Pointer.c,v retrieving revision 1.8 diff -u -p -u -r1.8 Pointer.c --- hw/xfree86/parser/Pointer.c 8 Dec 2017 15:02:01 -0000 1.8 +++ hw/xfree86/parser/Pointer.c 9 Oct 2023 05:56:22 -0000 @@ -104,6 +104,8 @@ xf86parsePointerSection(void) switch (token) { case COMMENT: ptr->inp_comment = xf86addComment(ptr->inp_comment, xf86_lex_val.str); + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; break; case PROTOCOL: if (xf86getSubToken(&(ptr->inp_comment)) != STRING) Index: hw/xfree86/parser/Screen.c =================================================================== RCS file: /cvs/OpenBSD/xenocara/xserver/hw/xfree86/parser/Screen.c,v retrieving revision 1.10 diff -u -p -u -r1.10 Screen.c --- hw/xfree86/parser/Screen.c 8 Dec 2017 15:02:01 -0000 1.10 +++ hw/xfree86/parser/Screen.c 9 Oct 2023 05:56:22 -0000 @@ -119,6 +119,8 @@ xf86parseDisplaySubSection(void) switch (token) { case COMMENT: ptr->disp_comment = xf86addComment(ptr->disp_comment, xf86_lex_val.str); + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; break; case VIEWPORT: if (xf86getSubToken(&(ptr->disp_comment)) != NUMBER) @@ -256,6 +258,8 @@ xf86parseScreenSection(void) switch (token) { case COMMENT: ptr->scrn_comment = xf86addComment(ptr->scrn_comment, xf86_lex_val.str); + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; break; case IDENTIFIER: if (xf86getSubToken(&(ptr->scrn_comment)) != STRING) Index: hw/xfree86/parser/Vendor.c =================================================================== RCS file: /cvs/OpenBSD/xenocara/xserver/hw/xfree86/parser/Vendor.c,v retrieving revision 1.8 diff -u -p -u -r1.8 Vendor.c --- hw/xfree86/parser/Vendor.c 8 Dec 2017 15:02:01 -0000 1.8 +++ hw/xfree86/parser/Vendor.c 9 Oct 2023 05:56:22 -0000 @@ -98,6 +98,8 @@ xf86parseVendorSubSection(void) switch (token) { case COMMENT: ptr->vs_comment = xf86addComment(ptr->vs_comment, xf86_lex_val.str); + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; break; case IDENTIFIER: if (xf86getSubToken(&(ptr->vs_comment))) @@ -151,6 +153,8 @@ xf86parseVendorSection(void) switch (token) { case COMMENT: ptr->vnd_comment = xf86addComment(ptr->vnd_comment, xf86_lex_val.str); + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; break; case IDENTIFIER: if (xf86getSubToken(&(ptr->vnd_comment)) != STRING) Index: hw/xfree86/parser/Video.c =================================================================== RCS file: /cvs/OpenBSD/xenocara/xserver/hw/xfree86/parser/Video.c,v retrieving revision 1.8 diff -u -p -u -r1.8 Video.c --- hw/xfree86/parser/Video.c 8 Dec 2017 15:02:01 -0000 1.8 +++ hw/xfree86/parser/Video.c 9 Oct 2023 05:56:22 -0000 @@ -97,6 +97,8 @@ xf86parseVideoPortSubSection(void) switch (token) { case COMMENT: ptr->vp_comment = xf86addComment(ptr->vp_comment, xf86_lex_val.str); + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; break; case IDENTIFIER: if (xf86getSubToken(&(ptr->vp_comment)) != STRING) @@ -154,6 +156,8 @@ xf86parseVideoAdaptorSection(void) switch (token) { case COMMENT: ptr->va_comment = xf86addComment(ptr->va_comment, xf86_lex_val.str); + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; break; case IDENTIFIER: if (xf86getSubToken(&(ptr->va_comment)) != STRING) Index: hw/xfree86/parser/read.c =================================================================== RCS file: /cvs/OpenBSD/xenocara/xserver/hw/xfree86/parser/read.c,v retrieving revision 1.10 diff -u -p -u -r1.10 read.c --- hw/xfree86/parser/read.c 8 Dec 2017 15:02:01 -0000 1.10 +++ hw/xfree86/parser/read.c 9 Oct 2023 05:56:22 -0000 @@ -100,6 +100,8 @@ xf86readConfigFile(void) switch (token) { case COMMENT: ptr->conf_comment = xf86addComment(ptr->conf_comment, xf86_lex_val.str); + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; break; case SECTION: if (xf86getSubToken(&(ptr->conf_comment)) != STRING) { Index: hw/xfree86/parser/scan.c =================================================================== RCS file: /cvs/OpenBSD/xenocara/xserver/hw/xfree86/parser/scan.c,v retrieving revision 1.12 diff -u -p -u -r1.12 scan.c --- hw/xfree86/parser/scan.c 27 Jul 2019 07:57:18 -0000 1.12 +++ hw/xfree86/parser/scan.c 9 Oct 2023 05:56:22 -0000 @@ -278,9 +278,10 @@ xf86getToken(const xf86ConfigSymTabRec * if (!c) { char *ret; - if (numFiles > 0) + if (numFiles > 0) { + // xf86_lex_val.str = NULL; ret = xf86getNextLine(); - else { + } else { if (builtinConfig[builtinIndex] == NULL) ret = NULL; else { @@ -332,10 +333,10 @@ xf86getToken(const xf86ConfigSymTabRec * } while ((c != '\n') && (c != '\r') && (c != '\0')); configRBuf[i] = '\0'; - /* XXX no private copy. + /* XXX private copy. * Use xf86addComment when setting a comment. */ - xf86_lex_val.str = configRBuf; + xf86_lex_val.str = strdup(configRBuf); return COMMENT; } @@ -448,8 +449,11 @@ xf86getSubToken(char **comment) for (;;) { token = xf86getToken(NULL); if (token == COMMENT) { - if (comment) + if (comment) { *comment = xf86addComment(*comment, xf86_lex_val.str); + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; + } } else return token; @@ -464,8 +468,11 @@ xf86getSubTokenWithTab(char **comment, c for (;;) { token = xf86getToken(tab); if (token == COMMENT) { - if (comment) + if (comment) { *comment = xf86addComment(*comment, xf86_lex_val.str); + free(xf86_lex_val.str); + xf86_lex_val.str = NULL; + } } else return token; -- Matthieu Herrb
