NetBSD Problem Report #57809

From www@netbsd.org  Mon Jan  1 22:03:46 2024
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id DB5E91A9238
	for <gnats-bugs@gnats.NetBSD.org>; Mon,  1 Jan 2024 22:03:45 +0000 (UTC)
Message-Id: <20240101220344.C07921A9239@mollari.NetBSD.org>
Date: Mon,  1 Jan 2024 22:03:44 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: FD_ZERO is type-unsafe
X-Send-Pr-Version: www-1.0

>Number:         57809
>Category:       lib
>Synopsis:       FD_ZERO is type-unsafe
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    lib-bug-people
>State:          analyzed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Jan 01 22:05:00 +0000 2024
>Closed-Date:    
>Last-Modified:  Tue May 27 23:55:01 +0000 2025
>Originator:     Taylor R Campbell
>Release:        current
>Organization:
The SetFD Foundation
>Environment:
>Description:
FD_ZERO in sys/select.h accepts arguments of any pointer type, not just fd_set *.

The other FD_* functions (FD_SET, FD_CLR, FD_ISSET) are also moderately type-unsafe, in that they accept a pointer to any struct or union that has a member called `fds_bits' of an appropriate type.
>How-To-Repeat:
$ cat pr.c
#include <sys/select.h>

fd_set readfds;

void
setup(int readfd)
{
	FD_ZERO(&readfd);
	FD_SET(readfd, &readfds);
}
$ make pr.o DBG=-g\ -O2\ -Wall\ -Wextra\ -Werror
cc -g -O2 -Wall -Wextra -Werror   -c pr.c
$ 

This should report a type error, but it does not.  Instead, because of the typo, it silently overwrites the local variable `readfd' and, for any input other than 0, puts the wrong fie descriptor into the fd set `readfds'.
>Fix:
Yes, please!

Maybe these should just be redefined as inline functions instead of as macros.

>Release-Note:

>Audit-Trail:
From: nia <nia@NetBSD.org>
To: riastradh@netbsd.org
Cc: gnats-bugs@netbsd.org
Subject: Re: PR/57809
Date: Sun, 25 May 2025 10:12:00 +0000

 Several things that popped up during testing:

 - Some software expects these to be defined by preprocessor 
   so that it can test their existence with #ifdef. This is
   fairly easy to work around by making them #defines pointing
   to the underlying functions.

 - Some software passes const data to FD_ISSET. POSIX seems to
   define the pointer argument to FD_ISSET as non-const (!?)

From: nia <nia@NetBSD.org>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: PR/57809
Date: Mon, 26 May 2025 11:13:20 +0000

 --+U+4QbQFsSI8vxg4
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: inline

 And here is the patch I ended up with.

 --+U+4QbQFsSI8vxg4
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: attachment; filename="fd_set.diff"

 Index: sys/sys/fd_set.h
 ===================================================================
 RCS file: /cvsroot/src/sys/sys/fd_set.h,v
 retrieving revision 1.8
 diff -u -p -r1.8 fd_set.h
 --- sys/sys/fd_set.h	12 May 2024 10:34:56 -0000	1.8
 +++ sys/sys/fd_set.h	26 May 2025 11:12:06 -0000
 @@ -67,24 +67,44 @@ typedef	struct fd_set {
  	__fd_mask	fds_bits[__NFD_SIZE];
  } fd_set;

 -#define	FD_SET(n, p)	\
 -    ((p)->fds_bits[(unsigned)(n) >> __NFDSHIFT] |= (1U << ((n) & __NFDMASK)))
 -#define	FD_CLR(n, p)	\
 -    ((p)->fds_bits[(unsigned)(n) >> __NFDSHIFT] &= ~(1U << ((n) & __NFDMASK)))
  #define	FD_ISSET(n, p)	\
      ((p)->fds_bits[(unsigned)(n) >> __NFDSHIFT] & (1U << ((n) & __NFDMASK)))
 +
 +static inline void __FD_SET(int n, fd_set *p)
 +{
 +	p->fds_bits[(unsigned)n >> __NFDSHIFT] |= (1U << (n & __NFDMASK));
 +}
 +
 +static inline void __FD_CLR(int n, fd_set *p)
 +{
 +	p->fds_bits[(unsigned)n >> __NFDSHIFT] &= ~(1U << (n & __NFDMASK));
 +}
 +
 +
  #if __GNUC_PREREQ__(2, 95)
 -#define	FD_ZERO(p)	(void)__builtin_memset((p), 0, sizeof(*(p)))
 +static inline void __FD_ZERO(fd_set *p)
 +{
 +	(void)__builtin_memset(p, 0, sizeof(*p));
 +}
  #else
 -#define	FD_ZERO(p)	do {						\
 -	fd_set *__fds = (p);						\
 -	unsigned int __i;						\
 -	for (__i = 0; __i < __NFD_SIZE; __i++)				\
 -		__fds->fds_bits[__i] = 0;				\
 -	} while (0)
 +static inline void __FD_ZERO(fd_set *p)
 +{
 +	unsigned int i;
 +
 +	for (i = 0; i < NFD_SIZE; i++)
 +		p->fds_bits[i] = 0;
 +}
  #endif /* GCC 2.95 */

  /*
 + * Some software expects them defined as macros and uses
 + * #ifdef to test their presence.
 + */
 +#define FD_SET __FD_SET
 +#define FD_CLR __FD_CLR
 +#define FD_ZERO __FD_ZERO
 +
 +/*
   * Expose our internals if we are not required to hide them.
   */
  #if defined(_NETBSD_SOURCE)

 --+U+4QbQFsSI8vxg4--

