From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_PASS autolearn=ham autolearn_force=no version=3.4.2 Received: (qmail 2443 invoked from network); 17 Apr 2020 17:57:05 -0000 Received-SPF: pass (mother.openwall.net: domain of lists.openwall.com designates 195.42.179.200 as permitted sender) receiver=inbox.vuxu.org; client-ip=195.42.179.200 envelope-from= Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with UTF8ESMTPZ; 17 Apr 2020 17:57:05 -0000 Received: (qmail 17585 invoked by uid 550); 17 Apr 2020 17:57:04 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 17564 invoked from network); 17 Apr 2020 17:57:03 -0000 Date: Fri, 17 Apr 2020 13:56:51 -0400 From: Rich Felker To: musl@lists.openwall.com Message-ID: <20200417175651.GK11469@brightrain.aerifal.cx> References: <1587138997905.95619@trust-in-soft.com> <20200417161351.GH11469@brightrain.aerifal.cx> <20200417164807.GI11469@brightrain.aerifal.cx> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200417164807.GI11469@brightrain.aerifal.cx> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] 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