[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0f78b9f0-e8fe-40a6-a4a0-f4f029605b49@brad-house.com>
Date: Mon, 19 Aug 2024 11:33:43 -0400
From: Brad House <brad@...d-house.com>
To: musl@...ts.openwall.com
Subject: Re: [PATCH 1/1] FD_SET() and FD_ISSET() warn on
-Wsign-conversion
On 8/15/24 3:58 PM, Brad House wrote:
> On 8/14/24 5:21 PM, Rich Felker wrote:
>> I don't think this is valid. While the standard allows them to be
>> functions or macros (I actually didn't remember that; I just looked it
>> up) normally that means they can be external functions or macros, not
>> static inline functions. This could be fixed just by having macros
>> that resolve to calls to static inline functions, though.
>>
>> Apart from that, I'm against the range checks. These destroy the
>> ability to diagnose code with missing bounds checks, which sanitizers
>> and static analysis tools could otherwise do.
>>
>> Finally, the identifier names fd and fdset are not valid to use in a
>> header. They are not in the reserved namespace and the including file
>> from the application could already have #define'd them to expand to
>> expressions not valid in the contexts you've used them in. Local
>> variables in static inline functions in standard headers have to be
>> __-prefixed.
>>
>> Overall, this approach is salvagable, but the patch as-written has
>> significant problems, and I'm still not sure it's the best path.
>>
>> Rich
>
> I'm not convinced the standard would require the use of a function
> with external linkage, especially if a macro is allowed. I can't see
> how any code using it might be any the wiser since it has to allow for
> both conditions to be compliant. If the developer's intent is to
> override FD_SET() or similar, that doesn't make sense either since it
> needs to be compatible with select() and doing implementation-specific
> things would mean they likely wouldn't be including sys/select.h
> anyhow. That said, like you said, if it really is a concern, using a
> macro to call the static inline would keep that "ability".
>
> The use of the __prefix on the parameters was missed, that's an easy
> change.
>
> On the final point of bounds checking based on FD_SETSIZE, I haven't
> seen any static analysis tools being able to properly detect
> FD_SETSIZE issues, afterall, the file descriptor assigned is dynamic.
> I've used clang's static analyzer through scan-build, Coverity, and
> SonarCloud ... none have ever reported any potential issues (even
> though they most definitely existed). That moves any detection to
> runtime via something like ASAN or Valgrind which can open a whole
> array of security issues when the developer doesn't have test cases
> covering file descriptors exceeding FD_SETSIZE, which opens up a whole
> array of security issues. Besides, this likely impacts old
> applications, anything modern has hopefully moved onto something
> else. We only use it in c-ares for supporting legacy integrations.
>
> -Brad
>
FYI, I just looked at android bionic and they do sanity check based on
FD_SETSIZE:
https://android.googlesource.com/platform/bionic/+/refs/heads/main/libc/include/sys/select.h#85
https://android.googlesource.com/platform/bionic/+/refs/heads/main/libc/bionic/fortify.cpp#91
https://android.googlesource.com/platform/bionic/+/refs/heads/main/libc/private/bionic_fortify.h#54
I can provide a new patch revision that should resolve your other
concerns, but I really think we should sanity check FD_SETSIZE,
honestly. Please let me know how to proceed.
-Brad
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.
