On Sat, Oct 14, 2023 at 07:14:29PM +0200, Matthieu Herrb wrote:
> 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?
Works fine for me, and I am unable to reproduce the original problem anymore
with this version of the patch so OK here.
There seems to be a stray comment left in, though, not sure if that was
intentional or not:
> 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 {
