[musl] [PATCH 1/1] FD_SET() and FD_ISSET() warn on -Wsign-conversion

[musl] [PATCH 1/1] FD_SET() and FD_ISSET() warn on -Wsign-conversion

[Brad House]

[-- Attachment #1: Type: text/plain, Size: 2672 bytes --]

I'm the maintainer of c-ares (https://c-ares.org) and have been scanning 
the CI build logs for various systems to catch warnings, and on Alpine 
Linux (which obviously uses musl c) we always get these warnings:

/tmp/cirrus-ci-build/src/lib/ares_event_select.c: In function 
'ares_evsys_select_wait':
/tmp/cirrus-ci-build/src/lib/ares_event_select.c:95:7: warning: 
conversion to 'long unsigned int' from 'ares_socket_t' {aka 'int'} may 
change the sign of the result [-Wsign-conversion]
    95 |       FD_SET(ev->fd, &read_fds);
       |       ^~~~~~
/tmp/cirrus-ci-build/src/lib/ares_event_select.c:95:7: warning: 
conversion to 'long unsigned int' from 'ares_socket_t' {aka 'int'} may 
change the sign of the result [-Wsign-conversion]
/tmp/cirrus-ci-build/src/lib/ares_event_select.c:98:7: warning: 
conversion to 'long unsigned int' from 'ares_socket_t' {aka 'int'} may 
change the sign of the result [-Wsign-conversion]
    98 |       FD_SET(ev->fd, &write_fds);
       |       ^~~~~~
/tmp/cirrus-ci-build/src/lib/ares_event_select.c:98:7: warning: 
conversion to 'long unsigned int' from 'ares_socket_t' {aka 'int'} may 
change the sign of the result [-Wsign-conversion]
/tmp/cirrus-ci-build/src/lib/ares_event_select.c:122:11: warning: 
conversion to 'long unsigned int' from 'ares_socket_t' {aka 'int'} may 
change the sign of the result [-Wsign-conversion]
   122 |       if (FD_ISSET(fdlist[i], &read_fds)) {
       |           ^~~~~~~~
/tmp/cirrus-ci-build/src/lib/ares_event_select.c:122:11: warning: 
conversion to 'long unsigned int' from 'ares_socket_t' {aka 'int'} may 
change the sign of the result [-Wsign-conversion]
/tmp/cirrus-ci-build/src/lib/ares_event_select.c:126:11: warning: 
conversion to 'long unsigned int' from 'ares_socket_t' {aka 'int'} may 
change the sign of the result [-Wsign-conversion]
   126 |       if (FD_ISSET(fdlist[i], &write_fds)) {
       |           ^~~~~~~~
/tmp/cirrus-ci-build/src/lib/ares_event_select.c:126:11: warning: 
conversion to 'long unsigned int' from 'ares_socket_t' {aka 'int'} may 
change the sign of the result [-Wsign-conversion]

full build output: 
https://cirrus-ci.com/task/6416020481507328?logs=main#L401

Sockets / File Descriptors in POSIX are defined as 'int', so it seems 
bad to have the standard C library complain when you pass an 'int' to a 
macro that should be expecting an 'int'.  I know -Wsign-conversion 
probably isn't a common warning flag to test with but its part of our 
default set of warnings.

I've attached a patch that will silence this warning, but otherwise not 
impact the behavior.

-Brad

[-- Attachment #2: musl-fd_set-warning-fix.patch --]
[-- Type: text/plain, Size: 1241 bytes --]

diff --git a/include/sys/select.h b/include/sys/select.h
index b3bab1d5..8982f39b 100644
--- a/include/sys/select.h
+++ b/include/sys/select.h
@@ -24,9 +24,9 @@ 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)))))
+#define FD_SET(d, s)   ((s)->fds_bits[((unsigned int)d)/(8*sizeof(long))] |= (1UL<<(((unsigned int)d)%(8*sizeof(long)))))
+#define FD_CLR(d, s)   ((s)->fds_bits[((unsigned int)d)/(8*sizeof(long))] &= ~(1UL<<(((unsigned int)d)%(8*sizeof(long)))))
+#define FD_ISSET(d, s) !!((s)->fds_bits[((unsigned int)d)/(8*sizeof(long))] & (1UL<<(((unsigned int)d)%(8*sizeof(long)))))
 
 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);

