[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Date: Tue, 26 Apr 2016 18:18:15 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Removing stupid, spurious UB in stdio (bikeshed time)
There's a lot of nonsense-UB in stdio due to buffer comparisons along
the lines of "f->rpos < f->rend". The intent of these comparisons is
to simultaneously check that the buffer is initialized for the proper
mode (read or write) and that there's data left in it (for reading) or
space left (to write) or buffered data to be written out (for write),
etc.
Unfortunately, when the buffer is uninitialized for the mode being
checked, the comparison becomes NULL<NULL, and while this should
obviously be false (since < implies !=), NULL<NULL is actually UB.
The attached patch makes a stab at fixing the problem by using !=
instead of < or >. This is correct because in the places where we're
testing for <, > is not a possibility; to be true, > would require OOB
pointer arithmetic (thus UB) already to have been performed.
Unfortunately there are still places where UB is left: subtraction,
such as this line in ftell:
return pos - (f->rend - f->rpos) + (f->wpos - f->wbase);
Adding branches to test for all the null buffer possibilities here
would uglify and pessimize the code (compilers are too stupid to
optimize them out).
So another possible idea is abolishing use of null pointers here, and
using a different sentinel, e.g. f->buf (start of buffer). Then
instead of disabling write buffer via:
f->wpos = f->wbase = f->wend = 0;
we could do something like:
f->wpos = f->wbase = f->wend = f->buf;
Unfortunately there are several places that explicitly check
predicates like !f->rpos, and in some places they're treated
differently than end-of-buffer, such as ungetc:
if (!f->rpos) __toread(f);
if (!f->rpos || f->rpos <= f->buf - UNGET) {
Here, empty buffer after __toread is expected, but null pointer
indicates an error entering read mode. This could possibly be handled
by using f->buf-UNGET (which would naturally inhibit ungetc) as the
sentinel for read mode off.
The other major user of null sentinels is code in fseek.c and fflush.c
that checks for write failure; after f->write(), f->wpos==0 is
interpreted as an error. f->wpos==f->wend can't in general be used as
an error conditional (it's always true for unbuffered output) but it
could be used in these specific cases since the code path is only
reachable when there's buffered data to be flushed.
Also related: vsnprintf does some wacky UB stuff in order to support
direct writes into a buffer of unknown size, and strto* use similar
hacks to read from buffers of unknown size. So the right long-term
solution might be to eliminate the whole system of end pointers and
instead compare pos-start against a size. And on top of that I guess
we need to stop using null so pos-start is defined.
So what to do? I'm not aware of any immediate need to fix these
issues, but I want to future-proof the code and make it friendly to
analysis tools that catch the undefined behavior. I think a good place
to start might be coming up with and documenting a clear model for how
stdio's buffer internals are supposed to work, what operations are
allowed, what invariants hold, etc. based on the above analysis of
current UB issues and what the code is doing.
Rich
View attachment "stdio-buff-compare.diff" of type "text/plain" (4876 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.
