Skip to content

Use widened bounds to update bounds in context #821

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 14 commits into from
May 11, 2020
Merged

Use widened bounds to update bounds in context #821

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
928008c
Use widened bounds to update bounds in context
Apr 14, 2020
90d162a
Merge branch 'master' into use_widened_bounds4
May 5, 2020
9499a01
Merge remote-tracking branch 'origin/master' into use_widened_bounds4
May 5, 2020
e669212
Update ObservedBounds with the widened bounds
May 6, 2020
2712d1e
Use the observed bounds as the rvalue bounds of an ArrayToPointerDeca…
May 6, 2020
2b4d323
Update LValueToRValue cast observed bounds comment
May 6, 2020
10f3c4f
Revert "Use the observed bounds as the rvalue bounds of an ArrayToPoi…
May 7, 2020
239391e
Revert "Update LValueToRValue cast observed bounds comment"
May 7, 2020
a4f71d0
Reset killed bounds to the normalized declared bounds
May 7, 2020
db85f93
Merge remote-tracking branch 'origin/master' into use_widened_bounds4
May 7, 2020
fb029a3
Update comment about ResetKilledBounds
May 7, 2020
0a33ff8
Merge remote-tracking branch 'origin/master' into use_widened_bounds4
May 11, 2020
23fd93b
Fix unit tests and add TODO in code
May 11, 2020
da129b8
Add fixed unit test again
May 11, 2020
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
2 changes: 1 addition & 1 deletion clang/include/clang/Sema/BoundsAnalysis.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ namespace clang {
// @param[in] B is the current CFGBlock.
// return A mapping of Stmts to variables whose bounds are killed by the
// Stmt.
StmtDeclSetTy GetKillSet(const clang::CFGBlock *B);
StmtDeclSetTy GetKilledBounds(const clang::CFGBlock *B);

private:
// Compute Gen set for each edge in the CFG. If there is an edge B1->B2 and
Expand Down
14 changes: 11 additions & 3 deletions clang/lib/Sema/BoundsAnalysis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -648,13 +648,21 @@ void BoundsAnalysis::ComputeOutSets(ElevatedCFGBlock *EB,
}
}

StmtDeclSetTy BoundsAnalysis::GetKillSet(const CFGBlock *B) {
ElevatedCFGBlock *EB = BlockMap[B];
StmtDeclSetTy BoundsAnalysis::GetKilledBounds(const CFGBlock *B) {
auto I = BlockMap.find(B);
if (I == BlockMap.end())
return StmtDeclSetTy();

ElevatedCFGBlock *EB = I->second;
return EB->Kill;
}

BoundsMapTy BoundsAnalysis::GetWidenedBounds(const CFGBlock *B) {
ElevatedCFGBlock *EB = BlockMap[B];
auto I = BlockMap.find(B);
if (I == BlockMap.end())
return BoundsMapTy();

ElevatedCFGBlock *EB = I->second;
return EB->In;
}

Expand Down
96 changes: 86 additions & 10 deletions clang/lib/Sema/SemaBounds.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2142,6 +2142,56 @@ namespace {
}
}

