Skip to content

Commit

Permalink
Diagnose taking address and reference binding of packed members
Browse files Browse the repository at this point in the history
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.

This change also adds a new error diagnostic when the user attempts to
bind a reference to a packed member, regardless of the alignment.

Differential Revision: https://reviews.llvm.org/D20561

llvm-svn: 275417
Roger Ferrer Ibanez committed Jul 14, 2016
1 parent 34be2a0 commit 585ea9d
Showing 10 changed files with 474 additions and 1 deletion.
5 changes: 5 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
Original file line number Diff line number Diff line change
@@ -5425,6 +5425,11 @@ def warn_pointer_indirection_from_incompatible_type : Warning<
"dereference of type %1 that was reinterpret_cast from type %0 has undefined "
"behavior">,
InGroup<UndefinedReinterpretCast>, DefaultIgnore;
def warn_taking_address_of_packed_member : Warning<
"taking address of packed member %0 of class or structure %q1 may result in an unaligned pointer value">,
InGroup<DiagGroup<"address-of-packed-member">>;
def err_binding_reference_to_packed_member : Error<
"binding reference to packed member %0 of class or structure %q1">;

def err_objc_object_assignment : Error<
"cannot assign to class object (%0 invalid)">;
49 changes: 49 additions & 0 deletions clang/include/clang/Sema/Sema.h
Original file line number Diff line number Diff line change
@@ -9518,6 +9518,10 @@ class Sema {
void CheckArgumentWithTypeTag(const ArgumentWithTypeTagAttr *Attr,
const Expr * const *ExprArgs);

/// \brief Check if we are taking the address of a packed field
/// as this may be a problem if the pointer value is dereferenced.
void CheckAddressOfPackedMember(Expr *rhs);

/// \brief The parser's current scope.
///
/// The parser maintains this state here.
@@ -9596,6 +9600,51 @@ class Sema {
// Emitting members of dllexported classes is delayed until the class
// (including field initializers) is fully parsed.
SmallVector<CXXRecordDecl*, 4> DelayedDllExportClasses;

private:
/// \brief Helper class that collects misaligned member designations and
/// their location info for delayed diagnostics.
struct MisalignedMember {
Expr *E;
RecordDecl *RD;
ValueDecl *MD;
CharUnits Alignment;

MisalignedMember() : E(), RD(), MD(), Alignment() {}
MisalignedMember(Expr *E, RecordDecl *RD, ValueDecl *MD,
CharUnits Alignment)
: E(E), RD(RD), MD(MD), Alignment(Alignment) {}
explicit MisalignedMember(Expr *E)
: MisalignedMember(E, nullptr, nullptr, CharUnits()) {}

bool operator==(const MisalignedMember &m) { return this->E == m.E; }
};
/// \brief Small set of gathered accesses to potentially misaligned members
/// due to the packed attribute.
SmallVector<MisalignedMember, 4> MisalignedMembers;

/// \brief Adds an expression to the set of gathered misaligned members.
void AddPotentialMisalignedMembers(Expr *E, RecordDecl *RD, ValueDecl *MD,
CharUnits Alignment);

public:
/// \brief Diagnoses the current set of gathered accesses. This typically
/// happens at full expression level. The set is cleared after emitting the
/// diagnostics.
void DiagnoseMisalignedMembers();

/// \brief This function checks if the expression is in the sef of potentially
/// misaligned members and it is converted to some pointer type T with lower
/// or equal alignment requirements. If so it removes it. This is used when
/// we do not want to diagnose such misaligned access (e.g. in conversions to void*).
void DiscardMisalignedMemberAddress(const Type *T, Expr *E);

/// \brief This function calls Action when it determines that E designates a
/// misaligned member due to the packed attribute. This is used to emit
/// local diagnostics like in reference binding.
void RefersToMemberWithReducedAlignment(
Expr *E,
std::function<void(Expr *, RecordDecl *, ValueDecl *, CharUnits)> Action);
};

/// \brief RAII object that enters a new expression evaluation context.
3 changes: 3 additions & 0 deletions clang/lib/Sema/SemaCast.cpp
Original file line number Diff line number Diff line change
@@ -256,6 +256,7 @@ Sema::BuildCXXNamedCast(SourceLocation OpLoc, tok::TokenKind Kind,
Op.CheckConstCast();
if (Op.SrcExpr.isInvalid())
return ExprError();
DiscardMisalignedMemberAddress(DestType.getTypePtr(), E);
}
return Op.complete(CXXConstCastExpr::Create(Context, Op.ResultType,
Op.ValueKind, Op.SrcExpr.get(), DestTInfo,
@@ -279,6 +280,7 @@ Sema::BuildCXXNamedCast(SourceLocation OpLoc, tok::TokenKind Kind,
Op.CheckReinterpretCast();
if (Op.SrcExpr.isInvalid())
return ExprError();
DiscardMisalignedMemberAddress(DestType.getTypePtr(), E);
}
return Op.complete(CXXReinterpretCastExpr::Create(Context, Op.ResultType,
Op.ValueKind, Op.Kind, Op.SrcExpr.get(),
@@ -291,6 +293,7 @@ Sema::BuildCXXNamedCast(SourceLocation OpLoc, tok::TokenKind Kind,
Op.CheckStaticCast();
if (Op.SrcExpr.isInvalid())
return ExprError();
DiscardMisalignedMemberAddress(DestType.getTypePtr(), E);
}

return Op.complete(CXXStaticCastExpr::Create(Context, Op.ResultType,
67 changes: 67 additions & 0 deletions clang/lib/Sema/SemaChecking.cpp
Original file line number Diff line number Diff line change
@@ -8302,6 +8302,8 @@ void CheckImplicitConversion(Sema &S, Expr *E, QualType T,

DiagnoseNullConversion(S, E, T, CC);

S.DiscardMisalignedMemberAddress(Target, E);

if (!Source->isIntegerType() || !Target->isIntegerType())
return;

@@ -9371,6 +9373,7 @@ void Sema::CheckCompletedExpr(Expr *E, SourceLocation CheckLoc,
CheckUnsequencedOperations(E);
if (!IsConstexpr && !E->isValueDependent())
CheckForIntOverflow(E);
DiagnoseMisalignedMembers();
}

void Sema::CheckBitFieldInitialization(SourceLocation InitLoc,
@@ -10916,3 +10919,67 @@ void Sema::CheckArgumentWithTypeTag(const ArgumentWithTypeTagAttr *Attr,
<< ArgumentExpr->getSourceRange()
<< TypeTagExpr->getSourceRange();
}

void Sema::AddPotentialMisalignedMembers(Expr *E, RecordDecl *RD, ValueDecl *MD,
CharUnits Alignment) {
MisalignedMembers.emplace_back(E, RD, MD, Alignment);
}

void Sema::DiagnoseMisalignedMembers() {
for (MisalignedMember &m : MisalignedMembers) {
Diag(m.E->getLocStart(), diag::warn_taking_address_of_packed_member)
<< m.MD << m.RD << m.E->getSourceRange();
}
MisalignedMembers.clear();
}

void Sema::DiscardMisalignedMemberAddress(const Type *T, Expr *E) {
if (!T->isPointerType())
return;
if (isa<UnaryOperator>(E) &&
cast<UnaryOperator>(E)->getOpcode() == UO_AddrOf) {
auto *Op = cast<UnaryOperator>(E)->getSubExpr()->IgnoreParens();
if (isa<MemberExpr>(Op)) {
auto MA = std::find(MisalignedMembers.begin(), MisalignedMembers.end(),
MisalignedMember(Op));
if (MA != MisalignedMembers.end() &&
Context.getTypeAlignInChars(T->getPointeeType()) <= MA->Alignment)
MisalignedMembers.erase(MA);
}
}
}

void Sema::RefersToMemberWithReducedAlignment(
Expr *E,
std::function<void(Expr *, RecordDecl *, ValueDecl *, CharUnits)> Action) {
const auto *ME = dyn_cast<MemberExpr>(E);
while (ME && isa<FieldDecl>(ME->getMemberDecl())) {
QualType BaseType = ME->getBase()->getType();
if (ME->isArrow())
BaseType = BaseType->getPointeeType();
RecordDecl *RD = BaseType->getAs<RecordType>()->getDecl();

ValueDecl *MD = ME->getMemberDecl();
bool ByteAligned = Context.getTypeAlignInChars(MD->getType()).isOne();
if (ByteAligned) // Attribute packed does not have any effect.
break;

if (!ByteAligned &&
(RD->hasAttr<PackedAttr>() || (MD->hasAttr<PackedAttr>()))) {
CharUnits Alignment = std::min(Context.getTypeAlignInChars(MD->getType()),
Context.getTypeAlignInChars(BaseType));
// Notify that this expression designates a member with reduced alignment
Action(E, RD, MD, Alignment);
break;
}
ME = dyn_cast<MemberExpr>(ME->getBase());
}
}

void Sema::CheckAddressOfPackedMember(Expr *rhs) {
using namespace std::placeholders;
RefersToMemberWithReducedAlignment(
rhs, std::bind(&Sema::AddPotentialMisalignedMembers, std::ref(*this), _1,
_2, _3, _4));
}

6 changes: 5 additions & 1 deletion clang/lib/Sema/SemaExpr.cpp
Original file line number Diff line number Diff line change
@@ -6001,7 +6001,9 @@ Sema::ActOnCastExpr(Scope *S, SourceLocation LParenLoc,
CheckTollFreeBridgeCast(castType, CastExpr);

CheckObjCBridgeRelatedCast(castType, CastExpr);


DiscardMisalignedMemberAddress(castType.getTypePtr(), CastExpr);

return BuildCStyleCastExpr(LParenLoc, castTInfo, RParenLoc, CastExpr);
}

@@ -10534,6 +10536,8 @@ QualType Sema::CheckAddressOfOperand(ExprResult &OrigOp, SourceLocation OpLoc) {
if (op->getType()->isObjCObjectType())
return Context.getObjCObjectPointerType(op->getType());

CheckAddressOfPackedMember(op);

return Context.getPointerType(op->getType());
}

13 changes: 13 additions & 0 deletions clang/lib/Sema/SemaInit.cpp
Original file line number Diff line number Diff line change
@@ -6457,6 +6457,15 @@ InitializationSequence::Perform(Sema &S,
ExtendingEntity->getDecl());

CheckForNullPointerDereference(S, CurInit.get());

S.RefersToMemberWithReducedAlignment(CurInit.get(), [&](Expr *E,
RecordDecl *RD,
ValueDecl *MD,
CharUnits) {
S.Diag(Kind.getLocation(), diag::err_binding_reference_to_packed_member)
<< MD << RD << E->getSourceRange();
});

break;

case SK_BindReferenceToTemporary: {
@@ -6645,12 +6654,16 @@ InitializationSequence::Perform(Sema &S,
getAssignmentAction(Entity), CCK);
if (CurInitExprRes.isInvalid())
return ExprError();

S.DiscardMisalignedMemberAddress(Step->Type.getTypePtr(), CurInit.get());

CurInit = CurInitExprRes;

if (Step->Kind == SK_ConversionSequenceNoNarrowing &&
S.getLangOpts().CPlusPlus && !CurInit.get()->isValueDependent())
DiagnoseNarrowingInInitList(S, *Step->ICS, SourceType, Entity.getType(),
CurInit.get());

break;
}

26 changes: 26 additions & 0 deletions clang/test/Sema/address-packed-member-memops.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// RUN: %clang_cc1 -fsyntax-only -verify %s
// expected-no-diagnostics

struct B {
int x, y, z, w;
} b;

struct __attribute__((packed)) A {
struct B b;
} a;

typedef __typeof__(sizeof(int)) size_t;

void *memcpy(void *dest, const void *src, size_t n);
int memcmp(const void *s1, const void *s2, size_t n);
void *memmove(void *dest, const void *src, size_t n);
void *memset(void *s, int c, size_t n);

int x;

void foo(void) {
memcpy(&a.b, &b, sizeof(b));
memmove(&a.b, &b, sizeof(b));
memset(&a.b, 0, sizeof(b));
x = memcmp(&a.b, &b, sizeof(b));
}
160 changes: 160 additions & 0 deletions clang/test/Sema/address-packed.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
// RUN: %clang_cc1 -fsyntax-only -verify %s
extern void f1(int *);
extern void f2(char *);

struct Ok {
char c;
int x;
};

struct __attribute__((packed)) Arguable {
char c0;
int x;
char c1;
};

union __attribute__((packed)) UnionArguable {
char c;
int x;
};

typedef struct Arguable ArguableT;

struct Arguable *get_arguable();

void to_void(void *);

void g0(void) {
{
struct Ok ok;
f1(&ok.x); // no-warning
f2(&ok.c); // no-warning
}
{
struct Arguable arguable;
f2(&arguable.c0); // no-warning
f1(&arguable.x); // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
f2(&arguable.c1); // no-warning

f1((int *)(void *)&arguable.x); // no-warning
to_void(&arguable.x); // no-warning
void *p = &arguable.x; // no-warning;
to_void(p);
}
{
union UnionArguable arguable;
f2(&arguable.c); // no-warning
f1(&arguable.x); // expected-warning {{packed member 'x' of class or structure 'UnionArguable'}}

f1((int *)(void *)&arguable.x); // no-warning
to_void(&arguable.x); // no-warning
}
{
ArguableT arguable;
f2(&arguable.c0); // no-warning
f1(&arguable.x); // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
f2(&arguable.c1); // no-warning

f1((int *)(void *)&arguable.x); // no-warning
to_void(&arguable.x); // no-warning
}
{
struct Arguable *arguable = get_arguable();
f2(&arguable->c0); // no-warning
f1(&arguable->x); // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
f2(&arguable->c1); // no-warning

f1((int *)(void *)&arguable->x); // no-warning
to_void(&arguable->c1); // no-warning
}
{
ArguableT *arguable = get_arguable();
f2(&(arguable->c0)); // no-warning
f1(&(arguable->x)); // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
f2(&(arguable->c1)); // no-warning

f1((int *)(void *)&(arguable->x)); // no-warning
to_void(&(arguable->c1)); // no-warning
}
}

struct S1 {
char c;
int i __attribute__((packed));
};

int *g1(struct S1 *s1) {
return &s1->i; // expected-warning {{packed member 'i' of class or structure 'S1'}}
}

struct S2_i {
int i;
};
struct __attribute__((packed)) S2 {
char c;
struct S2_i inner;
};

int *g2(struct S2 *s2) {
return &s2->inner.i; // expected-warning {{packed member 'inner' of class or structure 'S2'}}
}

struct S2_a {
char c;
struct S2_i inner __attribute__((packed));
};

int *g2_a(struct S2_a *s2_a) {
return &s2_a->inner.i; // expected-warning {{packed member 'inner' of class or structure 'S2_a'}}
}

struct __attribute__((packed)) S3 {
char c;
struct {
int i;
} inner;
};

int *g3(struct S3 *s3) {
return &s3->inner.i; // expected-warning {{packed member 'inner' of class or structure 'S3'}}
}

struct S4 {
char c;
struct __attribute__((packed)) {
int i;
} inner;
};

int *g4(struct S4 *s4) {
return &s4->inner.i; // expected-warning {{packed member 'i' of class or structure 'S4::(anonymous)'}}
}

struct S5 {
char c;
struct {
char c1;
int i __attribute__((packed));
} inner;
};

int *g5(struct S5 *s5) {
return &s5->inner.i; // expected-warning {{packed member 'i' of class or structure 'S5::(anonymous)'}}
}

struct __attribute__((packed, aligned(2))) AlignedTo2 {
int x;
};

char *g6(struct AlignedTo2 *s) {
return (char *)&s->x; // no-warning
}

struct __attribute__((packed, aligned(2))) AlignedTo2Bis {
int x;
};

struct AlignedTo2Bis* g7(struct AlignedTo2 *s)
{
return (struct AlignedTo2Bis*)&s->x; // no-warning
}
28 changes: 28 additions & 0 deletions clang/test/SemaCXX/address-packed-member-memops.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// RUN: %clang_cc1 -fsyntax-only -verify %s
// expected-no-diagnostics

struct B {
int x, y, z, w;
} b;

struct __attribute__((packed)) A {
struct B b;
} a;

typedef __typeof__(sizeof(int)) size_t;

extern "C" {
void *memcpy(void *dest, const void *src, size_t n);
int memcmp(const void *s1, const void *s2, size_t n);
void *memmove(void *dest, const void *src, size_t n);
void *memset(void *s, int c, size_t n);
}

int x;

void foo() {
memcpy(&a.b, &b, sizeof(b));
memmove(&a.b, &b, sizeof(b));
memset(&a.b, 0, sizeof(b));
x = memcmp(&a.b, &b, sizeof(b));
}
118 changes: 118 additions & 0 deletions clang/test/SemaCXX/address-packed.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
extern void f1(int *);
extern void f2(char *);

struct __attribute__((packed)) Arguable {
int x;
char c;
static void foo();
};

extern void f3(void());

namespace Foo {
struct __attribute__((packed)) Arguable {
char c;
int x;
static void foo();
};
}

struct Arguable *get_arguable();

void f4(int &);

void to_void(void *);

template <typename... T>
void sink(T...);

void g0() {
{
Foo::Arguable arguable;
f1(&arguable.x); // expected-warning {{packed member 'x' of class or structure 'Foo::Arguable'}}
f2(&arguable.c); // no-warning
f3(&arguable.foo); // no-warning

int &w = arguable.x; // expected-error {{binding reference to packed member 'x' of class or structure 'Foo::Arguable'}}
sink(w);
f4(arguable.x); // expected-error {{binding reference to packed member 'x' of class or structure 'Foo::Arguable'}}

to_void(&arguable.x); // no-warning
void *p1 = &arguable.x; // no-warning
void *p2 = static_cast<void *>(&arguable.x); // no-warning
void *p3 = reinterpret_cast<void *>(&arguable.x); // no-warning
void *p4 = (void *)&arguable.x; // no-warning
sink(p1, p2, p3, p4);
}
{
Arguable arguable1;
Arguable &arguable(arguable1);
f1(&arguable.x); // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
f2(&arguable.c); // no-warning
f3(&arguable.foo); // no-warning
}
{
Arguable *arguable1;
Arguable *&arguable(arguable1);
f1(&arguable->x); // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
f2(&arguable->c); // no-warning
f3(&arguable->foo); // no-warning
}
}

struct __attribute__((packed)) A {
int x;
char c;

int *f0() {
return &this->x; // expected-warning {{packed member 'x' of class or structure 'A'}}
}

int *g0() {
return &x; // expected-warning {{packed member 'x' of class or structure 'A'}}
}

char *h0() {
return &c; // no-warning
}
};

struct B : A {
int *f1() {
return &this->x; // expected-warning {{packed member 'x' of class or structure 'A'}}
}

int *g1() {
return &x; // expected-warning {{packed member 'x' of class or structure 'A'}}
}

char *h1() {
return &c; // no-warning
}
};

template <typename Ty>
class __attribute__((packed)) S {
Ty X;

public:
const Ty *get() const {
return &X; // expected-warning {{packed member 'X' of class or structure 'S<int>'}}
// expected-warning@-1 {{packed member 'X' of class or structure 'S<float>'}}
}
};

template <typename Ty>
void h(Ty *);

void g1() {
S<int> s1;
s1.get(); // expected-note {{in instantiation of member function 'S<int>::get'}}

S<char> s2;
s2.get();

S<float> s3;
s3.get(); // expected-note {{in instantiation of member function 'S<float>::get'}}
}

0 comments on commit 585ea9d

Please sign in to comment.