Re: [musl] [PATCH 1/1] FD_SET() and FD_ISSET() warn on -Wsign-conversion

[Szabolcs Nagy]

* Brad House <brad@brad-house.com> [2024-07-16 10:05:06 -0400]:
> I'm the maintainer of c-ares (https://c-ares.org) and have been scanning the
> CI build logs for various systems to catch warnings, and on Alpine Linux
> (which obviously uses musl c) we always get these warnings:
> 
> /tmp/cirrus-ci-build/src/lib/ares_event_select.c: In function
> 'ares_evsys_select_wait':
> /tmp/cirrus-ci-build/src/lib/ares_event_select.c:95:7: warning: conversion
> to 'long unsigned int' from 'ares_socket_t' {aka 'int'} may change the sign
> of the result [-Wsign-conversion]
>    95 |       FD_SET(ev->fd, &read_fds);
>       |       ^~~~~~
...
> full build output:
> https://cirrus-ci.com/task/6416020481507328?logs=main#L401
> 
> Sockets / File Descriptors in POSIX are defined as 'int', so it seems bad to
> have the standard C library complain when you pass an 'int' to a macro that
> should be expecting an 'int'.  I know -Wsign-conversion probably isn't a
> common warning flag to test with but its part of our default set of
> warnings.

note: -Wsign-conversion is not enabled by -Wall
nor by -Wextra.

> 
> I've attached a patch that will silence this warning, but otherwise not
> impact the behavior.
> 
> -Brad

> diff --git a/include/sys/select.h b/include/sys/select.h
> index b3bab1d5..8982f39b 100644
> --- a/include/sys/select.h
> +++ b/include/sys/select.h
> @@ -24,9 +24,9 @@ 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)))))
> +#define FD_SET(d, s)   ((s)->fds_bits[((unsigned int)d)/(8*sizeof(long))] |= (1UL<<(((unsigned int)d)%(8*sizeof(long)))))
> +#define FD_CLR(d, s)   ((s)->fds_bits[((unsigned int)d)/(8*sizeof(long))] &= ~(1UL<<(((unsigned int)d)%(8*sizeof(long)))))
> +#define FD_ISSET(d, s) !!((s)->fds_bits[((unsigned int)d)/(8*sizeof(long))] & (1UL<<(((unsigned int)d)%(8*sizeof(long)))))

the macro argument must be protected with ()
e.g. the patch changes behaviour if d is 0.5?x:y

and musl prefers 'unsigned' to 'unsigned int', so

#define FD_SET(d, s)   ((s)->fds_bits[(unsigned)(d)/(8*sizeof(long))] |= (1UL<<((unsigned)(d)%(8*sizeof(long)))))

note that this change can hide real bugs: now
int (d) is implicit converted to size_t and e.g
pointer types error out, but the cast suppresses
the error for pointers, not just the signedness
warning.

i think only cast can suppress the warning (at
least if we stick to standard c) and suspect
this is why the warning is not used widely: it
forces changes that make the code more buggy.

we used to argue that compilers should not warn
about system headers even if it's macro expansion
but when it's partly system header and partly
user code (d) then compilers tend to warn anyway.
i think for your project the cleanest is to wrap
FD_* and then you can do whatever.

Re: [musl] [PATCH 1/1] FD_SET() and FD_ISSET() warn on -Wsign-conversion

[Brad House]

