[PATCH 2/2] qemu_process: Fix theoretical overflow in uint to bool typecast

[Michal Prívozník]

On 2/9/22 10:08, Peter Krempa wrote:
> On Wed, Feb 09, 2022 at 09:39:50 +0100, Michal Privoznik wrote:
>> The qemuPrepareNVRAM() function accepts three arguments and the
>> last one being a boolean type. However, when the function is
>> called from qemuProcessPrepareHost() the argument passed is a
>> result of logical and of @flags (unsigned int) and
>> VIR_QEMU_PROCESS_START_RESET_NVRAM value. In theory this is
>> unsafe to do because if the value of the flag is ever changed
> 
> I don't think that this is accurate as casting a unsigned int (32bits)
> to a bool (8 bits) works properly even when the lower 8 bits of the
> value equal to 0:
> 
> $ cat bool.c
> #include <stdint.h>
> #include <stdio.h>
> #include <limits.h>
> #include <stdbool.h>
> 
> void p(char *s, unsigned long long v, uint8_t u, bool b) {
>     printf("%30s: %30llu: %4u: %d\n", s, v, u, b);
> }
> 
> #define E(val) \
>     p(#val, (val), (val), (val))
> 
> #define T(type) \
>     E(type - 1);\
>     E(type);\
>     E(type + 1)
> 
> int main() {
>     printf("bool size: %u\n", sizeof(bool));
>     E(0);
>     T(CHAR_MAX);
>     T(UCHAR_MAX);
>     T(UINT8_MAX);
>     T(INT_MAX);
>     T(UINT_MAX);
> }
> 
> 
> $ ./a.out
> bool size: 1
>                              0:                              0:    0: 0
>                       0x7f - 1:                            126:  126: 1
>                           0x7f:                            127:  127: 1
>                       0x7f + 1:                            128:  128: 1
>             (0x7f * 2 + 1) - 1:                            254:  254: 1
>                 (0x7f * 2 + 1):                            255:  255: 1
>             (0x7f * 2 + 1) + 1:                            256:    0: 1
>                      (255) - 1:                            254:  254: 1
>                          (255):                            255:  255: 1
>                      (255) + 1:                            256:    0: 1
>                 0x7fffffff - 1:                     2147483646:  254: 1
>                     0x7fffffff:                     2147483647:  255: 1
>                 0x7fffffff + 1:           18446744071562067968:    0: 1
>     (0x7fffffff * 2U + 1U) - 1:                     4294967294:  254: 1
>         (0x7fffffff * 2U + 1U):                     4294967295:  255: 1
>     (0x7fffffff * 2U + 1U) + 1:                              0:    0: 0
> 

Ah, looks like GCC is clever enough. For the following code:

void pb(bool b) {
    printf("%u\n", b);
}

int main(int argc, char *argv[])
{
    unsigned int x = -1;
    pb(x & (1 << (sizeof(bool) * 8)));
    return 0;
}

GCC generates some extra code when calling the pb() function:

    1172:       c7 45 fc ff ff ff ff    movl   $0xffffffff,-0x4(%rbp)
    1179:       8b 45 fc                mov    -0x4(%rbp),%eax
    117c:       25 00 01 00 00          and    $0x100,%eax
    1181:       85 c0                   test   %eax,%eax
    1183:       0f 95 c0                setne  %al
    1186:       0f b6 c0                movzbl %al,%eax
    1189:       89 c7                   mov    %eax,%edi
    118b:       e8 a9 ff ff ff          call   1139 <pb>


IOW, the compiler does "sign extension", well "reduction". Clang
produces similar assembler, except it makes doubly sure that bool has
just the lowest bit set:

  401166:       c7 45 ec ff ff ff ff    movl   $0xffffffff,-0x14(%rbp)
  40116d:       8b 45 ec                mov    -0x14(%rbp),%eax
  401170:       25 00 01 00 00          and    $0x100,%eax
  401175:       83 f8 00                cmp    $0x0,%eax
  401178:       0f 95 c0                setne  %al
  40117b:       0f b6 f8                movzbl %al,%edi
  40117e:       83 e7 01                and    $0x1,%edi
  401181:       e8 9a ff ff ff          call   401120 <pb>


So, since compilers are now clever enough, I guess we can stop writing
the expression like I did.

Michal