[lttng-dev] [PATCH babeltrace v3] Fix: bitfield: left shift undefined behavior

[Simon Marchi]

On 2019-05-08 6:23 p.m., Mathieu Desnoyers wrote:
> bitfield.h uses the left shift operator with a left operand which
> may be negative. The C99 standard states that shifting a negative
> value is undefined.
> 
> When building with -Wshift-negative-value, we get this gcc warning:
> 
> In file included from /home/smarchi/src/babeltrace/include/babeltrace/ctfser-internal.h:44:0,
>                  from /home/smarchi/src/babeltrace/ctfser/ctfser.c:42:
> /home/smarchi/src/babeltrace/include/babeltrace/ctfser-internal.h: In function ‘bt_ctfser_write_unsigned_int’:
> /home/smarchi/src/babeltrace/include/babeltrace/bitfield-internal.h:116:24: error: left shift of negative value [-Werror=shift-negative-value]
>    mask = ~((~(type) 0) << (__start % ts));  \
>                         ^
> /home/smarchi/src/babeltrace/include/babeltrace/bitfield-internal.h:222:2: note: in expansion of macro ‘_bt_bitfield_write_le’
>   _bt_bitfield_write_le(ptr, type, _start, _length, _v)
>   ^~~~~~~~~~~~~~~~~~~~~
> /home/smarchi/src/babeltrace/include/babeltrace/ctfser-internal.h:418:3: note: in expansion of macro ‘bt_bitfield_write_le’
>    bt_bitfield_write_le(mmap_align_addr(ctfser->base_mma) +
>    ^~~~~~~~~~~~~~~~~~~~
> 
> This boils down to the fact that the expression ~((uint8_t)0) has type
> "signed int", which is used as an operand of the left shift.  This is due
> to the integer promotion rules of C99 (6.3.3.1):
> 
>     If an int can represent all values of the original type, the value is
>     converted to an int; otherwise, it is converted to an unsigned int.
>     These are called the integer promotions. All other types are unchanged
>     by the integer promotions.
> 
> We also need to cast the result explicitly into the left hand
> side type to deal with:
> 
> warning: large integer implicitly truncated to unsigned type [-Woverflow]
> 
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
> ---
> Changes since v1:
> - Generate compile-time error if the type argument passed to
>   _bt_unsigned_cast() is larger than sizeof(uint64_t), this
>   allows removing _bt_check_max_64bit,
> - Introduce _br_fill_mask, which replaces _bt_bitwise_not,
> - Clarify _bt_unsigned_cast comment,
> - Expand explanation of the issue within the patch commit message.
> 
> Changes since v2:
> - Fix unwanted sign extension when generating masks,
> - Introduce macro helpers to clarify code:
>   - _bt_cast_value_to_unsigned()
>   - _bt_cast_value_to_unsigned_type(),
>   - _bt_make_mask_complement(),
>   - _bt_make_mask().

I might have more cases for you to investigate :)

Can you try building with -fsanitize=undefined and run test_bitfield? I get some:

/home/smarchi/src/babeltrace/tests/lib/test_bitfield.c:222:4: runtime error: left shift of negative value -1

Which would hint that we are missing a cast before a shift.

Simon