[-- Attachment #1: Type: text/plain, Size: 1997 bytes --]

On 7/18/24 9:19 AM, Szabolcs Nagy wrote:

> the macro argument must be protected with ()
> e.g. the patch changes behaviour if d is 0.5?x:y
>
> and musl prefers 'unsigned' to 'unsigned int', so
>
> #define FD_SET(d, s)   ((s)->fds_bits[(unsigned)(d)/(8*sizeof(long))] |= (1UL<<((unsigned)(d)%(8*sizeof(long)))))
>
> note that this change can hide real bugs: now
> int (d) is implicit converted to size_t and e.g
> pointer types error out, but the cast suppresses
> the error for pointers, not just the signedness
> warning.
>
> i think only cast can suppress the warning (at
> least if we stick to standard c) and suspect
> this is why the warning is not used widely: it
> forces changes that make the code more buggy.
>
> we used to argue that compilers should not warn
> about system headers even if it's macro expansion
> but when it's partly system header and partly
> user code (d) then compilers tend to warn anyway.
> i think for your project the cleanest is to wrap
> FD_* and then you can do whatever.

I'm not sure about your statement above about a behavior change if d is 
0.5, file descriptors are required to be int, which can't represent 
0.5.  Passing in a garbage value would clearly fall into "Undefined 
Behavior".

We really can't wrap FD_SET/FD_ISSET in c-ares, since wrapping it would 
require the library to know the internals about the operations of the 
fd_set for each and every platform, which could change over time if 
there are ABI changes.

I understand your concern regarding casting away real programming errors 
by accident.  The only solution to that I see would be to turn the 
macros into functions, POSIX says either are valid: 
https://pubs.opengroup.org/onlinepubs/009604499/basedefs/sys/select.h.html

Of course, that would introduce a backwards-incompatible ABI change 
where an application compiled against a newer version of musl wouldn't 
run on an older version.  I don't know what your ABI policies look like 
to know if that's acceptable.

-Brad

[-- Attachment #2: Type: text/html, Size: 2575 bytes --]

Re: [musl] [PATCH 1/1] FD_SET() and FD_ISSET() warn on -Wsign-conversion

[Rich Felker]

On Thu, Jul 18, 2024 at 11:21:59AM -0400, Brad House wrote:
> On 7/18/24 9:19 AM, Szabolcs Nagy wrote:
> 
> >the macro argument must be protected with ()
> >e.g. the patch changes behaviour if d is 0.5?x:y
> >
> >and musl prefers 'unsigned' to 'unsigned int', so
> >
> >#define FD_SET(d, s)   ((s)->fds_bits[(unsigned)(d)/(8*sizeof(long))] |= (1UL<<((unsigned)(d)%(8*sizeof(long)))))
> >
> >note that this change can hide real bugs: now
> >int (d) is implicit converted to size_t and e.g
> >pointer types error out, but the cast suppresses
> >the error for pointers, not just the signedness
> >warning.
> >
> >i think only cast can suppress the warning (at
> >least if we stick to standard c) and suspect
> >this is why the warning is not used widely: it
> >forces changes that make the code more buggy.
> >
> >we used to argue that compilers should not warn
> >about system headers even if it's macro expansion
> >but when it's partly system header and partly
> >user code (d) then compilers tend to warn anyway.
> >i think for your project the cleanest is to wrap
> >FD_* and then you can do whatever.
> 
> I'm not sure about your statement above about a behavior change if d
> is 0.5, file descriptors are required to be int, which can't
> represent 0.5.  Passing in a garbage value would clearly fall into
> "Undefined Behavior".
> 
> We really can't wrap FD_SET/FD_ISSET in c-ares, since wrapping it
> would require the library to know the internals about the operations
> of the fd_set for each and every platform, which could change over
> time if there are ABI changes.
> 
> I understand your concern regarding casting away real programming
> errors by accident.  The only solution to that I see would be to
> turn the macros into functions, POSIX says either are valid: https://pubs.opengroup.org/onlinepubs/009604499/basedefs/sys/select.h.html
> 
> Of course, that would introduce a backwards-incompatible ABI change
> where an application compiled against a newer version of musl
> wouldn't run on an older version.  I don't know what your ABI
> policies look like to know if that's acceptable.

That's not acceptable, but I think you're confusing "functions" with
"functions with external linkage". There's no reason you can't do
functions with static linkage. However there is probably also some
fancy solution with the ternary operator that avoids the
type-unsafety. I haven't thought through the pros and cons of either
option.

What's really frustrating about these kinds of garbage warnings is
that they encourage (as in the proposed patch) writing casts that
*remove type-safety* and make very-wrong code silently compile "fine",
for the purpose of suppressing a warning that's supposedly about a
type-safety issue.

Rich

Re: [musl] [PATCH 1/1] FD_SET() and FD_ISSET() warn on -Wsign-conversion

[Thorsten Glaser]

Rich Felker dixit:

>What's really frustrating about these kinds of garbage warnings is
>that they encourage (as in the proposed patch) writing casts that
>*remove type-safety* and make very-wrong code silently compile "fine",
>for the purpose of suppressing a warning that's supposedly about a
>type-safety issue.

The warning *can* be useful, though.

You could always hide a cast-y version behind __extension__({…})
if GCC or compatible is detected (ugh).

For *this* specific case, it might be sufficient to instead cast
the constant term to signed (I can’t believe I found a case where
using signed ints, despite the UB danger, is the possible fix):

#define FD_SET(d,s)	((s)->fds_bits[(d) / (int)(8 * sizeof(long))] |= \
			    (1UL << ((d) % (int)(8 * sizeof(long)))))

bye,
//mirabilos
-- 
11:56⎜«liwakura:#!/bin/mksh» also, i wanted to add mksh to my own distro │
i was disappointed that there is no makefile │ but somehow the Build.sh is
the least painful built system i've ever seen │ honours CC, {CPP,C,LD}FLAGS
properly │ looks cleary like done by someone who knows what they are doing

Re: [musl] [PATCH 1/1] FD_SET() and FD_ISSET() warn on -Wsign-conversion

[Rich Felker]

On Thu, Jul 18, 2024 at 04:54:23PM +0000, Thorsten Glaser wrote:
> Rich Felker dixit:
> 
> >What's really frustrating about these kinds of garbage warnings is
> >that they encourage (as in the proposed patch) writing casts that
> >*remove type-safety* and make very-wrong code silently compile "fine",
> >for the purpose of suppressing a warning that's supposedly about a
> >type-safety issue.
> 
> The warning *can* be useful, though.

Yes, it can, but empirically this kind of warning leads to people
writing wrong casts that either suppress other useful diagnostics or
outright introduce bugs.

> You could always hide a cast-y version behind __extension__({…})
> if GCC or compatible is detected (ugh).

How about nope. :-)

