Libguestfs - Mar 2023 - [libnbd PATCH v3 03/19] socket activation: avoid manipulating the sign bit

F_SETFD takes an "int", so it stands to reason that FD_CLOEXEC expands
to
an "int". In turn, it's bad hygiene to manipulate the sign bit of
(signed)
integers with bit operations:

  ~FD_CLOEXEC

Convert FD_CLOEXEC to "unsigned int" for the bitwise complement
operator:

  ~(unsigned)FD_CLOEXEC

The bitwise complement then results in an "unsigned int".
"Flags" (of type
"int", and having, per POSIX, a non-negative value returned by
fcntl(F_GETFD)) will be automatically converted to "unsigned int" by
the
usual arithmetic conversions for the bitwise AND operator:

  flags & ~(unsigned)FD_CLOEXEC

We could pass the result of *that* to fcntl(), due to (a) the value being
in range for "signed int" ("flags" is a non-negative
"int", and we're only
clearing a value bit), and (b) non-negative "int" values being
represented
identically by "unsigned int" (C99 6.2.5 p9). But, for clarity,
let's cast
the result to "int" explicitly:

  (int)(flags & ~(unsigned)FD_CLOEXEC)

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---

Notes:
    context:-U5

 generator/states-connect-socket-activation.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
index 61d3d1900f45..b9845c11a221 100644
--- a/generator/states-connect-socket-activation.c
+++ b/generator/states-connect-socket-activation.c
@@ -182,11 +182,11 @@  CONNECT_SA.START:
       int flags = fcntl (s, F_GETFD, 0);
       if (flags == -1) {
         nbd_internal_fork_safe_perror ("fcntl: F_GETFD");
         _exit (126);
       }
-      if (fcntl (s, F_SETFD, flags & ~FD_CLOEXEC) == -1) {
+      if (fcntl (s, F_SETFD, (int)(flags & ~(unsigned)FD_CLOEXEC)) == -1) {
         nbd_internal_fork_safe_perror ("fcntl: F_SETFD");
         _exit (126);
       }
     }

On Thu, Mar 23, 2023 at 01:10:00PM +0100, Laszlo Ersek
wrote:
> F_SETFD takes an "int", so it stands to reason that FD_CLOEXEC
expands to
> an "int". In turn, it's bad hygiene to manipulate the sign
bit of (signed)
> integers with bit operations:
> 
>   ~FD_CLOEXEC
Hmm - I've never really programmed on a system with ones' complement
encoding, but you a right that in C99, the ~ operator on a signed
value (even where we know the original value is positive) is bad
hygeine because it creates a result whose value is dependent on
whether the encoding is the usual two's complement or not;
furthermore, using a signed argument to & is risky.  Note, however,
that POSIX issue 8 (adding a restriction beyond on C17;
https://www.austingroupbugs.net/view.php?id=1108#c4094), as well as
C23 (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2412.pdf) will
require two's complement encoding, at which point what used to be bad
hygiene is now entirely predictable.
> 
> Convert FD_CLOEXEC to "unsigned int" for the bitwise complement
operator:
> 
>   ~(unsigned)FD_CLOEXEC
> 
> The bitwise complement then results in an "unsigned int".
"Flags" (of type
> "int", and having, per POSIX, a non-negative value returned by
> fcntl(F_GETFD)) will be automatically converted to "unsigned int"
by the
> usual arithmetic conversions for the bitwise AND operator:
> 
>   flags & ~(unsigned)FD_CLOEXEC
> 
> We could pass the result of *that* to fcntl(), due to (a) the value being
> in range for "signed int" ("flags" is a non-negative
"int", and we're only
> clearing a value bit), and (b) non-negative "int" values being
represented
> identically by "unsigned int" (C99 6.2.5 p9). But, for clarity,
let's cast
> the result to "int" explicitly:
> 
>   (int)(flags & ~(unsigned)FD_CLOEXEC)
Rather verbose.  If you have evidence of a current sanitizing compiler
that warns about the short construct (at compile- or run-time), that
would be a definitive reason to do this.  But given that future
standards will require the short form to work identically to the long
form, and I'm unaware of a compiler that actually warns on the short
form, I'm 50-50 on whether to take this one.  It's not technically
wrong, just not compelling.  But since it is only one line, it's easy
to maintain, so if you still want to include it, I don't mind if you
add:

Reviewed-by: Eric Blake <eblake at redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org