[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAJgzZor54wSAYMbjHE3ozFs-WAjAuh3pzARBJjddXVwh3sh+CA@mail.gmail.com>
Date: Wed, 19 Oct 2022 11:10:32 -0700
From: enh <enh@...gle.com>
To: libc-coord@...ts.openwall.com
Subject: Re: fgets behavior for n<=0 (and =1)
On Tue, Oct 18, 2022 at 9:04 PM Pascal Cuoq <cuoq@...st-in-soft.com> wrote:
> 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).
>
+100
Android was burned badly by the compiler nullability attributes around that
time too. we added them to our headers for the warnings, but had to revert
them a few months later because the compiler (gcc at the time) took it as
an excuse to remove all the null checks from the implementation. i can
still name the most popular game it broke, and the "wtf?" moment when we
realized what the problem was...
i think the _newer_ nullability attributes do let you say what we meant
"warn the user, but don't screw anyone over", but no-one's had the stomach
to work on that again since :-(
> 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> wrote:
>
>
>
> On Mon, Oct 10, 2022 at 12:51 PM Rich Felker <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> 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 <(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.