> For *this* specific case, it might be sufficient to instead cast
> the constant term to signed (I can’t believe I found a case where
> using signed ints, despite the UB danger, is the possible fix):
> 
> #define FD_SET(d,s)	((s)->fds_bits[(d) / (int)(8 * sizeof(long))] |= \
> 			    (1UL << ((d) % (int)(8 * sizeof(long)))))

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).

Rich

Re: [musl] [PATCH 1/1] FD_SET() and FD_ISSET() warn on -Wsign-conversion

[Thorsten Glaser]

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)))))

Maybe this?

bye,
//mirabilos
-- 
Solange man keine schmutzigen Tricks macht, und ich meine *wirklich*
schmutzige Tricks, wie bei einer doppelt verketteten Liste beide
Pointer XORen und in nur einem Word speichern, funktioniert Boehm ganz
hervorragend.		-- Andreas Bogk über boehm-gc in d.a.s.r

Re: [musl] [PATCH 1/1] FD_SET() and FD_ISSET() warn on -Wsign-conversion

[Rich Felker]

On Thu, Jul 18, 2024 at 05:35:25PM +0000, 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)))))
> 
> Maybe this?

I would expect that to produce the exact same warning as promition via
division by an unsigned expression.

Rich

Re: [musl] [PATCH 1/1] FD_SET() and FD_ISSET() warn on -Wsign-conversion

[Brad House]

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

Re: [musl] [PATCH 1/1] FD_SET() and FD_ISSET() warn on -Wsign-conversion

[Thorsten Glaser]

Brad House dixit:

>> #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.

Mh. Thanks. Could always…

#define FD_SET(d,s) do {					\
	int FD_SET_fd = (d);					\
								\
	if (FD_SET_fd < 0 || FD_SET_fd >= FD_SETSIZE)		\
		abort();					\
	(s)->fds_bits[FD_SET_fd / (8 * sizeof(long))] |=	\
	    1UL << (FD_SET_fd % (8 * sizeof(long)));		\
} while (/* CONSTCOND */ 0)

… and, if there were¹ a suitable compile-time method to find out
whether a macro argument is a constant integer expression or not, use…
https://github.com/MirBSD/int/blob/6b2fe241e23bc52e428eb317bd531ce9cee2aea6/mbsdcc.h#L90-L98
… to force a compile-time abort if a constant value is out of range…

… but I can already hear dalias’ screams, so only half-kidding.

