[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240814212148.GM10433@brightrain.aerifal.cx>
Date: Wed, 14 Aug 2024 17:21:49 -0400
From: Rich Felker <dalias@...c.org>
To: Brad House <brad@...d-house.com>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH 1/1] FD_SET() and FD_ISSET() warn on
-Wsign-conversion
On Thu, Aug 01, 2024 at 04:42:22PM -0400, Brad House wrote:
> On 8/1/24 3:06 PM, Brad House wrote:
> >On 7/18/24 1:35 PM, Thorsten Glaser wrote:
> >
> >>Rich Felker dixit:
> >>
> >>>Use of signed ints generates worse code (not just bitshift/mask, needs
> >>>fixup for C's wrong definition of signed integer division) and has
> >>>more-dangerous behavior in the event of a negative input (small
> >>>negative offset likely to clobber data in an exploitable way rather
> >>>than giant positive offset likely to crash).
> >>Aieee. I see, more reasons against signed integers in C :/
> >>
> >>#define FD_SET(d,s) ((s)->fds_bits[(0U + (d)) / (8 *
> >>sizeof(long))] |= \
> >> (1UL << ((0U + (d)) % (8 * sizeof(long)))))
> >>
> >Sorry it took me a while to reply on this. But no, this doesn't
> >resolve the issue, it still emits the same warning.
> >
> >-Brad
> >
> 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
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.
