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.