(Also, a library, even libc, calling abort(3) is usually bad taste,
although, perhaps, this is better, security-wise, than out-of-bounds
memory writes.)

Meow,
//mirabilos

① TTBOMK, there isn’t; famously, GCC’s
  __builtin_choose_expr(__builtin_constant_p(x), constbla(x), runtimebla(x))
  is utterly broken.
-- 
22:20⎜<asarch> The crazy that persists in his craziness becomes a master
22:21⎜<asarch> And the distance between the craziness and geniality is
only measured by the success 18:35⎜<asarch> "Psychotics are consistently
inconsistent. The essence of sanity is to be inconsistently inconsistent

Re: [musl] [PATCH 1/1] FD_SET() and FD_ISSET() warn on -Wsign-conversion

[Brad House]

[-- Attachment #1: Type: text/plain, Size: 1047 bytes --]

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

[-- Attachment #2: musl-fd_set-warning-fix-v2.patch --]
[-- Type: text/plain, Size: 1711 bytes --]

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);

Re: [musl] [PATCH 1/1] FD_SET() and FD_ISSET() warn on -Wsign-conversion

[Brad House]

On 8/1/24 4:42 PM, 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

Any feedback?

-Brad

Re: [musl] [PATCH 1/1] FD_SET() and FD_ISSET() warn on -Wsign-conversion

[Rich Felker]

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

Re: [musl] [PATCH 1/1] FD_SET() and FD_ISSET() warn on -Wsign-conversion

[Brad House]

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

Re: [musl] [PATCH 1/1] FD_SET() and FD_ISSET() warn on -Wsign-conversion

[Brad House]

On 8/15/24 3:58 PM, Brad House wrote:

> On 8/14/24 5:21 PM, Rich Felker wrote:
>> 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
>
FYI, I just looked at android bionic and they do sanity check based on 
FD_SETSIZE:

https://android.googlesource.com/platform/bionic/+/refs/heads/main/libc/include/sys/select.h#85

https://android.googlesource.com/platform/bionic/+/refs/heads/main/libc/bionic/fortify.cpp#91

https://android.googlesource.com/platform/bionic/+/refs/heads/main/libc/private/bionic_fortify.h#54

I can provide a new patch revision that should resolve your other 
concerns, but I really think we should sanity check FD_SETSIZE, 
honestly.  Please let me know how to proceed.

-Brad

Re: [musl] [PATCH 1/1] FD_SET() and FD_ISSET() warn on -Wsign-conversion

[Rich Felker]

On Mon, Aug 19, 2024 at 11:33:43AM -0400, Brad House wrote:
> On 8/15/24 3:58 PM, Brad House wrote:
> 
> >On 8/14/24 5:21 PM, Rich Felker wrote:
> >>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
> >
> FYI, I just looked at android bionic and they do sanity check based
> on FD_SETSIZE:
> 
> https://android.googlesource.com/platform/bionic/+/refs/heads/main/libc/include/sys/select.h#85
> 
> https://android.googlesource.com/platform/bionic/+/refs/heads/main/libc/bionic/fortify.cpp#91
> 
> https://android.googlesource.com/platform/bionic/+/refs/heads/main/libc/private/bionic_fortify.h#54
> 
> I can provide a new patch revision that should resolve your other
> concerns, but I really think we should sanity check FD_SETSIZE,
> honestly.  Please let me know how to proceed.

A conditional to silently ignore UB will not be accepted. It needs to
actually perform the out-of-bounds access so compiling with
instrumentation can trap, or explicitly do something to trap itself
(but normally we only do that with fortify-headers).

Rich

Re: [musl] [PATCH 1/1] FD_SET() and FD_ISSET() warn on -Wsign-conversion

[Brad House]

[-- Attachment #1: Type: text/plain, Size: 1626 bytes --]

On 7/18/24 12:25 PM, Rich Felker wrote:

>> We really can't wrap FD_SET/FD_ISSET in c-ares, since wrapping it
>> would require the library to know the internals about the operations
>> of the fd_set for each and every platform, which could change over
>> time if there are ABI changes.
>>
>> I understand your concern regarding casting away real programming
>> errors by accident.  The only solution to that I see would be to
>> turn the macros into functions, POSIX says either are valid:https://pubs.opengroup.org/onlinepubs/009604499/basedefs/sys/select.h.html
>>
>> Of course, that would introduce a backwards-incompatible ABI change
>> where an application compiled against a newer version of musl
>> wouldn't run on an older version.  I don't know what your ABI
>> policies look like to know if that's acceptable.
> That's not acceptable, but I think you're confusing "functions" with
> "functions with external linkage". There's no reason you can't do
> functions with static linkage. However there is probably also some
> fancy solution with the ternary operator that avoids the
> type-unsafety. I haven't thought through the pros and cons of either
> option.

You are right, replacing a complex macro with a static inline function 
in a header might be acceptable and keep full ABI compatibility.  That 
said, I've always considered static inlines in headers to be a bad 
practice in general, but there are always exceptions to the rule.

You'd also have the ability to do bounds checking with a function, such 
(d < 0 || d >= FD_SETSIZE) which would be good for preventing out of 
bounds reads and writes.

-Brad

[-- Attachment #2: Type: text/html, Size: 2235 bytes --]

Re: [musl] [PATCH 1/1] FD_SET() and FD_ISSET() warn on -Wsign-conversion

[Thorsten Glaser]

Brad House dixit:

> || d >= FD_SETSIZE) which would be good for preventing out of bounds reads and

FD_SET can be used on dynamically allocated fd sets > FD_SETSIZE.

Could you check whether the one I posted with the (0U + (d))
throws the same warnings in your use case?

bye,
//mirabilos
-- 
Gestern Nacht ist mein IRC-Netzwerk explodiert. Ich hatte nicht damit
gerechnet, darum bin ich blutverschmiert… wer konnte ahnen, daß SIE so
reagier’n… gestern Nacht ist mein IRC-Netzwerk explodiert~~~
	(as of 2021-06-15 The MirOS Project temporarily reconvenes on OFTC)

Re: [musl] [PATCH 1/1] FD_SET() and FD_ISSET() warn on -Wsign-conversion

[Rich Felker]

On Thu, Jul 18, 2024 at 10:00:08PM +0000, Thorsten Glaser wrote:
> Brad House dixit:
> 
> > || d >= FD_SETSIZE) which would be good for preventing out of bounds reads and
> 
> FD_SET can be used on dynamically allocated fd sets > FD_SETSIZE.

This is not valid usage.

    "The behavior of these macros is undefined if the fd argument is
    less than 0 or greater than or equal to FD_SETSIZE, or if fd is
    not a valid file descriptor, or if any of the arguments are
    expressions with side-effects."

And in the real world, AFAIK, it *will trap* with fortify enabled.
Programs that try to do this are broken.

select is simply not compatible with use of file descriptors >=
FD_SETSIZE. As such it should be considered deprecated and poll should
be used.

I used to think that, if you *really* insist on trying to bypass the
limit, the "safe" way would be not to pass n>=FD_SETSIZE but to make
an array of fd_set objects and address fd number n%FD_SETSIZE in index
n/FD_SETSIZE of the array, then pass a pointer to the array to select.
This works in practice everywhere that your hack would work. However,
on further review, any implementation where it works is nonconforming.
POSIX documents under ERRORS for select and pselect, a "SHALL FAIL"
condition:

    "[EINVAL]
    
    The nfds argument is less than 0 or greater than FD_SETSIZE."

This forbids accepting oversized fd_sets in any way.

I don't believe we are currently conforming on this, so we should
probably either add a check or open a request for interpretation
whether that (presumably contrary to many real-world systems) is
actually intended to be a requirement.

Rich

Re: [musl] [PATCH 1/1] FD_SET() and FD_ISSET() warn on -Wsign-conversion

[Szabolcs Nagy]

* Brad House <brad@brad-house.com> [2024-07-18 11:21:59 -0400]:
> On 7/18/24 9:19 AM, Szabolcs Nagy wrote:
> 
> > the macro argument must be protected with ()
> > e.g. the patch changes behaviour if d is 0.5?x:y
> 
> I'm not sure about your statement above about a behavior change if d is 0.5,
> file descriptors are required to be int, which can't represent 0.5.  Passing
> in a garbage value would clearly fall into "Undefined Behavior".

d is not 0.5 but 0.5?x:y which is an int with int x,y.

but my point was just that you should always put macro args in ().