[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
<PR0P264MB0794D2F2C7B66A32BBE77F7CC02B9@PR0P264MB0794.FRAP264.PROD.OUTLOOK.COM>
Date: Wed, 19 Oct 2022 04:04:20 +0000
From: Pascal Cuoq <cuoq@...st-in-soft.com>
To: "libc-coord@...ts.openwall.com" <libc-coord@...ts.openwall.com>
Subject: Re: fgets behavior for n<=0 (and =1)
It is probably too late for C23, but I can submit a C standard change making calling fgets with n<=0 implementation-defined. This would allow all current behaviors including the "you're holding it wrong" aborts.
I don't want to suggest to make it UB because then C compilers would start assuming n>0 when n has been an argument of fgets, with consequences that would, on paper(*), be worse than aborting (you all already know about this but see https://www.imperialviolet.org/2016/06/26/nonnull.html for exactly what I want to avoid).
ImperialViolet - memcpy (and friends) with NULL pointers<https://www.imperialviolet.org/2016/06/26/nonnull.html>
The C standard (ISO/IEC 9899:2011) has a sane-seeming definition of memcpy (section 7.24.2.1):. The memcpy function copies n characters from the object pointed to by s2 into the object pointed to by s1.. Apart from a prohibition on passing overlapping objects, I think every C programmer understands that.
www.imperialviolet.org
This would be an opportunity to make n=1 implementation-defined too, although I really do not see what rationale pushed some implementations to report failure in that case.
(*) most of this is theoretical but if we are going to have new revisions of the C standard, we might as well use then to fix the oddities that were put in in 1989.
Pascal
________________________________
From: enh <enh@...gle.com>
Sent: Monday, October 10, 2022 10:12 PM
To: libc-coord@...ts.openwall.com <libc-coord@...ts.openwall.com>
Subject: Re: [libc-coord] fgets behavior for n<=0 (and =1)
On Mon, Oct 10, 2022 at 1:11 PM enh <enh@...gle.com<mailto:enh@...gle.com>> wrote:
On Mon, Oct 10, 2022 at 12:51 PM Rich Felker <dalias@...c.org<mailto:dalias@...c.org>> wrote:
On Mon, Oct 10, 2022 at 08:50:53AM -0700, enh wrote:
> fwiw, bionic's fgets is just locking plus a call to fgets_unlocked(), which
> has this:
>
> char* fgets_unlocked(char* buf, int n, FILE* fp) {
> if (n <= 0) __fortify_fatal("fgets: buffer size %d <= 0", n);
>
> (we've added a lot of "fortify" stuff for "obviously invalid arguments that
> we've actually seen app bugs from in the wild" over the years.)
This is an argument in favor of being able to interpret it as
undefined, but in absence of it being undefined, this would of course
be nonconforming behavior. I'd be in favor of this for n<0 but I'm not
sure about n=0 -- I wonder if there are any applications intentionally
using n=0. Doesn't seem useful but perhaps it's a cute way to "wait
for the read-mode file no longer to be locked"..?
well, my point was that no _android_ applications are using n==0 :-)
to clarify: the scare quotes around fortify in my original mail were because you get these "you're holding it wrong" aborts regardless of what _FORTIFY_SOURCE you built with --- there's only one fgets() on Android, and it starts by making this check, and aborting if you fail it.
Rich
> On Fri, Oct 7, 2022 at 4:13 PM Rich Felker <dalias@...c.org<mailto:dalias@...c.org>> wrote:
>
> > On Fri, Oct 07, 2022 at 01:12:27PM +0000, Pascal Cuoq wrote:
> > > C/ Have undefined behavior for n=INT_MIN
> > >
> > > Since undefined behavior can be anything and worse, and the C
> > > standard's description does not explicitly allow “anything and
> > > worse” for any value of n, this is arguably incorrect.
> > >
> > > if (n--<=1) {
> > > https://git.musl-libc.org/cgit/musl/tree/src/stdio/fgets.c
> >
> > I think we should fix this. In general, doing arithmetic like this on
> > signed caller-provided values without first range checking is a bug
> > and we should probably have some kind of linting on that.
> >
> > With that said, my reading of the spec is that passing n<=0 is
> > underspecified and arguably UB, if not even required to do something
> > unsafe and wrong. The spec does not actually use n-1 as a max number
> > of bytes to write, but a limit on the number of characters to write.
> > Once that limit is hit, it's supposed to write a null terminator. So
> > after reading "at most" -1 or "at most" -2147483649<tel:(214)%20748-3649> <(214)%20748-3649>
> > bytes (thus 0
> > bytes), it clobbers a byte of the buffer you didn't intend it to.
> >
> > The POSIX text gratuitously differs and says "until n-1 bytes are
> > read". Since at no point have a negative number of bytes been read,
> > the terminating condition is never met, and fgets reads forever until
> > hitting a newline.
> >
> > This really should be filed as a defect against C, hopefully to get in
> > a fix for the upcoming standard.
> >
> > It's ridiculous and frustrating that the POSIX text is gratuitously
> > different in a way that makes it "more wrong" here, too...
> >
> > Rich
> >
Content of type "text/html" skipped
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.
