Warn when taking address of packed member

[rogfer01]

Summary

This patch implements PR#22821.

Taking the address of a packed member is dangerous since the reduced
alignment of the pointee is lost. This can lead to memory alignment
faults in some architectures if the pointer value is dereferenced.

This change adds a new warning to clang emitted when taking the address
of a packed member. A packed member is either a field/data member
declared as attribute((packed)) or belonging to a struct/class
declared as such. The associated flag is -Waddress-of-packed-member.
Conversions (either implicit or via a valid casting) to pointer types
with lower or equal alignment requirements (e.g. void* or char*)
silence the warning.

[rogfer01]

include/clang/Basic/DiagnosticSemaKinds.td

5394

Ditto.

lib/Sema/SemaExpr.cpp

10540–10541

OK. I will remove it.

test/SemaCXX/address-packed.cpp

93–94

Probably clang introduces it internally when parsing an id-expression that happens to be a nonstatic data member because the diagnostic does include it.

That said this is going away as I'm removing the fix-it.

[rogfer01]

Remove fix-it as it is not suitable for this diagnostic.

[aaron.ballman]

I think the patch LGTM (with a minor formatting nit). @rsmith, what are your thoughts?

test/SemaCXX/address-packed.cpp

93

Formatting (may want to clang-format one last time before committing).

[rogfer01]

Ping?

Thanks a lot.

[aaron.ballman]

I think Richard has had enough time to comment. You can go ahead and commit, and if there are any issues, they can be handled post-commit. Thanks!

[rogfer01]

Thank you Aaron. I will commit ASAP.

[eugenis]

In D20561#446031, @aaron.ballman wrote:

In D20561#445824, @rogfer01 wrote:

I think I wasn't clear with the purpose of the fix-it: there are a few cases where getting the address of an unaligned pointer is safe (i.e. false positives).

For instance, when I checked Firefox and Chromium there are cases where getting the address of an unaligned pointer is fine. For the particular case of these two browsers, they both use a library (usrsctp) that represents protocol data as packed structs. That library passes addresses of packed fields to memcpy and memmove which is OK.

I think this is a false-positive that should be fixed.

This patch was committed without fixing the false positive case, why?

Could this warning be excluded from -Wall?

[rogfer01]

I've reverted the commit.

During implementation of the white list I felt the changes were too much for a follow up commit, so I prefer some feedback first.

[rogfer01]

This new patch adds a whitelist for memcpy, memmove, memset and memcmp.

I'm not entirely happy with the strategy that I'm using, where Parser communicates to Sema which is the LHS of a direct function call being parsed. Better approaches are of course welcome.

[rogfer01]

A build of Firefox does not show any false positive now.

[rsmith]

This still looks like it will have has lots of false positives for cases like:

struct __attribute__((packed)) A {
  char c;
  int n;
} a;
void *p = &a.n;

It also looks like it will now have false negatives for cases like:

memcpy(x, y, *&a.n);

I think whitelisting specific functions is not a reasonable approach here; instead, how about deferring the check until you see how the misaligned pointer is used? A couple of approaches seem feasible:

• you could extend the conversion-checking code in SemaChecking to look for such misaligned operations that are not immediately converted to a pointer type with suitable (lower) alignment requirements
• you could build a list in Sema of the cases that are pending a diagnostic, produce diagnostics at the end of the full-expression, and remove items from the list when you see a suitable conversion applied to them

In any case, this diagnostic should apply to reference binding as well as pointers.

GCC appears to check this as a separate step, at least after it applies its fold; for example:

struct __attribute__((packed, aligned(4))) S { char c[4]; int n; } s;
int k;
int &r = true ? s.n : k; // gcc rejects, "cannot bind paced field 's.S::n' to 'int&'
int &s = false ? s.n : k; // gcc accepts

[rogfer01]

Thanks @rsmith for the suggestions. I removed the whitelisting totally and changed the strategy. This patch implements your second suggestion which seemed more obvious to me. Since I expect the set of gathered misaligned member designations be small, I am using a plain SmallVector but maybe there is a more suitable structure.

I am now building Firefox and will post an update with the results.

[rogfer01]

Firefox build shows a couple of warnings in the sctp library that already gave warnings with the earlier patches.

That code has the following structure.

