On Wed, Dec 17, 2025 at 10:21:09PM +0100, Martin Wilck wrote:
> The macro BITFIELD used a cast of a pointer to a different anonymous struct
> to (struct bitfield *). This violates strict aliasing rules, because the
> two structs aren't compatible types in the sense of the language
> specification. With -O2, gcc 15.1 generates code that makes th pgcmp()
> function fail, because the compiler doesn't initialize bf->len aka
> __storage_for__bf.len to 64.
>
> The problem will show through error messages like this:
>
> multipathd[1974208]: is_bit_set_in_bitfield: bitfield overflow: 1 >= 0
>
> Fix this by using a union
>
> Fixes: 9a2f173 ("libmpathutil: change STATIC_BITFIELD to BITFIELD")
> Signed-off-by: Martin Wilck <[email protected]>
> ---
> libmpathutil/util.c | 8 ++++----
> libmpathutil/util.h | 41 +++++++++++++++++++++++----------------
> libmultipath/configure.c | 4 ++--
> libmultipath/pgpolicies.c | 2 +-
> tests/util.c | 10 +++++-----
> 5 files changed, 36 insertions(+), 29 deletions(-)
>
> diff --git a/libmpathutil/util.c b/libmpathutil/util.c
> index 37412c6..5e45750 100644
> --- a/libmpathutil/util.c
> +++ b/libmpathutil/util.c
> @@ -337,15 +337,15 @@ void cleanup_fclose(void *p)
> fclose(p);
> }
>
> -void cleanup_bitfield(struct bitfield **p)
> +void cleanup_bitfield(union bitfield **p)
> {
> free(*p);
> }
>
> -struct bitfield *alloc_bitfield(unsigned int maxbit)
> +union bitfield *alloc_bitfield(unsigned int maxbit)
> {
> unsigned int n;
> - struct bitfield *bf;
> + union bitfield *bf;
>
> if (maxbit == 0) {
> errno = EINVAL;
> @@ -353,7 +353,7 @@ struct bitfield *alloc_bitfield(unsigned int maxbit)
> }
>
> n = (maxbit - 1) / bits_per_slot + 1;
> - bf = calloc(1, sizeof(struct bitfield) + n * sizeof(bitfield_t));
> + bf = calloc(1, sizeof(union bitfield) + (n - 1) * sizeof(bitfield_t));
> if (bf)
> bf->len = maxbit;
> return bf;
> diff --git a/libmpathutil/util.h b/libmpathutil/util.h
> index 3799fd4..1ec6a46 100644
> --- a/libmpathutil/util.h
> +++ b/libmpathutil/util.h
> @@ -86,29 +86,36 @@ typedef unsigned int bitfield_t;
> #endif
> #define bits_per_slot (sizeof(bitfield_t) * CHAR_BIT)
>
> -struct bitfield {
> - unsigned int len;
> - bitfield_t bits[];
> +union bitfield {
> + struct {
> + unsigned int len;
> + bitfield_t bits[];
> + };
> + /* for initialization in the BITFIELD macro */
> + struct {
> + unsigned int __len;
> + bitfield_t __bits[1];
> + };
> };
>
> -#define BITFIELD(name, length) \
> - struct { \
> - unsigned int len; \
> - bitfield_t bits[((length) - 1) / bits_per_slot + 1]; \
> - } __storage_for__ ## name = { \
> - .len = (length), \
> - .bits = { 0, }, \
> +/*
> + * gcc 4.9.2 (Debian Jessie) raises an error if the initializer for
> + * .__len comes first. Thus put .__bits first.
> + */
> +#define BITFIELD(name, length)
> \
> + union bitfield __storage_for__ ## name = { \
> + .__bits = { 0 }, \
> + .__len = (length), \
> }; \
It looks like this only allocates one bitfield_t, regardless of length.
Am I missing something here?
-Ben
> - struct bitfield *name = (struct bitfield *)& __storage_for__ ## name
> + union bitfield *name = & __storage_for__ ## name
>
> -struct bitfield *alloc_bitfield(unsigned int maxbit);
> +union bitfield *alloc_bitfield(unsigned int maxbit);
>
> void log_bitfield_overflow__(const char *f, unsigned int bit, unsigned int
> len);
> #define log_bitfield_overflow(bit, len) \
> log_bitfield_overflow__(__func__, bit, len)
>
> -static inline bool is_bit_set_in_bitfield(unsigned int bit,
> - const struct bitfield *bf)
> +static inline bool is_bit_set_in_bitfield(unsigned int bit, const union
> bitfield *bf)
> {
> if (bit >= bf->len) {
> log_bitfield_overflow(bit, bf->len);
> @@ -118,7 +125,7 @@ static inline bool is_bit_set_in_bitfield(unsigned int
> bit,
> (1ULL << (bit % bits_per_slot)));
> }
>
> -static inline void set_bit_in_bitfield(unsigned int bit, struct bitfield *bf)
> +static inline void set_bit_in_bitfield(unsigned int bit, union bitfield *bf)
> {
> if (bit >= bf->len) {
> log_bitfield_overflow(bit, bf->len);
> @@ -127,7 +134,7 @@ static inline void set_bit_in_bitfield(unsigned int bit,
> struct bitfield *bf)
> bf->bits[bit / bits_per_slot] |= (1ULL << (bit % bits_per_slot));
> }
>
> -static inline void clear_bit_in_bitfield(unsigned int bit, struct bitfield
> *bf)
> +static inline void clear_bit_in_bitfield(unsigned int bit, union bitfield
> *bf)
> {
> if (bit >= bf->len) {
> log_bitfield_overflow(bit, bf->len);
> @@ -146,5 +153,5 @@ static inline void clear_bit_in_bitfield(unsigned int
> bit, struct bitfield *bf)
> void cleanup_charp(char **p);
> void cleanup_ucharp(unsigned char **p);
> void cleanup_udev_device(struct udev_device **udd);
> -void cleanup_bitfield(struct bitfield **p);
> +void cleanup_bitfield(union bitfield **p);
> #endif /* UTIL_H_INCLUDED */
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index a8e18a2..41b6a9f 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -440,7 +440,7 @@ static int pgcmp(struct multipath *mpp, struct multipath
> *cmpp)
> int i, j;
> struct pathgroup *pgp, *cpgp;
> BITFIELD(bf, bits_per_slot);
> - struct bitfield *bf__ __attribute__((cleanup(cleanup_bitfield))) =
> NULL;
> + union bitfield *bf__ __attribute__((cleanup(cleanup_bitfield))) = NULL;
>
> if (VECTOR_SIZE(mpp->pg) != VECTOR_SIZE(cmpp->pg))
> return 1;
> @@ -1049,7 +1049,7 @@ int coalesce_paths (struct vectors *vecs, vector mpvec,
> char *refwwid,
> vector newmp;
> struct config *conf = NULL;
> int allow_queueing;
> - struct bitfield *size_mismatch_seen;
> + union bitfield *size_mismatch_seen;
> struct multipath * cmpp;
>
> /* ignore refwwid if it's empty */
> diff --git a/libmultipath/pgpolicies.c b/libmultipath/pgpolicies.c
> index 2e1a543..57b7485 100644
> --- a/libmultipath/pgpolicies.c
> +++ b/libmultipath/pgpolicies.c
> @@ -201,7 +201,7 @@ int group_by_match(struct multipath * mp, vector paths,
> bool (*path_match_fn)(struct path *, struct path *))
> {
> int i, j;
> - struct bitfield *bitmap;
> + union bitfield *bitmap;
> struct path * pp;
> struct pathgroup * pgp;
> struct path * pp2;
> diff --git a/tests/util.c b/tests/util.c
> index da192ba..67d8148 100644
> --- a/tests/util.c
> +++ b/tests/util.c
> @@ -198,7 +198,7 @@ static uint64_t maybe_swap(uint64_t v)
>
> static void test_bitmask_1(void **state)
> {
> - struct bitfield *bf;
> + union bitfield *bf;
> uint64_t *arr;
> int i, j, k, m, b;
>
> @@ -240,7 +240,7 @@ static void test_bitmask_1(void **state)
>
> static void test_bitmask_2(void **state)
> {
> - struct bitfield *bf;
> + union bitfield *bf;
> uint64_t *arr;
> int i, j, k, m, b;
>
> @@ -307,7 +307,7 @@ static void test_bitmask_2(void **state)
> */
> static void test_bitmask_len_0(void **state)
> {
> - struct bitfield *bf;
> + union bitfield *bf;
>
> bf = alloc_bitfield(0);
> assert_null(bf);
> @@ -329,7 +329,7 @@ static unsigned int maybe_swap_idx(unsigned int i)
>
> static void _test_bitmask_small(unsigned int n)
> {
> - struct bitfield *bf;
> + union bitfield *bf;
> uint32_t *arr;
> unsigned int size = maybe_swap_idx((n - 1) / 32) + 1, i;
>
> @@ -378,7 +378,7 @@ static void _test_bitmask_small(unsigned int n)
>
> static void _test_bitmask_small_2(unsigned int n)
> {
> - struct bitfield *bf;
> + union bitfield *bf;
> uint32_t *arr;
> unsigned int size = maybe_swap_idx((n - 1) / 32) + 1, i;
>
> --
> 2.52.0
