Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 10 Oct 2022 08:50:53 -0700
From: enh <enh@...gle.com>
To: libc-coord@...ts.openwall.com
Subject: Re: fgets behavior for n<=0 (and =1)

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.)

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>
> 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.