From: Alejandro Colomar
> Sent: Thursday, August 17, 2023 1:23 AM
>
> Hi Kees, Gustavo,
>
> I've been discussing with a friend about the appropriateness of sizeof()
> vs offsetof() for calculating the size of a structure with a flexible
> array member (FAM).
>
> After reading Jens Gustedt's blog post about it[1], we tried some tests,
> and we got some interesting results that discouraged me from using sizeof().
> See below.
>
> But then, said friend pointed to me that the kernel uses sizeof() in
> struct_size(), and we wondered why you would have chosen it. It's safe
> as long as you _know_ that there's no padding, or that the alignment of
> the FAM is as large as the padding (which you probably know in the kernel),
> but it seems safer to use
>
> MAX(sizeof(s), offsetof(s, fam) + sizeof_member(s, fam) * count)
>
> The thing is, if there's any trailing padding in the struct, the FAM may
> overlap the padding, and the calculation with sizeof() will waste a few
> bytes, and if misused to get the location of the FAM, the problem will be
> bigger, as you'll get a wrong location.
>
> So, I just wanted to pry what and especially why the kernel chose to prefer
> a simple sizeof().
>
> Cheers,
> Alex
>
> ---
.....
> strcpy(s->fam, "Hello, sizeof!");
> p = (char *) s + sizeof(struct s);
> puts(p);
Why on earth would you expect the above to do anything sensible?
It is a shame you can just use offsetof(type, member[count + 1]).
That is fine for constants, but the C language requires offsetof()
to be a compile-time constant - I can't help feeling the standards
body didn't consider non-constant array offsets.
(The compiler for a well known OS won't compile that (or anything
that looks like it) even for a constant array subscript!)
The actual problem with using offsetof() is that you might end
up with something smaller than the structure size.
(When the variable sized array is smaller than the padding.)
While max() will generate a constant for constant input, it
will be a real compare for non-constant input.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Hi David,
On 2023-08-21 10:16, David Laight wrote:
> From: Alejandro Colomar
>> Sent: Thursday, August 17, 2023 1:23 AM
>>
>> strcpy(s->fam, "Hello, sizeof!");
>> p = (char *) s + sizeof(struct s);
>> puts(p);
>
> Why on earth would you expect the above to do anything sensible?
This trivial example may seem unreasonable, but I've seen code that
does something like that (but more complex). Not in the kernel, but
in an nginx subproject:
<https://github.com/nginx/unit/blob/47ff51009fa05d83bb67cd5db16829ab4c0081d7/src/wasm/nxt_wasm.c#L108>
<https://github.com/nginx/unit/blob/47ff51009fa05d83bb67cd5db16829ab4c0081d7/src/wasm/nxt_wasm.c#L160>
It uses pointer arithmetic with sizeof to get the offset of the FAM,
instead of calling it by its name.
>
> It is a shame you can just use offsetof(type, member[count + 1]).
> That is fine for constants, but the C language requires offsetof()
> to be a compile-time constant - I can't help feeling the standards
> body didn't consider non-constant array offsets.
This helped catch a bug last week, so I think it's good that the
standard disallows it.
You can always write a macro offsetof_fam(type, fam, n) which does
that. In fact, I've written it, and will be part of the patch that
I'll propose. It is much safer than if offsetof() would just
accept that, as I can embed some static assertions within that
macro.
Here's a look at the file I've been testing before submitting a patch.
A lot of what you'll see here is similar to what I pretend to send in
a patch.
$ cat alx_cdefs.h
/* Copyright (C) 2023 Alejandro Colomar <[email protected]> */
/* SPDX-License-Identifier: GPL-3.0-or-later */
#ifndef ALX_CDEFS_H_INCLUDED_
#define ALX_CDEFS_H_INCLUDED_
#include <stddef.h>
#include <sys/param.h>
#define sizeof_array(a) (sizeof(a) + must_be_array(a))
#define nitems(a) (sizeof_array(a) / sizeof((a)[0]))
#define memberof(T, member) ((T){}.member)
#define sizeof_incomplete(x) \
( \
sizeof( \
struct { \
max_align_t a; \
typeof(x) inc; \
} \
) \
- sizeof(max_align_t) \
)
#define sizeof_fam0(T, fam) (sizeof(memberof(T, fam[0])) + must_be_fam(T, fam))
#define sizeof_fam(T, fam, n) (sizeof_fam0(T, fam) * (n))
#define offsetof_fam(T, fam, n) (offsetof(T, fam) + sizeof_fam(T, fam, n))
#define sizeof_struct(T, fam, n) MAX(sizeof(T), offsetof_fam(T, fam, n))
#define is_zero_sizeof(z) (sizeof_incomplete(z) == 0)
#define is_same_type(a, b) __builtin_types_compatible_p(a, b)
#define is_same_typeof(a, b) is_same_type(typeof(a), typeof(b))
#define is_array(a) (!is_same_typeof(a, &(a)[0]))
#define must_be(e) \
( \
0 * (int) sizeof( \
struct { \
_Static_assert(e, ""); \
int ISO_C_forbids_a_struct_with_no_members_; \
} \
) \
)
#define must_be_array(a) must_be(is_array(a))
#define must_be_zero_sizeof(z) must_be(is_zero_sizeof(z))
#define must_be_fam(T, fam) \
(must_be_array(memberof(T, fam)) + must_be_zero_sizeof(memberof(T, fam)))
#endif /* ALX_CDEFS_H_INCLUDED_ */
> (The compiler for a well known OS won't compile that (or anything
> that looks like it) even for a constant array subscript!)
>
> The actual problem with using offsetof() is that you might end
> up with something smaller than the structure size.
> (When the variable sized array is smaller than the padding.)
That's why MAX().
>
> While max() will generate a constant for constant input, it
> will be a real compare for non-constant input.
If the input was non-constant, then it would already have been
non-constant with the current code. I don't think MAX() will
make it worse.
>
> David
Cheers,
Alex
--
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5