[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 17 Apr 2020 12:48:07 -0400
From: Rich Felker <dalias@...c.org>
To: Pascal Cuoq <cuoq@...st-in-soft.com>
Cc: "musl@...ts.openwall.com" <musl@...ts.openwall.com>
Subject: Re: Invalid pointer subtractions in __shlim and __shgetc
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
View attachment "shgetc_rpos_usage_fix.diff" of type "text/plain" (356 bytes)
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.