State-Changed-From-To: open->analyzed
State-Changed-By: nia@NetBSD.org
State-Changed-When: Mon, 26 May 2025 11:17:30 +0000
State-Changed-Why:


From: RVP <rvp@SDF.ORG>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: PR/57809
Date: Mon, 26 May 2025 11:55:51 +0000 (UTC)

 It's already doing the right thing in the `!__GNUC_PREREQ__(2, 95)' case.
 Why not do that everywhere:

 ```
 --- sys/fd_set.h.orig	2025-05-23 02:48:19.000000000 +0000
 +++ sys/fd_set.h	2025-05-26 11:41:36.167232304 +0000
 @@ -67,22 +67,24 @@
   	__fd_mask	fds_bits[__NFD_SIZE];
   } fd_set;

 -#define	FD_SET(n, p)	\
 -    ((p)->fds_bits[(unsigned)(n) >> __NFDSHIFT] |= (1U << ((n) & __NFDMASK)))
 -#define	FD_CLR(n, p)	\
 -    ((p)->fds_bits[(unsigned)(n) >> __NFDSHIFT] &= ~(1U << ((n) & __NFDMASK)))
 -#define	FD_ISSET(n, p)	\
 -    ((p)->fds_bits[(unsigned)(n) >> __NFDSHIFT] & (1U << ((n) & __NFDMASK)))
 -#if __GNUC_PREREQ__(2, 95)
 -#define	FD_ZERO(p)	(void)__builtin_memset((p), 0, sizeof(*(p)))
 -#else
 +#define	FD_SET(n, p)	do {						\
 +	fd_set *__fds = (p);						\
 +	((__fds)->fds_bits[(unsigned)(n) >> __NFDSHIFT] |= (1U << ((n) & __NFDMASK)));  \
 +	} while (0)
 +#define	FD_CLR(n, p)	do {						\
 +	fd_set *__fds = (p);						\
 +	((__fds)->fds_bits[(unsigned)(n) >> __NFDSHIFT] &= ~(1U << ((n) & __NFDMASK))); \
 +	} while (0)
 +#define	FD_ISSET(n, p)	do {						\
 +	fd_set *__fds = (p);						\
 +	((__fds)->fds_bits[(unsigned)(n) >> __NFDSHIFT] & (1U << ((n) & __NFDMASK)));   \
 +	} while (0)
   #define	FD_ZERO(p)	do {						\
   	fd_set *__fds = (p);						\
   	unsigned int __i;						\
   	for (__i = 0; __i < __NFD_SIZE; __i++)				\
   		__fds->fds_bits[__i] = 0;				\
   	} while (0)
 -#endif /* GCC 2.95 */

   /*
    * Expose our internals if we are not required to hide them.
 ```

 -RVP

From: nia <nia@NetBSD.org>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: PR/57809
Date: Mon, 26 May 2025 22:21:31 +0000

 On Mon, May 26, 2025 at 12:00:02PM +0000, RVP via gnats wrote:
 >  It's already doing the right thing in the `!__GNUC_PREREQ__(2, 95)' case.
 >  Why not do that everywhere:

 You've added a cast, but the goal is to generate a compiler warning
 when the wrong pointer type is used.

From: RVP <rvp@SDF.ORG>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: PR/57809
Date: Tue, 27 May 2025 23:53:08 +0000 (UTC)

 On Mon, 26 May 2025, nia via gnats wrote:

 > You've added a cast,
 >

 Not a cast: assignment (which is what function parameter passing devolves to
 in ANSI C anyway...).

 > but the goal is to generate a compiler warning when the wrong pointer
 > type is used.
 >

 With my patch:

 ```
 $ cc -c pr.c
 pr.c: In function 'setup':
 pr.c:8:9: warning: initialization of 'fd_set *' from incompatible pointer type 'int *' [-Wincompatible-pointer-types]
      8 |         FD_ZERO(&readfd);
        |         ^~~~~~~

 $ cc -Werror -c pr.c
 pr.c: In function 'setup':
 pr.c:8:9: error: initialization of 'fd_set *' from incompatible pointer type 'int *' [-Werror=incompatible-pointer-types]
      8 |         FD_ZERO(&readfd);
        |         ^~~~~~~
 cc1: all warnings being treated as errors

 $
 ```

 this is what you wanted, right?

 -RVP

>Unformatted:

Home	PR Database Search