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
