[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240718162541.GO10433@brightrain.aerifal.cx>
Date: Thu, 18 Jul 2024 12:25:41 -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, 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
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.
