[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <33acd42b-a5e7-45ca-9be0-851daefa056e@brad-house.com>
Date: Thu, 15 Aug 2024 15:58:22 -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/14/24 5:21 PM, Rich Felker wrote:
> On Thu, Aug 01, 2024 at 04:42:22PM -0400, Brad House wrote:
>>
>> As a follow-up, here's a new patch that [maybe?] resolves the issues
>> you had with my first patch.
>>
>> It uses static inlines to accomplish the task that should still
>> provide relevant warnings to integrators.
>>
>> Please see attached.
>>
>> -Brad
>>
>> diff --git a/include/sys/select.h b/include/sys/select.h
>> index b3bab1d5..387c2a26 100644
>> --- a/include/sys/select.h
>> +++ b/include/sys/select.h
>> @@ -24,9 +24,32 @@ typedef struct {
>> } fd_set;
>>
>> #define FD_ZERO(s) do { int __i; unsigned long *__b=(s)->fds_bits; for(__i=sizeof (fd_set)/sizeof (long); __i; __i--) *__b++=0; } while(0)
>> -#define FD_SET(d, s) ((s)->fds_bits[(d)/(8*sizeof(long))] |= (1UL<<((d)%(8*sizeof(long)))))
>> -#define FD_CLR(d, s) ((s)->fds_bits[(d)/(8*sizeof(long))] &= ~(1UL<<((d)%(8*sizeof(long)))))
>> -#define FD_ISSET(d, s) !!((s)->fds_bits[(d)/(8*sizeof(long))] & (1UL<<((d)%(8*sizeof(long)))))
>> +
>> +/* Make FD_ISSET(), FD_SET(), and FD_CLR() into static inline functions in order
>> + * to silence -Wsign-conversion warnings */
>> +#define __FD_ISSET(d, s) !!((s)->fds_bits[(d)/(8*sizeof(long))] & (1UL<<((d)%(8*sizeof(long)))))
>> +static inline int FD_ISSET(int fd, fd_set *fdset)
>> +{
>> + if (fd < 0 || fd >= FD_SETSIZE)
>> + return 0;
>> + return __FD_ISSET((unsigned)fd, fdset);
>> +}
>> +
>> +#define __FD_SET(d, s) ((s)->fds_bits[(d)/(8*sizeof(long))] |= (1UL<<((d)%(8*sizeof(long)))))
>> +static inline void FD_SET(int fd, fd_set *fdset)
>> +{
>> + if (fd < 0 || fd >= FD_SETSIZE)
>> + return;
>> + __FD_SET((unsigned)fd, fdset);
>> +}
>> +
>> +#define __FD_CLR(d, s) ((s)->fds_bits[(d)/(8*sizeof(long))] &= ~(1UL<<((d)%(8*sizeof(long)))))
>> +static inline void FD_CLR(int fd, fd_set *fdset)
>> +{
>> + if (fd < 0 || fd >= FD_SETSIZE)
>> + return;
>> + __FD_CLR((unsigned)fd, fdset);
>> +}
>>
>> int select (int, fd_set *__restrict, fd_set *__restrict, fd_set *__restrict, struct timeval *__restrict);
>> int pselect (int, fd_set *__restrict, fd_set *__restrict, fd_set *__restrict, const struct timespec *__restrict, const sigset_t *__restrict);
> 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
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.
