Skip to content

[Clang] Correct handling of negative and out-of-bounds indices #71877

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 20, 2023
+172 −155
Merged

[Clang] Correct handling of negative and out-of-bounds indices #71877

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7214158
[Clang] Correct handling of negative and out-of-bounds indices
bwendling Nov 9, 2023
dd9866b
Merge branch 'main' into llvm-project/counted-by-offsets
bwendling Nov 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
139 changes: 73 additions & 66 deletions clang/lib/CodeGen/CGBuiltin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -851,78 +851,94 @@ CodeGenFunction::emitFlexibleArrayMemberSize(const Expr *E, unsigned Type,
// max(sizeof(struct s),
// offsetof(struct s, array) + p->count * sizeof(*p->array))
//
ASTContext &Ctx = getContext();
const Expr *Base = E->IgnoreParenImpCasts();
const Expr *Idx = nullptr;

if (const auto *UO = dyn_cast<UnaryOperator>(Base);
UO && UO->getOpcode() == UO_AddrOf) {
if (const auto *ASE =
dyn_cast<ArraySubscriptExpr>(UO->getSubExpr()->IgnoreParens())) {
Base = ASE->getBase();
Expr *SubExpr = UO->getSubExpr()->IgnoreParenImpCasts();
if (const auto *ASE = dyn_cast<ArraySubscriptExpr>(SubExpr)) {
Base = ASE->getBase()->IgnoreParenImpCasts();
Idx = ASE->getIdx()->IgnoreParenImpCasts();

if (const auto *IL = dyn_cast<IntegerLiteral>(Idx);
IL && !IL->getValue().getSExtValue())
Idx = nullptr;
if (const auto *IL = dyn_cast<IntegerLiteral>(Idx)) {
int64_t Val = IL->getValue().getSExtValue();
if (Val < 0)
// __bdos returns 0 for negative indexes into an array in a struct.
return getDefaultBuiltinObjectSizeResult(Type, ResType);

if (Val == 0)
// The index is 0, so we don't need to take it into account.
Idx = nullptr;
}
} else {
// Potential pointer to another element in the struct.
Base = SubExpr;
}
}

// Get the flexible array member Decl.
const ValueDecl *FAMDecl = nullptr;
if (const auto *ME = dyn_cast<MemberExpr>(Base)) {
// Check if \p Base is referencing the FAM itself.
if (const ValueDecl *MD = ME->getMemberDecl()) {
const LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel =
getLangOpts().getStrictFlexArraysLevel();
if (!Decl::isFlexibleArrayMemberLike(
Ctx, MD, MD->getType(), StrictFlexArraysLevel,
/*IgnoreTemplateOrMacroSubstitution=*/true))
return nullptr;

FAMDecl = MD;
}
} else if (const auto *DRE = dyn_cast<DeclRefExpr>(Base)) {
// Check if we're pointing to the whole struct.
QualType Ty = DRE->getDecl()->getType();
if (Ty->isPointerType())
Ty = Ty->getPointeeType();

if (const auto *RD = Ty->getAsRecordDecl())
// Don't use the outer lexical record because the FAM might be in a
// different RecordDecl.
FAMDecl = FindFlexibleArrayMemberField(Ctx, RD);
}

if (!FAMDecl || !FAMDecl->hasAttr<CountedByAttr>())
// No flexible array member found or it doesn't have the "counted_by"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the "alloc_size" attribute?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are already tests for alloc_size and __bos() and __bdos(). (https://github.com/llvm/llvm-project/blob/c2a2fd209e2da4c80e55d11b114d47b8d1eaaa16/clang/test/CodeGen/alloc-size.c)

// attribute.
return nullptr;

const ValueDecl *CountedByFD = FindCountedByField(Base);
if (!CountedByFD)
// Can't find the field referenced by the "counted_by" attribute.
return nullptr;

// Build a load of the counted_by field.
bool IsSigned = CountedByFD->getType()->isSignedIntegerType();
const RecordDecl *OuterRD =
CountedByFD->getDeclContext()->getOuterLexicalRecordContext();
ASTContext &Ctx = getContext();

// Load the counted_by field.
const Expr *CountedByExpr = BuildCountedByFieldExpr(Base, CountedByFD);
Value *CountedByInst = EmitAnyExprToTemp(CountedByExpr).getScalarVal();
llvm::Type *CountedByTy = CountedByInst->getType();

// Build a load of the index and subtract it from the count.
Value *IdxInst = nullptr;
if (Idx) {
// There's an index into the array. Remove it from the count.
bool IdxSigned = Idx->getType()->isSignedIntegerType();
Value *IdxInst = EmitAnyExprToTemp(Idx).getScalarVal();
IdxInst = EmitAnyExprToTemp(Idx).getScalarVal();
IdxInst = IdxSigned ? Builder.CreateSExtOrTrunc(IdxInst, CountedByTy)
: Builder.CreateZExtOrTrunc(IdxInst, CountedByTy);

// If the index is negative, don't subtract it from the counted_by
// value. The pointer is pointing to something before the FAM.
IdxInst = Builder.CreateNeg(IdxInst, "", !IdxSigned, IdxSigned);
// We go ahead with the calculation here. If the index turns out to be
// negative, we'll catch it at the end.
CountedByInst =
Builder.CreateAdd(CountedByInst, IdxInst, "", !IsSigned, IsSigned);
Builder.CreateSub(CountedByInst, IdxInst, "", !IsSigned, IsSigned);
}

// Get the size of the flexible array member's base type.
const ValueDecl *FAMDecl = nullptr;
if (const auto *ME = dyn_cast<MemberExpr>(Base)) {
const LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel =
getLangOpts().getStrictFlexArraysLevel();
if (const ValueDecl *MD = ME->getMemberDecl()) {
if (!Decl::isFlexibleArrayMemberLike(
Ctx, MD, MD->getType(), StrictFlexArraysLevel,
/*IgnoreTemplateOrMacroSubstitution=*/true))
return nullptr;
// Base is referencing the FAM itself.
FAMDecl = MD;
}
}

if (!FAMDecl)
FAMDecl = FindFlexibleArrayMemberField(Ctx, OuterRD);

assert(FAMDecl && "Can't find the flexible array member field");

// Calculate how large the flexible array member is in bytes.
const ArrayType *ArrayTy = Ctx.getAsArrayType(FAMDecl->getType());
CharUnits Size = Ctx.getTypeSizeInChars(ArrayTy->getElementType());
llvm::Constant *ElemSize =
llvm::ConstantInt::get(CountedByTy, Size.getQuantity(), IsSigned);

// Calculate how large the flexible array member is in bytes.
Value *FAMSize =
Builder.CreateMul(CountedByInst, ElemSize, "", !IsSigned, IsSigned);
FAMSize = IsSigned ? Builder.CreateSExtOrTrunc(FAMSize, ResType)
Expand All @@ -931,47 +947,38 @@ CodeGenFunction::emitFlexibleArrayMemberSize(const Expr *E, unsigned Type,

if (const auto *DRE = dyn_cast<DeclRefExpr>(Base)) {
// The whole struct is specificed in the __bdos.
const RecordDecl *OuterRD =
CountedByFD->getDeclContext()->getOuterLexicalRecordContext();
const ASTRecordLayout &Layout = Ctx.getASTRecordLayout(OuterRD);

// Get the offset of the FAM.
CharUnits Offset = Ctx.toCharUnitsFromBits(Ctx.getFieldOffset(FAMDecl));
llvm::Constant *FAMOffset =
ConstantInt::get(ResType, Offset.getQuantity(), IsSigned);

// max(sizeof(struct s),
// offsetof(struct s, array) + p->count * sizeof(*p->array))
Value *OffsetAndFAMSize =
Builder.CreateAdd(FAMOffset, Res, "", !IsSigned, IsSigned);

// Get the full size of the struct.
llvm::Constant *SizeofStruct =
ConstantInt::get(ResType, Layout.getSize().getQuantity(), IsSigned);

// max(sizeof(struct s),
// offsetof(struct s, array) + p->count * sizeof(*p->array))
Res = IsSigned
? Builder.CreateBinaryIntrinsic(llvm::Intrinsic::smax,
OffsetAndFAMSize, SizeofStruct)
: Builder.CreateBinaryIntrinsic(llvm::Intrinsic::umax,
OffsetAndFAMSize, SizeofStruct);
} else if (const auto *ME = dyn_cast<MemberExpr>(Base)) {
// Pointing to a place before the FAM. Add the difference to the FAM's
// size.
if (const ValueDecl *MD = ME->getMemberDecl(); MD != FAMDecl) {
CharUnits Offset = Ctx.toCharUnitsFromBits(Ctx.getFieldOffset(MD));
CharUnits FAMOffset =
Ctx.toCharUnitsFromBits(Ctx.getFieldOffset(FAMDecl));

Res = Builder.CreateAdd(
Res, ConstantInt::get(ResType, FAMOffset.getQuantity() -
Offset.getQuantity()));
}
}

// A negative 'FAMSize' means that the index was greater than the count,
// or an improperly set count field. Return -1 (for types 0 and 1) or 0
// (for types 2 and 3).
return Builder.CreateSelect(Builder.CreateIsNeg(FAMSize),
getDefaultBuiltinObjectSizeResult(Type, ResType),
Res);
// A negative \p IdxInst or \p CountedByInst means that the index lands
// outside of the flexible array member. If that's the case, we want to
// return 0.
Value *Cmp = Builder.CreateIsNotNeg(CountedByInst);
if (IdxInst)
Cmp = Builder.CreateAnd(Builder.CreateIsNotNeg(IdxInst), Cmp);

return Builder.CreateSelect(Cmp, Res, ConstantInt::get(ResType, 0, IsSigned));
}

/// Returns a Value corresponding to the size of the given expression.
Expand Down Expand Up @@ -1006,19 +1013,19 @@ CodeGenFunction::emitBuiltinObjectSize(const Expr *E, unsigned Type,
}
}

// LLVM can't handle Type=3 appropriately, and __builtin_object_size shouldn't
// evaluate E for side-effects. In either case, we shouldn't lower to
// @llvm.objectsize.
if (Type == 3 || (!EmittedE && E->HasSideEffects(getContext())))
return getDefaultBuiltinObjectSizeResult(Type, ResType);

if (IsDynamic) {
// Emit special code for a flexible array member with the "counted_by"
// attribute.
if (Value *V = emitFlexibleArrayMemberSize(E, Type, ResType))
return V;
}

// LLVM can't handle Type=3 appropriately, and __builtin_object_size shouldn't
// evaluate E for side-effects. In either case, we shouldn't lower to
// @llvm.objectsize.
if (Type == 3 || (!EmittedE && E->HasSideEffects(getContext())))
return getDefaultBuiltinObjectSizeResult(Type, ResType);

Value *Ptr = EmittedE ? EmittedE : EmitScalarExpr(E);
assert(Ptr->getType()->isPointerTy() &&
"Non-pointer passed to __builtin_object_size?");
Expand Down
Loading