Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 13 Feb 2018 22:49:04 -0500
From: Dale Weiler <weilercdale@...il.com>
To: musl@...ts.openwall.com
Subject: Re: stdio review

>> fgetpos.c:
>>     fgetpos:                                                       [bug]
>>         using *(off_t*) to write _Int64 data into fpos_t structure when
>>         memcpy should be used, this breaks strict aliasing. Maybe add
>>         an off_t to the fpos_t union?

> My leaning would be to add the off_t, but the type might not be
> exposed and thus we would need to find a matching type that is
> exposed. memcpy would be the nicest solution, but only if we had a way
> of allowing the compiler to use builtin memcpy; otherwise it's a
> gratuitous call.

Seeing as the type _is_ exposed, adding the off_t to the union is likewise
the nicest solution. Getting the compiler to use it's builtin memcpy, while
using things like -fno-builtin seems more challenging here. If the type did
need to be hidden, there's always the possibility of using the __may_alias__
stuff that memcpy/memset do but that seems more gratuitous to me.

>> fmemopen.c:
>>     mseek:                                                       [style]
>>         It does goto upwards.

> I guess you could call it that, but it's into a block with no path
> out, so I don't think I would.

It's one of the rarer instances where goto is used unconventionally. I say
that because most uses of goto, especially in the case where they're
in response to an error go down.

>         Compound literal table to reference whence as a lookup table
>         as a single expression.

> I thought this was cute.

It's definitely cute, but it does depend on the seek argument being
one of the macro definitions in the [0, 2] range which I had to check,
obviously those have no reason to ever be anything but those values;
ABI and all but it's just additional mental load to ensure they weren't
hence why I brought it up.

>> fwrite.c:
>>     fwrite:                                                   [question]
>>         Should there be a check for size*nmemb overflow?

> This is actually a complicated topic. Formally, I think the C standard
> reads such that it would be valid for size*nmemb to exceed the size of
> the data object to be written if you somehow know you'll hit a write
> error before that happens. However real world implementations don't
> work like that. In particular, the kernel will error out with EFAULT
> if the buffer length extends past the valid userspace address range,
> even if the writes would never happen; the only way to avoid this
> would be to break longer-than-page writes down into separate
> page-sized writes. So I think for practical purposes, we have to
> interpret the standard as requiring that size*nmemb actually reflect
> the size of the object passed in, and in that case, the multiplication
> necessarily does not overflow. If there's an interpretation from WG14
> contrary to this, we'll have to revisit it.

> See also https://sourceware.org/bugzilla/show_bug.cgi?id=19165

That is an interesting and somewhat odd edge case. Maybe for the time
being a comment within here w.r.t it maybe needing to be revisited
wouldn't hurt. In either case it doesn't appear to be harming anything.

>> gets.c:
>>     gets:                                                     [optimize]
>>         The length of the string is calculated twice to strip the
>>         newline character off it. Why not rewrite it as:
>>         if (ret) { size_t i = strlen(s)-1; if (s[i] == '\n') s[i] = 0; }

> Seriously, this is gets. It's always unsafe, deprecated, removed from
> the current C standard. If it's gratuitously slow too, great. :-)

Yes it's gets, but fixing it for O(n) instead of O(n*2) does make the musl
static set slightly smaller, also makes programs using it crash twice as
fast ;-)

>> stdio_impl.h:                                                    [style]
>>     FUNLOCK macro has a trailing else which prompted me to look at every
>>     single instance of FUNLOCK to make sure all of them contained a
>>     semicolon. This is just dangerous, why not use the more common
>>     idiom of do { } while (0).

> Indeed that should be fine.

I think it's better understood by most folks as well, glad we're on the same
page w.r.t this one. At least then you cannot fail to forget the semicolon.

>> intscan.h:                                                       [style]
>>     It isn't apparent for why <stdio.h> needs to be included. Should
>>     just forward declare struct FILE; here instead.

> That would not work, because it's *not* struct FILE, it's FILE, which
> happens to be defined as "struct _IO_FILE", but that's an
> implementation detail. Including <stdio.h> is the clean way to have
> that.

I don't understand why you couldn't replicate that behavior. It's what
stdio.h already _does_ and seeing as the associated translation unit
already includes stdio.h it seems gratuitously excessive. It's just an
opaque pointer type being passed, how is a forward declaration
incorrect. Does C distinguish between "opaque T" and "opaque T"
with different underlying struct? If so I have many of code that needs
to be changed on my end.

>> floatscan.h:                                                     [style]
>>     It isn't apparent for why <stdio.h> needs to be included. Should
>>     just forward declare struct FILE; here instead.

> Same.

Same

Powered by blists - more mailing lists

Your e-mail address:

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.