#define SCTP_PACKED __attribute__((packed))
#define SCTP_IDENTIFICATION_SIZE 16
// ...
/* state cookie header */
struct sctp_state_cookie {	/* this is our definition... */
	uint8_t identification[SCTP_IDENTIFICATION_SIZE];/* id of who we are */
	struct timeval time_entered;	/* the time I built cookie */
        // other fields
} SCTP_PACKED;

The warning is triggered by the following code (the other occurence is almost exact).

net->RTO = sctp_calculate_rto(stcb, asoc, net,
				      &cookie->time_entered,   // ← warning here!
				      sctp_align_unsafe_makecopy,
				      SCTP_RTT_FROM_NON_DATA);

the called function being declared as

uint32_t
sctp_calculate_rto(struct sctp_tcb *stcb,
		   struct sctp_association *asoc,
		   struct sctp_nets *net,
		   struct timeval *told,
		   int safe, int rtt_from_sack)
{
  ...

So I think that is a legitimate warning.

But the code is working (assuming this function is ever called that I didn't check). Checking the code, though, reveals a curious usage of told.

	/* Copy it out for sparc64 */
	if (safe == sctp_align_unsafe_makecopy) {
		old = &then;
		memcpy(&then, told, sizeof(struct timeval));
	} else if (safe == sctp_align_safe_nocopy) {
		old = told;
	} else {
		/* error */
		SCTP_PRINTF("Huh, bad rto calc call\n");
		return (0);
	}

which suggests that the code somehow knows that the pointer is unaligned and cannot be copied straightforwardly.

In order to silence the warning we can cast to void* and back to the original pointer type.

net->RTO = sctp_calculate_rto(stcb, asoc, net,
				      (struct timeval*)(void*)&cookie->time_entered, // note: the cast is because this pointer is unaligned
				      sctp_align_unsafe_makecopy,
				      SCTP_RTT_FROM_NON_DATA);

Which looks a bit ugly to me but clearly pinpoints a problem and can be wrapped in a macro.

#define UNALIGNED_ADDRESS(x) ((__typeof__(x))(void*)(x))

...

net->RTO = sctp_calculate_rto(stcb, asoc, net,
				      UNALIGNED_ADDRESS(&cookie->time_entered),
				      sctp_align_unsafe_makecopy,
				      SCTP_RTT_FROM_NON_DATA);

What do you think?

[eugenis]

This timeval thing looks like a legitimate warning to me.
I don't think the analysis should go beyond the function boundaries. If a callee expects timeval * as part of its signature it should get a properly aligned timeval *.

[rsmith]

I agree, that looks like correct behavior for the warning. Thanks!

lib/Sema/SemaCast.cpp

259–260

More generally, I think we should discard it if the destination of the cast is a suitably-aligned pointer type. (For instance, casting to char* should be OK, and casting an attribute((packed,aligned(2))) to a 2-byte-aligned pointer type should be OK.)

[rogfer01]

Following @rsmith suggestion, the diagnostic is silenced if the address is converted to a pointer with lower or equal alignment requirements.

[rogfer01]

Ping?

Thank you very much.

[rogfer01]

Ping? Any further comments, thoughts?

Thank you very much.

[aaron.ballman]

In D20561#466545, @rogfer01 wrote:

Ping? Any further comments, thoughts?

Thank you very much.

The usual practice is to ping after a week has gone by, btw. There were standards meetings this week, so that may be delaying some of the comments on this thread.

[rogfer01]

Ping?

Any comment on this?

Thank you very much.

[rogfer01]

Ping?

Looking forward any further comments before committing.

Thank you very much.

[rogfer01]

If there are not any objections I will commit this tomorrow.

[aaron.ballman]

In D20561#482963, @rogfer01 wrote:

If there are not any objections I will commit this tomorrow.

If @rsmith doesn't comment by tomorrow, then I think any feedback can be handled post-commit instead.

[jyknight]

This seems to trigger even for the implicitly generated copier of a packed struct. E.g.

#include <sys/epoll.h>

void copyit(epoll_event&out, const epoll_event &in) {
  out = in;
}

Is that as intended?

[jyknight]

Regardless, I think this should not have added a new un-disableable error, but instead only a default-on warning.

The ""binding reference to packed member" error is firing on some of our code, and even if it's not a false-positive, it should be possible to disable it until the code is modified.

[rogfer01]

Reverted.

[rogfer01]

I suggest that we handle the reference binding diagnostic in another change and leave this one only for the address taking diagnostic.

[rogfer01]

In D20561#484421, @jyknight wrote:

This seems to trigger even for the implicitly generated copier of a packed struct. E.g.

#include <sys/epoll.h>

void copyit(epoll_event&out, const epoll_event &in) {
  out = in;
}

Is that as intended?

No, it wasn't. It seems to happen as well for the implicit copy constructor as well.

#include <sys/epoll.h>

void copyit2(epoll_event foo);
void copyit(epoll_event &out) { copyit2(out); }

clang++ -c test.cc
In file included from test.cc:1:
/usr/include/x86_64-linux-gnu/sys/epoll.h:87:8: error: binding reference to packed member 'data' of class or structure 'epoll_event'
struct epoll_event
       ^~~~~~~~~~~
test.cc:4:41: note: implicit copy constructor for 'epoll_event' first required here
void copyit(epoll_event &out) { copyit2(out); }
                                        ^
1 error generated.

[rogfer01]

I've opened https://llvm.org/bugs/show_bug.cgi?id=28571 to track the reference binding issue.

[rogfer01]

Remove the diagnostic for the binding of references.

[rogfer01]

I plan to commit this today. It only diagnoses address-taking of packed fields.

Reference binding to packed fields will be addressed in another change as proper support may involve codegen changes which are out of the scope of this work.

Any further comments are welcome, as usual.

[aaron.ballman]

In D20561#486994, @rogfer01 wrote:

I plan to commit this today. It only diagnoses address-taking of packed fields.

Please wait until someone has had the chance to review before committing (the fix does not appear trivial and the original code broke code). I'm on vacation today (in theory), but perhaps @rsmith will have a chance to review.

~Aaron

Reference binding to packed fields will be addressed in another change as proper support may involve codegen changes which are out of the scope of this work.

Any further comments are welcome, as usual.

[rogfer01]

Please wait until someone has had the chance to review before committing (the fix does not appear trivial and the original code broke code). I'm on vacation today (in theory), but perhaps @rsmith will have a chance to review.

Sure. Thanks.

[rogfer01]

Hi, friendly ping.

@rsmith any further comment on this?

Thank you very much.

[MatzeB]

The sanitizer code triggers this warning for code that looks conceptually like this:

typedef struct Bla { char bar; int foo; } __attribute__((packed));

uintptr_t getu(struct Bla *b) { return (uintptr_t)&b->foo; }

Resulting in:

taking address of packed member 'foo' of class or structure
      'Bla' may result in an unaligned pointer value
      [-Waddress-of-packed-member]

Of course the warning can be silenced with return (uintptr_t)(char*)&b->foo; still casting to an int type seems like a benign case for this warning so maybe we should exclude that?

[royger]

Hello,

This addition is silly, and too noisy for real world usage.

For once, it doesn't really check whether the access is actually unaligned or not, and then on x86 for example unaligned accesses are allowed (although with a performance penalty). Please either make this only complain about real unaligned accesses, or remove it. I don't think it's going to be used by any real-world projects, since it's too noise and gives false positives.

[rogfer01]

In D20561#662922, @royger wrote:

Hi,

some targets do not support unaligned accesses so this warning is valuable in terms of portability. In platforms where unaligned accesses are allowed, it may happen that such accesses incur in a performance penalty. So there is also value in terms of performance.

We fixed all identified false positives in later patches to this one. So maybe you want to check against trunk clang. If trunk still diagnoses false positives, please report them to me and I will be more than happy to fix them.

Kind regards.

[royger]

In D20561#662959, @rogfer01 wrote:

We fixed all identified false positives in later patches to this one. So maybe you want to check against trunk clang. If trunk still diagnoses false positives, please report them to me and I will be more than happy to fix them.

Can you tell me which revisions are those? It seems like they are not contained in clang 4.0 AFAICT.

[rogfer01]

In D20561#662963, @royger wrote:

In D20561#662959, @rogfer01 wrote:

We fixed all identified false positives in later patches to this one. So maybe you want to check against trunk clang. If trunk still diagnoses false positives, please report them to me and I will be more than happy to fix them.

Can you tell me which revisions are those? It seems like they are not contained in clang 4.0 AFAICT.

https://reviews.llvm.org/rL283304
https://reviews.llvm.org/rL286798

The fixes for the false positives are in the second change but the first is required for the second to apply cleanly.

Regards

[kimgr]

... and both revisions should be in the 4.0 branch (taken from r291814)

I was looking forward to this warning to help iron out alignment issues at compile-time instead of runtime (our ARM CPUs don't like unaligned access). @royger, can you expand a little on what you mean by

For once, it doesn't really check whether the access is actually unaligned or no

? Isn't that exactly what it does? Or do you mean in relation to whether the target allows unaligned access or not?

[royger]

In D20561#663006, @kimgr wrote:

... and both revisions should be in the 4.0 branch (taken from r291814)

I was looking forward to this warning to help iron out alignment issues at compile-time instead of runtime (our ARM CPUs don't like unaligned access). @royger, can you expand a little on what you mean by

For once, it doesn't really check whether the access is actually unaligned or no

? Isn't that exactly what it does? Or do you mean in relation to whether the target allows unaligned access or not?

Take the following example structure:

typedef union __attribute__((__packed__)) segment_attributes {
    uint16_t bytes;
    struct
    {
        uint16_t type:4;    /* 0;  Bit 40-43 */
        uint16_t s:   1;    /* 4;  Bit 44 */
        uint16_t dpl: 2;    /* 5;  Bit 45-46 */
        uint16_t p:   1;    /* 7;  Bit 47 */
        uint16_t avl: 1;    /* 8;  Bit 52 */
        uint16_t l:   1;    /* 9;  Bit 53 */
        uint16_t db:  1;    /* 10; Bit 54 */
        uint16_t g:   1;    /* 11; Bit 55 */
        uint16_t pad: 4;
    } fields;
} segment_attributes_t;


struct __attribute__((__packed__)) segment_register {
    uint16_t   sel;
    segment_attributes_t attr;
    uint32_t   limit;
    uint64_t   base;
};

clang is complaining about:

vmx.c:973:38: error: taking address of packed member 'base' of class or structure 'segment_register' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
        __vmread(GUEST_CS_BASE,     &reg->base);
                                     ^~~~~~~~~

But "base" is indeed aligned (it would be at the same offset without the packed attribute), so I'm not sure why it complains.

[joerg]

It is not true that all false positives have been fixed, some of the more complex cases involving nested data structures are still open.

@royger: Your example is missing explicit alignment. packed has two side effects: remove internal padding and set the alignment to 1. That means that the offset of base doesn't matter so much because reg itself is not necessarily aligned.

[rogfer01]

In D20561#663069, @joerg wrote:

It is not true that all false positives have been fixed, some of the more complex cases involving nested data structures are still open.

I could not find any in bugzilla after a quick search. So please feel free to point me to them (or CC) if you identify more false positives. I'm interested in fixing them.

Thanks!

[royger]

In D20561#663069, @joerg wrote:

@royger: Your example is missing explicit alignment. packed has two side effects: remove internal padding and set the alignment to 1. That means that the offset of base doesn't matter so much because reg itself is not necessarily aligned.

Does this means that it's "working as intended" then? I could expect this to complain when doing something like:

struct __attribute__((__packed__)) bar {
    uint64_t x1;
    uint16_t x2;
    uint64_t x3;
};

&bar->x3;

But not in the previous case.

[joerg]

The last review left out the case of naturally aligned packed members, IIRC. I have to go over the warning list in NetBSD again, but I'm moderately sure it is not fixed.

The better example is:

struct __attribute__((__packed__)) bar {
    uint16_t x1;
    uint16_t x2;
};

&bar->x2;

...which is not aligned.

[royger]

Ping?

It's not clear to me whether upstream is going to do something about this or not. I would like to know in case I need to start passing "-Wno-address-of-packed-member" around.

[joerg]

Yes, the project is interested on reducing the number of false positives. The example you gave is *not* a FP, but exactly the kind of situation the warning is supposed to trigger on.

[leonardchan]

I think I found a false negative with this where if the member is accessed from a packed struct type returned from a function, the warning does not appear:

typedef struct {
  uint8_t a;
  uint32_t b; 
} __attribute__((packed)) registers_t;

registers_t *Class::method() { return registers_; }

void Class::func() {
  other_func(&method()->b);  // I think the warning should appear here?
}