Re: [PATCH 11/42] tools/nolibc/types: move the FD_* functions to macros in types.h

[Willy Tarreau <w@1wt.eu>]

Hi David,

On Mon, Feb 07, 2022 at 05:05:21PM +0000, David Laight wrote:
> From: Willy Tarreau
> > Sent: 07 February 2022 16:23
> > 
> > FD_SET, FD_CLR, FD_ISSET, FD_ZERO are supposed to be macros and not
> > functions.
> 
> Are you sure?
> I'd have thought they could be either.
> There are certainly systems where they are functions.

I've rechecked and you're right, they can be both. But I now remember
why I mentioned this, I used to have some early userland code that used
to redefine these based on #ifndef FD_CLR and that started to fail to
build depending on the include order (i.e. macro defined first, then
nolibc being included from another file).

> They can be implemented as an array of fd numbers rather than a bitmap.

I didn't think about the array of FDs but I see how that can be done,
indeed!

> > In addition we already have a file dedicated to such macros
> > and types used by syscalls, it's types.h, so let's move them
> > there and turn them to macros. FD_CLR() and FD_ISSET() were missing,
> > so they were added. FD_ZERO() now deals with its own loop so that it
> > doesn't rely on memset() that sets one byte at a time.
> > 
> ....
> > +#define FD_CLR(fd, set) do {                                            \
> > +		int __fd = (int)(fd);                                   \
> > +		fd_set *__set = (fd_set *)(set);                        \
> 
> I'm not sure you really want either cast.
> They are just likely to hide some horrid bugs.

I generally hate casts exactly for the reason you mentioned, but wanted
to stick as close as possible to the equivalent of the original function.
However now that there are these local variables the casts are not needed
anymore at all and I agree we should drop them. I'll do that.

> +		if (__fd >= 0 && __fd < FD_SETSIZE)                     \
> +			__set->fd32[__fd / 32] &= ~(1U << (__fd & 31)); \
> +	} while (0)
> +
> 
> Do you need the range check?
> I don't think glibc has one.
> Things just break in obscure ways when you use select on big fd.

Sadly, some versions of glibc have enforced such checks "recently".
I've seen code that used to work fine for 20 years with arbitrary
fd_set array lengths start to break because the libc was seeing some
out-of-bounds accesses, to the point that I finally completely stopped
using select() around 2013, and also stopped using FD_SET/FD_CLR to
manipulate arbitrary bitmaps and implement my own instead (see the
__FD_ELT macro in glibc for this).

The select(2) man page says:

  An fd_set is a fixed size buffer.  Executing FD_CLR() or FD_SET() with
  a value of fd that is negative or is equal to or larger than FD_SETSIZE
  will result in undefined behavior.  

The main reason for the check here in fact is to avoid annoying compiler
warnings such as -Warray-bounds and limit side effects if FDs are not
tested immediately before being passed to FD_SET (we're supposed to be
dealing with ugly early code where tests are often non-existent). I
think it's a reasonable compromise. I would also be fine with only
keeping the test on (fd >= 0) and dropping the one on FD_SETSIZE though.

Paul, I'll send a V2 of that patch once updated.

Thank you!
Willy