From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/13263 Path: news.gmane.org!.POSTED!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: string-backed FILEs mess Date: Fri, 14 Sep 2018 11:52:27 -0400 Message-ID: <20180914155227.GL1878@brightrain.aerifal.cx> References: <20180912140239.GV1878@brightrain.aerifal.cx> <20180912150941.GB13976@voyager> <20180912154306.GW1878@brightrain.aerifal.cx> <20180912163345.GX1878@brightrain.aerifal.cx> <20180912171824.GZ1878@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: blaine.gmane.org 1536940239 9876 195.159.176.226 (14 Sep 2018 15:50:39 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Fri, 14 Sep 2018 15:50:39 +0000 (UTC) User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-13279-gllmg-musl=m.gmane.org@lists.openwall.com Fri Sep 14 17:50:34 2018 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1g0qMa-0002S4-8v for gllmg-musl@m.gmane.org; Fri, 14 Sep 2018 17:50:32 +0200 Original-Received: (qmail 14307 invoked by uid 550); 14 Sep 2018 15:52:40 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 14286 invoked from network); 14 Sep 2018 15:52:40 -0000 Content-Disposition: inline In-Reply-To: <20180912171824.GZ1878@brightrain.aerifal.cx> Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:13263 Archived-At: On Wed, Sep 12, 2018 at 01:18:24PM -0400, Rich Felker wrote: > On Wed, Sep 12, 2018 at 12:33:45PM -0400, Rich Felker wrote: > > OK, I've been properly initializing the FILE rather than leaving it > > uninitialized except for the important fields like the old code did. > > Changing that, it's 1.44s with step 8, 1.60s with step 24. I also > > confirmed that this version of the code is almost as fast as the > > existing code with the memchr removed (just assuming it can read > > ahead). > > Uhg, the source of the "almost" here makes me even more convinced the > current code must go. Part of the reason it's not as fast was that I > was still setting f.read=__string_read, which requires (this is on > i386, 32-bit) setting up the GOT pointer. > > What was the old code doing? f.read was uninitialized. But the new > code crashes in that case when hitting the end of the string. Why > doesn't the old code crash? Because f.rend is set way past the end of > the string and never reached. If it were reached: > > 1. The shgetc macro calls the __shgetc function. > 2. The __shgetc function calls __uflow. > 3. __uflow calls __toread. > 4. __toread inspects uninitialized f->wpos/f->wbase fields and, > depending on the values it sees, calls f->write, which is also > uninitialized. > 5. If it gets past that, next __uflow calls the uninitialized f->read. > > The fact that any of this works at all is a fragile shit-show, and > completely depends on __intscan/__floatscan just using (via shgetc) a > tiny undocumented subset of the FILE structure and a tiny undocumented > subset of the stdio interfaces on it. > > Really the existing code is just a poor substitute for having an > abstraction for windowed string iteration, using the stdio FILE > structure in a way that also works with real FILEs. It's clever, but > this kind of clever is not a good thing. > > I'm still not sure what the right way forward is, though. OK, a small breakthrough that makes this mess a lot simpler: The __intscan and __floatscan backends do not (and are not allowed to, but this should be documented and isn't) call any stdio functions on the fake FILE* passed to them except for the shgetc and shunget macros, defined as: #define shgetc(f) (((f)->rpos < (f)->shend) ? *(f)->rpos++ : __shgetc(f)) #define shunget(f) ((f)->shend ? (void)(f)->rpos-- : (void)0) If the < is merely replaced by !=, which is functionally equivalent, then shend can be any type of sentinel pointer we want (e.g. (void*)-1 or even just &localvar) to use the buffer as a string with no known length, and we have a guarantee that __shgetc is never called. I think this -1+2-byte change is an acceptable resolution to the issue for now. I did however perform some casual performance testing to determine that strtol on short numbers would be 30% faster if it operated directly on strings only rather than going through the fake FILE and having to check end pointer and including code to conditionally call __shgetc. So it might make sense in the longer term to redesign the interface to operate only on strings, and have scanf be responsible for transforming the input to a bounded-size string that will produce the same result. (Note: vfscanf could shortcut out of having to do this if it could detect that its underlying FILE is a vsscanf string buffer.) Such a redesign would also reduce the static linking cost for programs that use strto* but not stdio input functions. Rich