void ResetKilledBounds(StmtDeclSetTy &KilledBounds, Stmt *S,
BoundsContextTy &ObservedBounds) {
auto I = KilledBounds.find(S);
if (I == KilledBounds.end())
return;

// KilledBounds stores a mapping of statements to all variables whose
// bounds are killed by each statement. Here are remove the bounds of all
// such variables from ObservedBounds whose bounds are killed by the
// statement S. After removal, the bounds of these variables would default
// to the user declared bounds in DeclaredBounds.
for (const VarDecl *V : I->second)
ObservedBounds.erase(V);
Copy link
Contributor

@kkjeer kkjeer May 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think this should reset ObservedBounds[V] to the normalized declared bounds of V, rather than erasing V from the context. Before calling Check on a statement S, the ObservedBounds context should contain, for each variable V that is in scope at S:

  1. If V has widened bounds (lb, ub + i) at S and S does not make an assignment to V, ObservedBounds[V] => bounds(lb, ub + i)
  2. Otherwise, if V has declared bounds (lb, ub), ObservedBounds[V] => bounds(lb, ub)

The InitialObservedBounds variable should also contain these bounds. After calling Check on S, ObservedBounds may contain updated bounds due to any assignments in S. These updated observed bounds should be used to check that they imply the declared bounds for each variable. After this check, ObservedBounds should be reset to InitialObservedBounds and contain the bounds described above.

For example:

void pointer_widening() {
  nt_array_ptr<char> p : count(0) = "a";

  if (p[0]) {
    // This assignment kills the widened bounds (p, p + 1) of p.
    // Before checking this assignment, ObservedBounds should contain p => (p, p + 0).
    // The original value of p in this assignment is p - 1.
    // p - 1 should replace p in the observed bounds of p.
    // After checking this assignment, ObservedBounds should contain p => (p - 1, p - 1 + 0).
    // The updated observed bounds (p - 1, p - 1 + 0) should be used to check that they imply
    // the declared bounds (p, p + 0) for p. After this check, ObservedBounds should be reset
    // to the initial observed bounds before checking this assignment: (p, p + 0).
    p = p + 1;
  }
}

}

void UpdateCtxWithWidenedBounds(BoundsMapTy &WidenedBounds,
BoundsContextTy &ObservedBounds) {
// WidenedBounds contains the mapping from _Nt_array_ptr to the offset by
// which its declared bounds should be widened. In this function we apply
// the offset to the declared bounds of the _Nt_array_ptr and update its
// bounds in ObservedBounds.

for (const auto item : WidenedBounds) {
const VarDecl *V = item.first;
unsigned Offset = item.second;

// We normalize the declared bounds to RangBoundsExpr here so that we
// can easily apply the offset to the upper bound.
BoundsExpr *Bounds = S.ExpandBoundsToRange(V, V->getBoundsExpr());
Copy link
Member

Choose a reason for hiding this comment

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

Here too we may be allocating AST data structures multiple times. We want to avoid doing that, and do the expansion only once.

if (RangeBoundsExpr *RBE = dyn_cast<RangeBoundsExpr>(Bounds)) {
const llvm::APInt
APIntOff(Context.getTargetInfo().getPointerWidth(0), Offset);
IntegerLiteral *WidenedOffset = CreateIntegerLiteral(APIntOff);

Expr *Lower = RBE->getLowerExpr();
Expr *Upper = RBE->getUpperExpr();

// WidenedUpperBound = UpperBound + WidenedOffset.
Expr *WidenedUpper = ExprCreatorUtil::CreateBinaryOperator(
S, Upper, WidenedOffset,
BinaryOperatorKind::BO_Add);

RangeBoundsExpr *R =
new (Context) RangeBoundsExpr(Lower, WidenedUpper,
SourceLocation(), SourceLocation());
ObservedBounds[V] = R;
}
}
}

