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 26561 invoked from network); 17 Apr 2020 16:48:21 -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 16:48:21 -0000 Received: (qmail 23943 invoked by uid 550); 17 Apr 2020 16:48:19 -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 23922 invoked from network); 17 Apr 2020 16:48:19 -0000 Date: Fri, 17 Apr 2020 12:48:07 -0400 From: Rich Felker To: Pascal Cuoq Cc: "musl@lists.openwall.com" Message-ID: <20200417164807.GI11469@brightrain.aerifal.cx> References: <1587138997905.95619@trust-in-soft.com> <20200417161351.GH11469@brightrain.aerifal.cx> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="ILuaRSyQpoVaJ1HG" Content-Disposition: inline In-Reply-To: <20200417161351.GH11469@brightrain.aerifal.cx> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] Invalid pointer subtractions in __shlim and __shgetc --ILuaRSyQpoVaJ1HG Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 --ILuaRSyQpoVaJ1HG Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="shgetc_rpos_usage_fix.diff" 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; --ILuaRSyQpoVaJ1HG--