Re: [musl] [PATCH 1/1] FD_SET() and FD_ISSET() warn on -Wsign-conversion

[Rich Felker <dalias@libc.org>]

On Mon, Aug 19, 2024 at 11:33:43AM -0400, Brad House wrote:
> 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.

A conditional to silently ignore UB will not be accepted. It needs to
actually perform the out-of-bounds access so compiling with
instrumentation can trap, or explicitly do something to trap itself
(but normally we only do that with fortify-headers).

Rich