[prev in list] [next in list] [prev in thread] [next in thread] 

List:       libguestfs
Subject:    Re: [Libguestfs] [libnbd PATCH v3 03/19] socket activation: avoid manipulating the sign bit
From:       Laszlo Ersek <lersek () redhat ! com>
Date:       2023-03-23 15:23:11
Message-ID: 81ed0daf-fe92-5676-1568-61411b0f270d () redhat ! com
[Download RAW message or body]

On 3/23/23 14:59, Eric Blake wrote:
> 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@redhat.com>
> 

Thanks, I'll include it then.

The reason I'm particularly pedantic about hygiene here is the
following. Quite frequently, you'll see a pattern like:

  flags &= ~FLAG_MASK_WHATEVER;

Unfortunately, whether this is correct or bogus cannot be determined at
face value. I've seen *multiple bugs* where:

- "flags" was of type "unsigned long" or "unsigned long long" (that is,
it had an integer conversion rank higher than that of "int"), AND (:

  - FLAG_MASK_WHATEVER had type int (such as from
    #define FLAG_MASK_WHATEVER 0x1), OR

  - FLAG_MASK_WHATEVER had type unsigned int (such as from
    #define FLAG_MASK_WHATEVER 0x1u)).

Now, consider what happens:

- If FLAG_MASK_WHATEVER has type unsigned int (0x1u), then
~FLAG_MASK_WHATEVER also has type unsigned int, and the assignment ends
up clearing even those bits from "flags" that an unsigned long (or
unsigned long long) can represent, but an unsigned int cannot. Boom, bug
("flip-before-widen").

- If FLAG_MASK_WHATEVER has type int (0x1), then ~FLAG_MASK_WHATEVER has
type int, and it has a negative value (because the sign bit gets
negated, so it becomes negative, regardless of one's complement vs.
sign-and-magnitude vs. two's complement representation). Assume no
padding bits, and two's complement; okay. Then this negative value --
representing the current example in a 32-bit int: 0xFFFF_FFFE, i.e. (-2)
-- gets converted to unsigned long, as a part of the usual arithmetic
conversions, for the sake of the & operator hidden inside the &=
compound assignment operator. The conversion is well-defined, and the
result -- per standard -- is (ULONG_MAX+1)-2 == (ULONG_MAX-1) =
0xFFFF_FFFF_FFFF_FFFE. This *happens* to be the proper mask, and it will
clear just the right bit from "flags".

People that play mostly with assembly and microarchitecture like to call
these things "zero-extend" and "sign-extend", and while that's right at
the uarch/asm level, it's not proper talk at the C language level IMO.
At the C language level:

- the first case is a plain bug,

- and the second case *happens* not to be a bug, but only because two's
complement saves the day (when the initial bit-neg results in (-2)). And
most people don't even realize this; they don't understand that they in
fact do "flags &= (-2)", and rely on two's complement even for the (-2).

(Not to mention that most people assume that 0x1 has type "unsigned int"
-- that's not the case. The prefix 0x only *permits* the compiler to
consider unsigned types for determining the proper ("minimal") type for
the integer constant, it does not "require" the compiler to choose an
unsigned type. The "u" suffix does the forcing.)

Therefore I like to write

  flags &= ~(unsigned_type_of_flags)FLAG_MASK_WHATEVER;

because then the bit-neg will properly affect all bits of "flags",
regardless of the type of FLAG_MASK_WHATEVER.

Now, *IF* the C standard starts mandating two's complement, then (a)
defining FLAG_MASK_WHATEVER as a signed int (with positive value), such
as 0x1, *and* (b) writing just "flags &= ~FLAG_MASK_WHATEVER" will
become beneficial. That's because (a) the "happenstance" will be taken
out of (~FLAG_MASK_WHATEVER) resulting in the *proper* negative value
(which plays well with the modular reduction in the conversion to
unsigned integer types), and (b) it won't matter whether "flags" has
type unsigned int, unsigned long, or unsigned long long, even without an
explicit typecast, as the negative value will automatically be added to
UINT_MAX+1, ULONG_MAX+1, or ULLONG_MAX+1 as needed.

In other words, this form is going to force "sign extension" (which is
what we ultimately want here), but with proper C language foundation.
People will still be doing "flags &= (-2)" and not know it, but at least
they won't have to know it anymore.

Until C17 becomes a project's baseline though, I'll likely stick to the form

  flags &= ~(unsigned_type_of_flags)FLAG_MASK_WHATEVER;

because that's the safest form.

Laszlo
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[prev in list] [next in list] [prev in thread] [next in thread]