On Tue, May 04, 2021 at 01:14:22PM -0700, harshad shirwadkar wrote:

I see thanks for the explanation. I quickly tried it too and saw that
UBSAN warnings went away but I got compiler warnings
"recovery.c:413:27: warning: taking address of packed member
't_blocknr_high' of class or structure 'journal_block_tag_s' may
result in an unaligned pointer value [-Waddress-of-packed-member]".
These compiler warnings seem to be added in [1].

These warnings make me think that de-referencing a member of a packed
struct is still not safe. My concern is this - If we define
journal_block_tag_t as a packed struct AND if we have following unsafe
code, then we won't see UBSAN warnings and the following unaligned
accesses would go unnoticed. That may not go well on certain
architectures.

j_block_tag_t *unaligned_ptr;

flags = unaligned_ptr->t_flags;

It looks like if the compiler doesn't support
-Waddress-of-packed-member [1], we may not even see these warnings, we
won't see UBSAN warnings and the unaligned accesses may cause problems
on the architectures that you mentioned.

In other words, what I'm trying to say is that while
__atribute__((packed)) would silence UBSAN warnings (and we should do
it), it's still not sufficient to ensure that our code doesn't do
unaligned accesses to the struct in question. Does that make sense?

- Harshad

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.

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.)

- Eric