Re: [PATCH] D24307: calculate extent size for memory regions allocated by C++ new expression
This revision was automatically updated to reflect the committed changes.
Closed by commit rL281934: [analyzer] Calculate extent size for memory regions
allocated by new expression. (authored by xazax).
Changed prior to commit:
https://reviews.llvm.org/D24307?vs=70821=71874#toc
Repository:
rL LLVM
https://reviews.llvm.org/D24307
Files:
cfe/trunk/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
cfe/trunk/test/Analysis/out-of-bounds-new.cpp
Index: cfe/trunk/test/Analysis/out-of-bounds-new.cpp
===
--- cfe/trunk/test/Analysis/out-of-bounds-new.cpp
+++ cfe/trunk/test/Analysis/out-of-bounds-new.cpp
@@ -0,0 +1,150 @@
+// 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 under-indexing
+// of a multi-dimensional array
+void test2_multi(int x) {
+ auto buf = new int[100][100];
+ buf[0][-1] = 1; // expected-warning{{Out of bound memory access}}
+}
+
+// Tests under-indexing
+// of a multi-dimensional 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 over-indexing
+// of a multi-dimensional 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 over-indexing
+// of a multi-dimensional 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 normal access of
+// a multi-dimensional 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
+void test_diff_types(int x) {
+ int *buf = new int[10]; //10*sizeof(int) 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}}
+}
+
+// Tests over-indexing
+//if the allocated area is non-array
+void test_non_array(int x) {
+ int *ip = new int;
+ ip[0] = 1; // no-warning
+ ip[1] = 2; // expected-warning{{Out of bound memory access}}
+}
+
+//Tests over-indexing
+//if the allocated area size is a runtime parameter
+void
Re: [PATCH] D24307: calculate extent size for memory regions allocated by C++ new expression
dkrupp added a comment. Thanks. Gabor, could you please merge this? I don't have commit right. https://reviews.llvm.org/D24307 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24307: calculate extent size for memory regions allocated by C++ new expression
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Looks good! Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:291 @@ +290,3 @@ + static ProgramStateRef addExtentSize(CheckerContext , + const CXXNewExpr *NE, + ProgramStateRef State Whitespace a bit strange. Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:997 @@ +996,3 @@ +// +ProgramStateRef MallocChecker::addExtentSize(CheckerContext , + const CXXNewExpr *NE, >> Perhaps ExprEngine would be the proper place for that code as well. > Interesting point. Can you clarify the last sentence? I'm thinking that the standard operator new() should be properly modeled by the analyzer core; we are already doing this with respect to memory space of the region it returns, why not do that for extent as well, somewhere at the same place. We could probably make a refactoring pass over `MallocChecker` to move things around and return it to a readable state. Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1011 @@ +1010,3 @@ +// containing the elements. +Region = (State->getSVal(NE, LCtx)) + .getAsRegion() dkrupp wrote: > MemRegion has now method called castAs<>, only getAs<>, so I stayed with it. Ouch, i meant, `cast(State->getSVal(NE, LCtx).getAsRegion())` etc. https://reviews.llvm.org/D24307 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24307: calculate extent size for memory regions allocated by C++ new expression
zaks.anna added a comment. I do not have any more comments; however, let's wait for @NoQ to review this as well. Thanks! https://reviews.llvm.org/D24307 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24307: calculate extent size for memory regions allocated by C++ new expression
dkrupp marked 11 inline comments as done. dkrupp added a comment. issues fixed https://reviews.llvm.org/D24307 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24307: calculate extent size for memory regions allocated by C++ new expression
dkrupp added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1011
@@ +1010,3 @@
+// containing the elements.
+Region = (State->getSVal(NE, LCtx))
+ .getAsRegion()
MemRegion has now method called castAs<>, only getAs<>, so I stayed with it.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1020
@@ +1019,3 @@
+ }
+ assert(Region);
+
I changed the type of Region to SubRegion, hope this is clearer this way.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1043
@@ -988,3 +1042,3 @@
void MallocChecker::checkPreStmt(const CXXDeleteExpr *DE,
CheckerContext ) const {
now inlined
https://reviews.llvm.org/D24307
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24307: calculate extent size for memory regions allocated by C++ new expression
dkrupp added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:83
@@ -78,1 +82,3 @@
+ // we can assume that the region starts at 0.
+ if (!state->isNull(extentVal).isConstrained()) {
return UnknownVal();
NoQ wrote:
> Perhaps you could consider the memory space of the `region`, it would look a
> bit less hacky to me.
>
> In my dreams, i wish heap regions were no longer symbolic regions, and this
> hack would go away then.
>
> Also, i recall there is a bug in `isNull()`: in the `ConstraintManager` class
> (this time i actually mean //the abstract base class// of
> `RangeConstraintManager`) this function boils down to `assume()`, but in
> `RangeConstraintManager` it is overridden to do a direct lookup into the
> constraint map; which means that in fact this function does not simplify
> symbolic expressions before answering. This code is probably unaffected
> because extents are always either concrete or atomic symbols, but i think i'd
> make a patch for that.
Good point!
region->getMemorySpace() does a very similar recursion as the while loop in
this function. So I guess the while loop can be refactored like this:
```
static SVal computeExtentBegin(SValBuilder ,
const MemRegion *region) {
const MemSpaceRegion *SR = region->getMemorySpace();
if (SR->getKind() == MemRegion::UnknownSpaceRegionKind)
return UnknownVal();
else
return svalBuilder.makeZeroArrayIndex();
}
```
All test cases pass. Particularly it filters out this false positive from
out-of-bounds.c :
```
// Don't warn when indexing below the start of a symbolic region's whose
// base extent we don't know.
int *get_symbolic();
void test_index_below_symboloc() {
int *buf = get_symbolic();
buf[-1] = 0; // no-warning;
}
```
https://reviews.llvm.org/D24307
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24307: calculate extent size for memory regions allocated by C++ new expression
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1003
@@ +1002,3 @@
+//
+ProgramStateRef MallocChecker::addExtentSize(CheckerContext ,
+ const CXXNewExpr *NE,
NoQ wrote:
> dkrupp wrote:
> > xazax.hun wrote:
> > > zaks.anna wrote:
> > > > I am not sure this code belongs to the malloc checker since it only
> > > > supports the array bounds checker. Is there a reason it's not part of
> > > > that checker?
> > > I think it is part of the malloc checker because it already does
> > > something very very similar to malloc, see the MallocMemAux function. So
> > > in fact, for the array bounds checker to work properly, the malloc
> > > checker should be turned on.
> > Extent size is used by ArrayBoundChecker, ArrayBoundCheckerV2 and
> > CStringChecker checkers currently. New expression in case of simple
> > allocations (0 allocation) was already handled in Malloc checker , that's
> > why I implemented it there. But I agree it feels odd that one has to switch
> > on unix.Malloc checker to get the size of new allocated heap regions.
> > Should I move this to ArrayBoundChecker or ArrayBoundCheckerV2?
> 1. Well, in my dreams, this code should be in the silent operator-new
> modelling checker, which would be separate from the stateless operator-new
> bug-finding checker. Then all the checkers that rely on extent would
> automatically load the modelling checker as a dependency.
>
> 2. Yeah, i think many checkers may consider extents, not just array bound;
> other checkers may appear in the future. This info is very useful, which is
> why we have the whole symbol class just for that (however, checker
> communication through constraints on this symbol class wasn't IMHO a good
> idea, because it's harder for the constraint manager to deal with symbols and
> constraints rather than with concrete values).
>
> //Just wanted to speak out, thoughts below might perhaps be more on-topic//
>
> 3. The MallocChecker is not just unix.Malloc, but also cplusplus.NewDelete,
> etc., which makes a lot more sense to leave it here.
>
> 4. Consider another part of MallocChecker's job - modeling the memory spaces
> for symbolic regions (heap vs. unknown). For malloc(), this is done in the
> checker. For operator-new, it is done in the ExprEngine(!), because this is
> part of the language rather than a library function. Perhaps ExprEngine would
> be the proper place for that code as well.
> Well, in my dreams, this code should be in the silent operator-new modelling
> checker, which would be
> separate from the stateless operator-new bug-finding checker. Then all the
> checkers that rely on extent
> would automatically load the modelling checker as a dependency.
I agree. This sounds like the best approach! (Though it's not a must have to
get the patch in.)
> Consider another part of MallocChecker's job - modeling the memory spaces for
> symbolic regions (heap vs.
> unknown). For malloc(), this is done in the checker. For operator-new, it is
> done in the ExprEngine(!), because
> this is part of the language rather than a library function. Perhaps
> ExprEngine would be the proper place for
> that code as well.
Interesting point. Can you clarify the last sentence?
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1020
@@ +1019,3 @@
+ } else {
+Size = svalBuilder.makeIntVal(1, true);
+Region = (State->getSVal(NE, LCtx)).getAsRegion()->getAs();
Please, be more explicit that this is not a size of allocation (as in not 1
byte)? Maybe call this ElementCount or something like that.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1021
@@ +1020,3 @@
+Size = svalBuilder.makeIntVal(1, true);
+Region = (State->getSVal(NE, LCtx)).getAsRegion()->getAs();
+ }
A bit of code repetition from above. Please, add comments explaining why we
need subregion in one case and super region in the other.
Are there test cases for this branch?
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1049
@@ +1048,3 @@
+ CharUnits Scaling, SValBuilder ) {
+ return Sb.evalBinOpNN(State, BO_Mul, BaseVal,
+Sb.makeArrayIndex(Scaling.getQuantity()),
This should probably be inline/have different name since it talks about
ArrayIndexType and is not reused elsewhere.
Comment at: test/Analysis/out-of-bounds.cpp:1
@@ +1,2 @@
+// RUN: %clang_cc1 -std=c++11 -Wno-array-bounds -analyze
-analyzer-checker=unix,core,alpha.security.ArrayBoundV2 -verify %s
+
Let's use a more specific file name.
https://reviews.llvm.org/D24307
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24307: calculate extent size for memory regions allocated by C++ new expression
NoQ added a comment.
Thanks for the patch! Not sure why, but i always have warm feelings for the
`MallocChecker` and wish it to improve.
Added minor style comments, joined the "where to put the code" debate.
Comment at: lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:69
@@ -68,2 +68,3 @@
static SVal computeExtentBegin(SValBuilder ,
+ const MemRegion *region, ProgramStateRef state)
{
For ease of use, you no longer need to pass `svalBuilder` here - you can take
it from the `state->getStateManager()`.
Comment at: lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:81
@@ +80,3 @@
+ // If we known the symbolic region extent size
+ //(e.g. allocated by malloc or new)
+ // we can assume that the region starts at 0.
Whitespace slightly off.
Comment at: lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:83
@@ -78,1 +82,3 @@
+ // we can assume that the region starts at 0.
+ if (!state->isNull(extentVal).isConstrained()) {
return UnknownVal();
Perhaps you could consider the memory space of the `region`, it would look a
bit less hacky to me.
In my dreams, i wish heap regions were no longer symbolic regions, and this
hack would go away then.
Also, i recall there is a bug in `isNull()`: in the `ConstraintManager` class
(this time i actually mean //the abstract base class// of
`RangeConstraintManager`) this function boils down to `assume()`, but in
`RangeConstraintManager` it is overridden to do a direct lookup into the
constraint map; which means that in fact this function does not simplify
symbolic expressions before answering. This code is probably unaffected because
extents are always either concrete or atomic symbols, but i think i'd make a
patch for that.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:995
@@ -983,2 +994,3 @@
: AF_CXXNew);
+ State=addExtentSize(C,NE,State);
State = ProcessZeroAllocation(C, NE, 0, State);
This code could use a bit more spaces around "=" and ",".
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1003
@@ +1002,3 @@
+//
+ProgramStateRef MallocChecker::addExtentSize(CheckerContext ,
+ const CXXNewExpr *NE,
dkrupp wrote:
> xazax.hun wrote:
> > zaks.anna wrote:
> > > I am not sure this code belongs to the malloc checker since it only
> > > supports the array bounds checker. Is there a reason it's not part of
> > > that checker?
> > I think it is part of the malloc checker because it already does something
> > very very similar to malloc, see the MallocMemAux function. So in fact, for
> > the array bounds checker to work properly, the malloc checker should be
> > turned on.
> Extent size is used by ArrayBoundChecker, ArrayBoundCheckerV2 and
> CStringChecker checkers currently. New expression in case of simple
> allocations (0 allocation) was already handled in Malloc checker , that's why
> I implemented it there. But I agree it feels odd that one has to switch on
> unix.Malloc checker to get the size of new allocated heap regions. Should I
> move this to ArrayBoundChecker or ArrayBoundCheckerV2?
1. Well, in my dreams, this code should be in the silent operator-new modelling
checker, which would be separate from the stateless operator-new bug-finding
checker. Then all the checkers that rely on extent would automatically load the
modelling checker as a dependency.
2. Yeah, i think many checkers may consider extents, not just array bound;
other checkers may appear in the future. This info is very useful, which is why
we have the whole symbol class just for that (however, checker communication
through constraints on this symbol class wasn't IMHO a good idea, because it's
harder for the constraint manager to deal with symbols and constraints rather
than with concrete values).
//Just wanted to speak out, thoughts below might perhaps be more on-topic//
3. The MallocChecker is not just unix.Malloc, but also cplusplus.NewDelete,
etc., which makes a lot more sense to leave it here.
4. Consider another part of MallocChecker's job - modeling the memory spaces
for symbolic regions (heap vs. unknown). For malloc(), this is done in the
checker. For operator-new, it is done in the ExprEngine(!), because this is
part of the language rather than a library function. Perhaps ExprEngine would
be the proper place for that code as well.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1010
@@ +1009,3 @@
+ SVal Size;
+ const LocationContext *LCtx = C.getPredecessor()->getLocationContext();
+ const MemRegion *Region;
`C.getLocationContext()` is a bit shorter.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1017
@@ +1016,3 @@
+
Re: [PATCH] D24307: calculate extent size for memory regions allocated by C++ new expression
dkrupp added inline comments. Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1003 @@ +1002,3 @@ +// +ProgramStateRef MallocChecker::addExtentSize(CheckerContext , + const CXXNewExpr *NE, xazax.hun wrote: > zaks.anna wrote: > > I am not sure this code belongs to the malloc checker since it only > > supports the array bounds checker. Is there a reason it's not part of that > > checker? > I think it is part of the malloc checker because it already does something > very very similar to malloc, see the MallocMemAux function. So in fact, for > the array bounds checker to work properly, the malloc checker should be > turned on. Extent size is used by ArrayBoundChecker, ArrayBoundCheckerV2 and CStringChecker checkers currently. New expression in case of simple allocations (0 allocation) was already handled in Malloc checker , that's why I implemented it there. But I agree it feels odd that one has to switch on unix.Malloc checker to get the size of new allocated heap regions. Should I move this to ArrayBoundChecker or ArrayBoundCheckerV2? https://reviews.llvm.org/D24307 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24307: calculate extent size for memory regions allocated by C++ new expression
xazax.hun added inline comments. Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1003 @@ +1002,3 @@ +// +ProgramStateRef MallocChecker::addExtentSize(CheckerContext , + const CXXNewExpr *NE, zaks.anna wrote: > I am not sure this code belongs to the malloc checker since it only supports > the array bounds checker. Is there a reason it's not part of that checker? I think it is part of the malloc checker because it already does something very very similar to malloc, see the MallocMemAux function. So in fact, for the array bounds checker to work properly, the malloc checker should be turned on. https://reviews.llvm.org/D24307 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24307: calculate extent size for memory regions allocated by C++ new expression
zaks.anna added inline comments. Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1003 @@ +1002,3 @@ +// +ProgramStateRef MallocChecker::addExtentSize(CheckerContext , + const CXXNewExpr *NE, I am not sure this code belongs to the malloc checker since it only supports the array bounds checker. Is there a reason it's not part of that checker? https://reviews.llvm.org/D24307 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
