From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: from second.openwall.net (second.openwall.net [193.110.157.125]) by inbox.vuxu.org (Postfix) with SMTP id 16629216DF for ; Wed, 14 Aug 2024 23:22:02 +0200 (CEST) Received: (qmail 10020 invoked by uid 550); 14 Aug 2024 21:21:56 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 9986 invoked from network); 14 Aug 2024 21:21:56 -0000 Date: Wed, 14 Aug 2024 17:21:49 -0400 From: Rich Felker To: Brad House Cc: musl@lists.openwall.com Message-ID: <20240814212148.GM10433@brightrain.aerifal.cx> References: <10dead9f-55f9-4426-adcb-1d25769e6fc0@brad-house.com> <20240718131949.GQ3766212@port70.net> <0c1ddb63-8bb9-4bfc-918d-3fa4f55fcfa4@brad-house.com> <20240718162541.GO10433@brightrain.aerifal.cx> <20240718173129.GP10433@brightrain.aerifal.cx> <3a1e3c19-a150-4400-ae80-8a4f56a951dd@brad-house.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [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