// Walk the CFG, traversing basic blocks in reverse post-oder.
// For each element of a block, check bounds declarations. Skip
// CFG elements that are subexpressions of other CFG elements.
Expand Down Expand Up @@ -2177,13 +2227,28 @@ namespace {
StmtSet MemoryCheckedStmts;
StmtSet BoundsCheckedStmts;
IdentifyChecked(Body, MemoryCheckedStmts, BoundsCheckedStmts, CheckedScopeSpecifier::CSS_Unchecked);

// Run the bounds widening analysis on this function.
BoundsAnalysis BA = getBoundsAnalyzer();
BA.WidenBounds(FD);
if (S.getLangOpts().DumpWidenedBounds)
BA.DumpWidenedBounds(FD);

PostOrderCFGView POView = PostOrderCFGView(Cfg);
ResetFacts();
for (const CFGBlock *Block : POView) {
AFA.GetFacts(Facts);
CheckingState BlockState = GetIncomingBlockState(Block, BlockStates);
// TODO: update BlockState.ObservedBounds to reflect the widened bounds
// for the block.

// Get the widened bounds for the current block as computed by the
// bounds widening analysis invoked by WidenBounds above.
BoundsMapTy WidenedBounds = BA.GetWidenedBounds(Block);
// Also get the bounds killed (if any) by each statement in the current
// block.
StmtDeclSetTy KilledBounds = BA.GetKilledBounds(Block);
// Update the Observed bounds with the widened bounds calculated above.
UpdateCtxWithWidenedBounds(WidenedBounds, BlockState.ObservedBounds);

for (CFGElement Elem : *Block) {
if (Elem.getKind() == CFGElement::Statement) {
CFGStmt CS = Elem.castAs<CFGStmt>();
Expand Down Expand Up @@ -2223,6 +2288,10 @@ namespace {
BoundsContextTy InitialObservedBounds = BlockState.ObservedBounds;
BlockState.G.clear();

// If any bounds are killed by statement S, remove their bounds
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment no longer seems quite accurate, since the bounds that are killed by S are being reset to their declared bounds rather than removed from ObservedBounds.

// from the ObservedBounds.
ResetKilledBounds(KilledBounds, S, BlockState.ObservedBounds);
Copy link
Contributor

@kkjeer kkjeer May 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think this should be done before saving InitialObservedBounds = BlockState.ObservedBounds. InitialBounds should contain the contents of ObservedBounds before calling Check(S, CSS, BlockState) in order for the observed bounds to be reset after the Check call.

For example:

void pointer_widening() {
  nt_array_ptr<char> p : count(0) = "a";

  if (p[0]) {
    // This assignment kills the widened bounds of p.
    // InitialObservedBounds should contain p => bounds(p, p + 0),
    // not p => bounds(p, p + 0 + 1).
    // After checking this assignment, BlockState.ObservedBounds should contain
    // p => bounds(p, p + 0).
    p = "";
    if (p[1]) // observed bounds of p are (p, p + 0), so this is an out-of-bounds memory access
  }
}


Check(S, CSS, BlockState);

if (DumpState)
Expand Down Expand Up @@ -4632,8 +4701,22 @@ namespace {
// given target bounds.
return TargetBounds;
}
case CastKind::CK_ArrayToPointerDecay:
case CastKind::CK_ArrayToPointerDecay: {
// For an array to pointer cast of a variable v, if v has observed
// bounds, the rvalue bounds of the value of v should be the observed
// bounds. This also accounts for variables with array type that have
// widened bounds.
if (DeclRefExpr *V = GetRValueVariable(E)) {
if (const VarDecl *D = dyn_cast_or_null<VarDecl>(V->getDecl())) {
if (BoundsExpr *B = State.ObservedBounds[D])
return B;
}
}
// If an array to pointer cast e is not the value of a variable
// with observed bounds, the rvalue bounds of e default to the
// given lvalue bounds.
return LValueBounds;
}
case CastKind::CK_DynamicPtrBounds:
case CastKind::CK_AssumePtrBounds:
llvm_unreachable("unexpected rvalue bounds cast");
Expand Down Expand Up @@ -5075,13 +5158,6 @@ void Sema::CheckFunctionBodyBoundsDecls(FunctionDecl *FD, Stmt *Body) {
Checker.Check(Body, CheckedScopeSpecifier::CSS_Unchecked);
}

if (Cfg != nullptr) {
BoundsAnalysis BA = Checker.getBoundsAnalyzer();
BA.WidenBounds(FD);
if (getLangOpts().DumpWidenedBounds)
BA.DumpWidenedBounds(FD);
}

#if TRACE_CFG
llvm::outs() << "Done " << FD->getName() << "\n";
#endif
Expand Down
20 changes: 20 additions & 0 deletions clang/test/CheckedC/inferred-bounds/widened-bounds-check.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Tests for using bounds widening to control "out-of-bounds access" errors.
//
// RUN: %clang_cc1 -verify -verify-ignore-unexpected=note -fcheckedc-extension %s

#include <limits.h>

void f1() {
_Nt_array_ptr<char> p : bounds(p, p) = "";

if (*p)
if (*(p + 1))
if (*(p + 3)) // expected-error {{out-of-bounds memory access}}
{}

if (*p) {
p++;
if (*(p + 1)) // expected-error {{out-of-bounds memory access}}
{}
}
}
38 changes: 19 additions & 19 deletions clang/test/CheckedC/inferred-bounds/widened-bounds.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ void f2() {
_Nt_array_ptr<char> p : count(0) = "ab";

if (*p)
if (*(p + 1)) // expected-error {{out-of-bounds memory access}}
if (*(p + 2)) // expected-error {{out-of-bounds memory access}}
if (*(p + 1))
if (*(p + 2))
{}

// CHECK: In function: f2
Expand Down Expand Up @@ -83,8 +83,8 @@ void f5() {
char p _Nt_checked[] : count(0) = "abc";

if (p[0])
if (p[1]) // expected-error {{out-of-bounds memory access}}
if (p[2]) // expected-error {{out-of-bounds memory access}}
if (p[1])
if (p[2])
{}

// CHECK: In function: f5
Expand Down Expand Up @@ -148,7 +148,7 @@ void f8() {
if (*p)
if (*(p + 1))
if (*(p + 2))
if (*(p + 3)) // expected-error {{out-of-bounds memory access}}
if (*(p + 3))
{}

// CHECK: In function: f8
Expand Down Expand Up @@ -266,8 +266,8 @@ void f13() {

if (p[0])
if (1[p])
if (p[2]) // expected-error {{out-of-bounds memory access}}
if (3[p]) // expected-error {{out-of-bounds memory access}}
if (p[2])
if (3[p])
{}

// CHECK: In function: f13
Expand Down Expand Up @@ -318,7 +318,7 @@ void f15(int i) {

_Nt_array_ptr<char> q : count(0) = "a";
if (*q)
if (*(q - 1)) // expected-error {{out-of-bounds memory access}}
if (*(q - 1))
{}

// CHECK: [B7]
Expand Down Expand Up @@ -354,7 +354,7 @@ void f16(_Nt_array_ptr<char> p : bounds(p, p)) {
_Nt_array_ptr<char> r : bounds(p, p + 1) = "a";

if (*(p))
if (*(p + 1)) // expected-error {{out-of-bounds memory access}}
if (*(p + 1))
{}

// CHECK: In function: f16
Expand Down Expand Up @@ -446,9 +446,9 @@ void f19() {
_Nt_array_ptr<char> p : count(0) = "a";

if (*p)
if (*(p + 1)) // expected-error {{out-of-bounds memory access}}
if (*(p + 3)) // expected-error {{out-of-bounds memory access}}
if (*(p + 2)) // expected-error {{out-of-bounds memory access}}
if (*(p + 1))
if (*(p + 3))
if (*(p + 2))
{}

// CHECK: In function: f19
Expand Down Expand Up @@ -539,8 +539,8 @@ void f21() {
char p _Nt_checked[] : count(0) = "abc";

while (p[0])
while (p[1]) // expected-error {{out-of-bounds memory access}}
while (p[2]) // expected-error {{out-of-bounds memory access}}
while (p[1])
while (p[2])
{}

// CHECK: In function: f21
Expand All @@ -564,8 +564,8 @@ void f22() {
_Nt_array_ptr<char> p : count(0) = "a";

if (*p)
while (*(p + 1)) // expected-error {{out-of-bounds memory access}}
if (*(p + 2)) // expected-error {{out-of-bounds memory access}}
while (*(p + 1))
if (*(p + 2))
{}

// CHECK: In function: f22
Expand Down Expand Up @@ -614,7 +614,7 @@ B: p;

while (*p) {
p;
while (*(p + 1)) { // expected-error {{out-of-bounds memory access}}
while (*(p + 1)) {
C: p;
}
}
Expand Down Expand Up @@ -673,9 +673,9 @@ void f25() {

for (; *p; ) {
i = 0;
for (; *(p + 1); ) { // expected-error {{out-of-bounds memory access}}
for (; *(p + 1); ) {
i = 1;
for (; *(p + 2); ) { // expected-error {{out-of-bounds memory access}}
for (; *(p + 2); ) {
i = 2;
}
}
Expand Down