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

[Simon Marchi]

On 2019-05-08 10:39 a.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.

I didn't spot any mistake by inspecting the code, but I get some errors running
tests/lib/test_bitfield, so I guess there are some :).  Does it pass on your side?

> ---
>  include/babeltrace/bitfield-internal.h | 77 ++++++++++++++++------------------
>  1 file changed, 37 insertions(+), 40 deletions(-)
> 
> diff --git a/include/babeltrace/bitfield-internal.h b/include/babeltrace/bitfield-internal.h
> index c5d5eccd..7eb36989 100644
> --- a/include/babeltrace/bitfield-internal.h
> +++ b/include/babeltrace/bitfield-internal.h
> @@ -55,21 +55,26 @@
>  #define _bt_is_signed_type(type)	((type) -1 < (type) 0)
>  
>  /*
> - * NOTE: The cast to (uint64_t) below ensures that we're not casting a
> - * negative value, which is undefined in C. However, this limits the
> - * maximum type size of `type` and `v` to 64-bit. The
> - * _bt_check_max_64bit() is used to check that the users of this header
> - * do not use types with a size greater than 64-bit.
> + * Cast value `v` to an unsigned integer type of the size of type `type`.
> + *
> + * The unsigned cast ensures that we're not shifting a negative value,
> + * which is undefined in C. However, this limits the maximum type size
> + * of `type` to 64-bit. Generate a compile-time error if the size of
> + * `type` is larger than 64-bit.
>   */
>  #define _bt_unsigned_cast(type, v)					\
> -({									\
> -	(sizeof(v) < sizeof(type)) ?					\
> -		((type) (v)) & ((type) (~(~(uint64_t) 0 << (sizeof(v) * CHAR_BIT)))) : \
> -		(type) (v);						\
> -})
> +	(sizeof(type) == sizeof(uint8_t) ? (uint8_t) (v) :		\
> +	sizeof(type) == sizeof(uint16_t) ? (uint16_t) (v) :		\
> +	sizeof(type) == sizeof(uint32_t) ? (uint32_t) (v) :		\
> +	sizeof(type) == sizeof(uint64_t) ? (uint64_t) (v) :		\
> +	sizeof(struct { int f:(sizeof(type) > sizeof(uint64_t) ? -1 : 1); }))
>  
> -#define _bt_check_max_64bit(type)					\
> -	char _max_64bit_assertion[sizeof(type) <= sizeof(uint64_t) ? 1 : -1] __attribute__((unused))
> +/*
> + * _bt_fill_mask evaluates to an unsigned integer with the size of
> + * "type" with all bits set. It is meant to be used as a left operand to
> + * the shift-left operator to create bit masks.
> + */
> +#define _bt_fill_mask(type)		_bt_unsigned_cast(type, ~(type) 0)

Thanks for the comment, it makes what this macro does very clear.

>  
>  /*
>   * bt_bitfield_write - write integer to a bitfield in native endianness
> @@ -108,15 +113,15 @@ do {									\
>  									\
>  	/* Trim v high bits */						\
>  	if (__length < sizeof(__v) * CHAR_BIT)				\
> -		__v &= ~((~(typeof(__v)) 0) << __length);		\
> +		__v &= (typeof(__v)) ~(_bt_fill_mask(typeof(__v)) << __length); \

Not a blocker, but this operation is done enough times that it might be worth
giving it a name and make a macro for it.  It could help make this big macro
more readable, and we could test it on its own if needed.

/* Generate a mask of type `type` with the `length` least significant bits set. */

#define _bt_make_mask(type, length) \
	((type) ~(_bt_fill_mask(type) << length))

Simon