dkrupp created this revision.
dkrupp added reviewers: xazax.hun, NoQ, dcoughlin, zaks.anna.
dkrupp added a subscriber: cfe-commits.
ArrayBoundChecker did not detect out of bounds memory access errors in case an
array was allocated by the new expression.
1. MallocChecker.cpp was updated to calculate the extent size in Bytes
similarly how it was done for memory regions allocated by malloc. The size
constraint is added for arrays and non-arrays allocated by new.
2. ArrayBoundCheckerV2.cpp was updated to better handle accessing locations
preceding a symbolic memory region (such as buf[-1] in test2(..) in
out-of-bounds.cpp). So computeExtentBegin(..) was updated to assume that the
extent of a symbolic region starts at 0 if we know the size of the extent (as
is the case in case of malloc or new).
3. out-of-bounds.cpp contains the relevant test cases for C++.
https://reviews.llvm.org/D24307
Files:
lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
test/Analysis/out-of-bounds.cpp
Index: test/Analysis/out-of-bounds.cpp
===================================================================
--- /dev/null
+++ test/Analysis/out-of-bounds.cpp
@@ -0,0 +1,147 @@
+// RUN: %clang_cc1 -std=c++11 -Wno-array-bounds -analyze -analyzer-checker=unix,core,alpha.security.ArrayBoundV2 -verify %s
+
+// Tests doing an out-of-bounds access after the end of an array using:
+// - constant integer index
+// - constant integer size for buffer
+void test1(int x) {
+ int *buf = new int[100];
+ buf[100] = 1; // expected-warning{{Out of bound memory access}}
+}
+
+void test1_ok(int x) {
+ int *buf = new int[100];
+ buf[99] = 1; // no-warning
+}
+
+// Tests doing an out-of-bounds access after the end of an array using:
+// - indirect pointer to buffer
+// - constant integer index
+// - constant integer size for buffer
+void test1_ptr(int x) {
+ int *buf = new int[100];
+ int *p = buf;
+ p[101] = 1; // expected-warning{{Out of bound memory access}}
+}
+
+void test1_ptr_ok(int x) {
+ int *buf = new int[100];
+ int *p = buf;
+ p[99] = 1; // no-warning
+}
+
+// Tests doing an out-of-bounds access before the start of an array using:
+// - indirect pointer to buffer, manipulated using simple pointer arithmetic
+// - constant integer index
+// - constant integer size for buffer
+void test1_ptr_arith(int x) {
+ int *buf = new int[100];
+ int *p = buf;
+ p = p + 100;
+ p[0] = 1; // expected-warning{{Out of bound memory access}}
+}
+
+void test1_ptr_arith_ok(int x) {
+ int *buf = new int[100];
+ int *p = buf;
+ p = p + 99;
+ p[0] = 1; // no-warning
+}
+
+void test1_ptr_arith_bad(int x) {
+ int *buf = new int[100];
+ int *p = buf;
+ p = p + 99;
+ p[1] = 1; // expected-warning{{Out of bound memory access}}
+}
+
+void test1_ptr_arith_ok2(int x) {
+ int *buf = new int[100];
+ int *p = buf;
+ p = p + 99;
+ p[-1] = 1; // no-warning
+}
+
+// Tests doing an out-of-bounds access before the start of an array using:
+// - constant integer index
+// - constant integer size for buffer
+void test2(int x) {
+ int *buf = new int[100];
+ buf[-1] = 1; // expected-warning{{Out of bound memory access}}
+}
+
+// Tests doing an out-of-bounds access before the start of an array using:
+// - indirect pointer to buffer
+// - constant integer index
+// - constant integer size for buffer
+void test2_ptr(int x) {
+ int *buf = new int[100];
+ int *p = buf;
+ p[-1] = 1; // expected-warning{{Out of bound memory access}}
+}
+
+// Tests doing an out-of-bounds access before the start of an array using:
+// - indirect pointer to buffer, manipulated using simple pointer arithmetic
+// - constant integer index
+// - constant integer size for buffer
+void test2_ptr_arith(int x) {
+ int *buf = new int[100];
+ int *p = buf;
+ --p;
+ p[0] = 1; // expected-warning {{Out of bound memory access (accessed memory precedes memory block)}}
+}
+
+// Tests doing an out-of-bounds access before the start of a multi-dimensional
+// array using:
+// - constant integer indices
+// - constant integer sizes for the array
+void test2_multi(int x) {
+ auto buf = new int[100][100];
+ buf[0][-1] = 1; // expected-warning{{Out of bound memory access}}
+}
+
+// Tests doing an out-of-bounds access before the start of a multi-dimensional
+// array using:
+// - constant integer indices
+// - constant integer sizes for the array
+void test2_multi_b(int x) {
+ auto buf = new int[100][100];
+ buf[-1][0] = 1; // expected-warning{{Out of bound memory access}}
+}
+
+// Tests doing an out-of-bounds access before the start of a multi-dimensional
+// array using:
+// - constant integer indices
+// - constant integer sizes for the array
+void test2_multi_c(int x) {
+ auto buf = new int[100][100];
+ buf[100][0] = 1; // expected-warning{{Out of bound memory access}}
+}
+
+// Tests doing an out-of-bounds access before the start of a multi-dimensional
+// array using:
+// - constant integer indices
+// - constant integer sizes for the array
+void test2_multi_2(int x) {
+ auto buf = new int[100][100];
+ buf[99][100] = 1; // expected-warning{{Out of bound memory access}}
+}
+
+// Tests doing an out-of-bounds access before the start of a multi-dimensional
+// array using:
+// - constant integer indices
+// - constant integer sizes for the array
+void test2_multi_ok(int x) {
+ auto buf = new int[100][100];
+ buf[0][0] = 1; // no-warning
+}
+
+// Tests over-indexing using different types
+// array using:
+// - constant integer indices
+// - constant integer sizes for the array
+void test_diff_types(int x) {
+ int *buf = new int[10]; //40 Bytes allocated
+ char *cptr = (char *)buf;
+ cptr[sizeof(int) * 9] = 1; // no-warning
+ cptr[sizeof(int) * 10] = 1; // expected-warning{{Out of bound memory access}}
+}
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -287,6 +287,17 @@
ProgramStateRef State,
AllocationFamily Family = AF_Malloc);
+ static ProgramStateRef addExtentSize(CheckerContext &C,
+ const CXXNewExpr *NE,
+ ProgramStateRef State
+ );
+
+ // Scale a base value by a scaling factor, and return the scaled
+ // value as an SVal.
+ static SVal scaleValue(ProgramStateRef State,
+ NonLoc BaseVal, CharUnits Scaling,
+ SValBuilder &Sb);
+
// Check if this malloc() for special flags. At present that means M_ZERO or
// __GFP_ZERO (in which case, treat it like calloc).
llvm::Optional<ProgramStateRef>
@@ -981,10 +992,65 @@
// existing binding.
State = MallocUpdateRefState(C, NE, State, NE->isArray() ? AF_CXXNewArray
: AF_CXXNew);
+ State=addExtentSize(C,NE,State);
State = ProcessZeroAllocation(C, NE, 0, State);
C.addTransition(State);
}
+// Sets the extent value of the MemRegion allocated by
+// new expression NE to its size in Bytes.
+//
+ProgramStateRef MallocChecker::addExtentSize(CheckerContext &C,
+ const CXXNewExpr *NE,
+ ProgramStateRef State) {
+ if (!State)
+ return nullptr;
+ SValBuilder &svalBuilder = C.getSValBuilder();
+ SVal Size;
+ const LocationContext *LCtx = C.getPredecessor()->getLocationContext();
+ const MemRegion *Region;
+ if (NE->isArray()) {
+ const Expr *SizeExpr = NE->getArraySize();
+ Size = State->getSVal(SizeExpr, C.getLocationContext());
+ Region = (State->getSVal(NE, LCtx))
+ .getAsRegion()
+ ->getAs<SubRegion>()
+ ->getSuperRegion();
+ } else {
+ Size = svalBuilder.makeIntVal(1, true);
+ Region = (State->getSVal(NE, LCtx)).getAsRegion()->getAs<SubRegion>();
+ }
+ assert(Region);
+
+ // Set the region's extent equal to the Size parameter.
+ const SymbolicRegion *R = dyn_cast<SymbolicRegion>(Region);
+ if (!R)
+ return nullptr;
+
+ QualType ElementType = NE->getAllocatedType();
+ ASTContext &AstContext = C.getASTContext();
+ CharUnits TypeSize = AstContext.getTypeSizeInChars(ElementType);
+
+ if (Optional<DefinedOrUnknownSVal> DefinedSize =
+ Size.getAs<DefinedOrUnknownSVal>()) {
+ DefinedOrUnknownSVal Extent = R->getExtent(svalBuilder);
+ NonLoc s = Size.castAs<NonLoc>();
+ SVal SizeInBytes = scaleValue(State, s, TypeSize, svalBuilder);
+ DefinedOrUnknownSVal extentMatchesSize = svalBuilder.evalEQ(
+ State, Extent, SizeInBytes.castAs<DefinedOrUnknownSVal>());
+ State = State->assume(extentMatchesSize, true);
+ }
+ return State;
+}
+// Scale a base value by a scaling factor, and return the scaled
+// value as an SVal.
+SVal MallocChecker::scaleValue(ProgramStateRef State, NonLoc BaseVal,
+ CharUnits Scaling, SValBuilder &Sb) {
+ return Sb.evalBinOpNN(State, BO_Mul, BaseVal,
+ Sb.makeArrayIndex(Scaling.getQuantity()),
+ Sb.getArrayIndexType());
+}
+
void MallocChecker::checkPreStmt(const CXXDeleteExpr *DE,
CheckerContext &C) const {
Index: lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
+++ lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
@@ -67,19 +67,30 @@
}
static SVal computeExtentBegin(SValBuilder &svalBuilder,
- const MemRegion *region) {
- while (true)
+ const MemRegion *region, ProgramStateRef state) {
+ while (true) {
switch (region->getKind()) {
- default:
- return svalBuilder.makeZeroArrayIndex();
- case MemRegion::SymbolicRegionKind:
- // FIXME: improve this later by tracking symbolic lower bounds
- // for symbolic regions.
+ default:
+ return svalBuilder.makeZeroArrayIndex();
+ case MemRegion::SymbolicRegionKind: {
+ // FIXME: improve this later by tracking symbolic lower bounds
+ // for symbolic regions.
+ DefinedOrUnknownSVal extentVal =
+ cast<SubRegion>(region)->getExtent(svalBuilder);
+ // If we known the symbolic region extent size
+ //(e.g. allocated by malloc or new)
+ // we can assume that the region starts at 0.
+ if (!state->isNull(extentVal).isConstrained()) {
return UnknownVal();
- case MemRegion::ElementRegionKind:
- region = cast<SubRegion>(region)->getSuperRegion();
- continue;
+ } else {
+ return svalBuilder.makeZeroArrayIndex();
+ }
+ }
+ case MemRegion::ElementRegionKind:
+ region = cast<SubRegion>(region)->getSuperRegion();
+ continue;
}
+ }
}
// TODO: once the constraint manager is smart enough to handle non simplified
@@ -147,7 +158,7 @@
// If so, we are doing a load/store
// before the first valid offset in the memory region.
- SVal extentBegin = computeExtentBegin(svalBuilder, rawOffset.getRegion());
+ SVal extentBegin = computeExtentBegin(svalBuilder, rawOffset.getRegion(),state);
if (Optional<NonLoc> NV = extentBegin.getAs<NonLoc>()) {
if (NV->getAs<nonloc::ConcreteInt>()) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
