On Tue, May 04, 2021 at 01:45:02PM -0700, Eric Biggers wrote: > > No, 'flags = unaligned_ptr->t_flags' is fine, provided that unaligned_ptr is a > pointer to a struct with the packed attribute. What -Waddress-of-packed-member > will warn about is if you do something like &unaligned_ptr->t_flags to get a > pointer directly to the t_flags field, as such pointers can then be incorrectly > used for misaligned accesses. Yeah, the warnings appear if you apply my patch, and then *also* add the __attribute__((__packed__)) annotation. If you revert my patch, and then only add the __packed__ annotation, there are no warnings, because we're no longer taking the address of the field with a packed attribute. Basically, what gcc (and presumably clang) is doing is it is special casing packed_ptr->field so that the compiled code will work regardless of the alignment of packed_ptr. This isn't documented anywhere, but it apparently is the case. (I had assumed that it would only generate unaligned access for those fields that are not aligned if the structure started on an aligned boundary.) However, if you take an address of a packed memory, short *p = &packed_ptr->field; .. and then later derference that pointer, the C compiler can't know that it needs to generate the magic unaligned derferencing code when it dereferences that pointer. So that's why that warning is there. But if you just add __packed__ attribute, without my proposed patch, we aren't taking the &packed_ptr->field anywhere in e2fsprogs, so we're fine. > If we really don't want to use __attribute__((packed)) that is fine, but then > we'll need to remember to use an unaligned accessor *every* field access (except > for bytes), which seems harder to me -- and the compiler won't warn when one of > these is missing. (They can only be detected at runtime using UBSAN.) One reason not to use the __packed__ attribute is that there are cases where people attempt to build e2fsprogs on non-gcc/non-clang binaries. At one point FreeBSD was trying to use pcc to build e2fsprogs IIRC. And certainly there are people who try to build e2fsprogs on MSVC on Windows. So maybe the memcpy to a local copy is the better way to go, and hopefully the C compiler will optimize away the local copy on architectures where it is safe to do so. And in the unlikely case that it is a performance bottleneck, we could add a -DUBSAN when configure --enable-ubsan is in force, which switches in the memcpy when only when ubsan is enabled. - Ted
