[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 17 Apr 2020 13:56:51 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: Invalid pointer subtractions in __shlim and __shgetc
On Fri, Apr 17, 2020 at 12:48:07PM -0400, Rich Felker wrote:
> On Fri, Apr 17, 2020 at 12:13:51PM -0400, Rich Felker wrote:
> > On Fri, Apr 17, 2020 at 03:56:06PM +0000, Pascal Cuoq wrote:
> > > ?Hello,
> > >
> > > both functions `__shlim` and `__shgetc` subtract the members
> > > named `buf` and `rpos` of the struct they manipulate.
> > >
> > > In `__shlim`, this happens in the statement `f->shcnt = f->buf - f->rpos;`.
> > > And in `__shgetc`, in happens inside the `shcnt` macro:
> > >
> > > #define shcnt(f) ((f)->shcnt + ((f)->rpos - (f)->buf))
> > >
> > > In our tests, while running `testsuite` in `libc-testsuite`,
> > > both the `__shlim` and `__shgetc` functions are reached
> > > with `f->buf` non-null and `f->rpos` a null pointer.
> > >
> > > This can be made visible on execution platforms other than ours
> > > by adding a statement at the beginning of the functions:
> > >
> > > + if (f->buf && !f->rpos) dprintf (2, "XXX Problem in __shlim\n");
> > > + if (f->buf && !f->rpos) dprintf (2, "XXX Problem in __shgetc\n");
> > >
> > > Then if, running `libc-testsuite`, you see the following, it means that
> > > `f->buf` was non-null and `f->rpos` was null when these points were
> > > reached:
> > >
> > > $ ./testsuite
> > > fdopen test passed
> > > fcntl test passed
> > > fnmatch test passed
> > > XXX Problem in __shlim
> > > XXX Problem in __shgetc
> > > XXX Problem in __shlim
> > > XXX Problem in __shgetc
> > > XXX Problem in __shlim
> > > XXX Problem in __shgetc
> > > XXX Problem in __shlim
> > > XXX Problem in __shgetc
> > > XXX Problem in __shlim
> > > XXX Problem in __shgetc
> > > XXX Problem in __shlim
> > > XXX Problem in __shgetc
> > > fscanf test passed
> > > (...)
> > >
> > > This has been tested on the (tag: v1.2.0) branch of git://git.musl-libc.org/musl
> > >
> > > These pointer subtractions are undefined behavior. This is slightly worse
> > > than computing `(char*)0-(char*)0`, which is undefined in C and defined in C++,
> > > because compilers for both C and C++ are unlikely to exploit this one
> > > for optimization. Subtracting between a non-null pointer and a null pointer
> > > on the other hand is undefined behavior in both languages, and it is
> > > plausible that doing it may someday have unexpected consequences.
> > >
> > > I mention this because similar undefined behaviors that were extremely
> > > unlikely to cause harm have been fixed in musl in recent months,
> > > so that this looks like something you may want to fix too.
> >
> > Absolutely. Do you have an analysis of how this is reached? Neither of
> > these should be called when the FILE is not in suitable state for
> > reading. It might just be that vfscanf needs to call __toread on the
> > FILE before starting and error out if it fails.
>
> Indeed I think the attached fixes it.
>
> Rich
> diff --git a/src/stdio/vfscanf.c b/src/stdio/vfscanf.c
> index 9e030fc4..d990db9f 100644
> --- a/src/stdio/vfscanf.c
> +++ b/src/stdio/vfscanf.c
> @@ -76,6 +76,8 @@ int vfscanf(FILE *restrict f, const char *restrict fmt, va_list ap)
>
> FLOCK(f);
>
> + if (!f->rpos && __toread(f)) goto input_fail;
> +
> for (p=(const unsigned char *)fmt; *p; p++) {
>
> alloc = 0;
I think this patch may result in wrong error behavior on a trivial
scanf that doesn't try to read anything. Instead it should be:
if (!f->rpos) __toread(f);
if (!f->rpos) goto input_fail;
so that the error path is taken only on failure to enter read mode,
not on EOF.
If this works on in my tests I'll commit it.
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
