Warn when taking address of packed member
AcceptedPublic

Authored by rogfer01 on May 24 2016, 12:47 AM.

Details

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.

Diff Detail

Repository
rL LLVM
There are a very large number of changes, so older changes are hidden. Show Older Changes
aaron.ballman requested changes to this revision.Jun 1 2016, 12:06 PM

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.

The fix-it can help silencing the warning for those cases once deemed safe. This is the reason why I keep comparing the new warning to the assignment-vs-comparison warning: there may not be an error, just extra syntax (parentheses) can be used to silence the warning.

Assignment vs comparison is not a valuable comparison because that is a case relying on programmer intent (could be assignment, could be comparison, but there's no way to tell what's more likely, so use parens to disambiguate). In this case, there is code that could be dangerous (more likely) or benign (less likely), but adding parens does not disambiguate between two choices, it simply disables the diagnostic while leaving the semantics the same.

Fix-its are meant to be used when we can reasonably fix the user's code. Adding parens does *not* fix the user's code for the true positive case and is actually a dangerous code transformation (because it hides a true positive). I do not think this check should have a fix-it simply to silence false positives. If the false positive rate is that high, this should become a clang-tidy check instead (and I would still be opposed to the fix-it hint there as well).

include/clang/Basic/DiagnosticSemaKinds.td
5392 ↗(On Diff #59241)

I don't think this note adds value. Placing parens around the expression does silence the warning for false-positives, but that seems more like a documentation issue than a diagnostic.

lib/Sema/SemaExpr.cpp
10537–10538 ↗(On Diff #59241)

I do not think this diagnostic should have a FixIt hint. This isn't actually *fixing* the problem, it is simply a way to silence the diagnostic while retaining the same behavior.

test/SemaCXX/address-packed.cpp
92–93 ↗(On Diff #59241)

Why this->X in the diagnostic when that's not what the user wrote?

This revision now requires changes to proceed.Jun 1 2016, 12:06 PM
rogfer01 marked 3 inline comments as done.Jun 2 2016, 12:11 AM
rogfer01 added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
5392 ↗(On Diff #59241)

Ditto.

lib/Sema/SemaExpr.cpp
10537–10538 ↗(On Diff #59241)

OK. I will remove it.

test/SemaCXX/address-packed.cpp
92–93 ↗(On Diff #59241)

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 updated this revision to Diff 59342.Jun 2 2016, 12:18 AM
rogfer01 marked 3 inline comments as done.

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

aaron.ballman accepted this revision.Jun 2 2016, 6:19 AM

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

test/SemaCXX/address-packed.cpp
92 ↗(On Diff #59342)

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

This revision is now accepted and ready to land.Jun 2 2016, 6:19 AM
rogfer01 marked an inline comment as done.Jun 2 2016, 6:24 AM

Ping?

Thanks a lot.

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!

Thank you Aaron. I will commit ASAP.

This revision was automatically updated to reflect the committed changes.

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 reopened this revision.Jun 14 2016, 4:28 AM

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.

This revision is now accepted and ready to land.Jun 14 2016, 4:28 AM
rogfer01 updated this revision to Diff 60664.Jun 14 2016, 4:34 AM
rogfer01 removed rL LLVM as the repository for this revision.

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.

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

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 updated this revision to Diff 60951.Jun 16 2016, 2:09 AM
rogfer01 updated this object.

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 added a comment.EditedJun 16 2016, 4:56 AM

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?

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

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

lib/Sema/SemaCast.cpp
259–260 ↗(On Diff #60951)

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 marked an inline comment as done.Jun 17 2016, 6:21 AM
rogfer01 updated this revision to Diff 61089.Jun 17 2016, 6:23 AM

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

Ping?

Thank you very much.

Ping? Any further comments, thoughts?

Thank you very much.

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.

Ping?

Any comment on this?

Thank you very much.

Ping?

Looking forward any further comments before committing.

Thank you very much.

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

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.

This revision was automatically updated to reflect the committed changes.

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?

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.

trybka added a subscriber: trybka.Jul 14 2016, 11:26 AM
rogfer01 reopened this revision.Jul 15 2016, 12:19 AM

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

This revision is now accepted and ready to land.Jul 15 2016, 12:19 AM

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.

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

rogfer01 updated this revision to Diff 64114.Jul 15 2016, 2:52 AM
rogfer01 updated this object.
rogfer01 removed rL LLVM as the repository for this revision.

Remove the diagnostic for the binding of references.

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.

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.

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.

Hi, friendly ping.

@rsmith any further comment on this?

Thank you very much.

This revision was automatically updated to reflect the committed changes.
MatzeB added a subscriber: MatzeB.Aug 15 2016, 3:20 PM

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 reopened this revision.Feb 1 2017, 2:40 AM
royger added a subscriber: 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.

This revision is now accepted and ready to land.Feb 1 2017, 2:40 AM

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 added a comment.Feb 1 2017, 3:27 AM

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.

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 added a subscriber: kimgr.Feb 1 2017, 3:59 AM

... 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 added a comment.Feb 1 2017, 4:15 AM

... 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 added a subscriber: joerg.Feb 1 2017, 5:14 AM

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.

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 added a comment.Feb 1 2017, 5:29 AM

@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 added a comment.Feb 1 2017, 1:30 PM

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 added a comment.EditedFeb 2 2017, 8:18 AM

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 added a comment.Feb 2 2017, 11:07 AM

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.

asl added a subscriber: asl.Mar 24 2017, 2:02 AM