[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Gábor Horváth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG5882e6f36fd9: [analyzer] Escape symbols conjured into 
specific regions during a conservative… (authored by xazax.hun).

Changed prior to commit:
  https://reviews.llvm.org/D71224?vs=233235=233431#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h
  clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/expr-inspection.c
  clang/test/Analysis/pointer-escape-on-conservative-calls.c

Index: clang/test/Analysis/pointer-escape-on-conservative-calls.c
===
--- /dev/null
+++ clang/test/Analysis/pointer-escape-on-conservative-calls.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=debug.AnalysisOrder -analyzer-config debug.AnalysisOrder:PointerEscape=true -analyzer-config debug.AnalysisOrder:PostCall=true %s 2>&1 | FileCheck %s
+
+
+void f(int *);
+int *getMem();
+
+int main() {
+f(getMem());
+return 0;
+}
+
+// CHECK: PostCall (f)
+// CHECK-NEXT: PointerEscape
Index: clang/test/Analysis/expr-inspection.c
===
--- clang/test/Analysis/expr-inspection.c
+++ clang/test/Analysis/expr-inspection.c
@@ -50,5 +50,5 @@
 
 void test_field_dumps(struct S s, struct S *p) {
   clang_analyzer_dump_pointer(); // expected-warning{{}}
-  clang_analyzer_dump_pointer(>x); // expected-warning{{{reg_$0}.x}}
+  clang_analyzer_dump_pointer(>x); // expected-warning{{{reg_$1}.x}}
 }
Index: clang/test/Analysis/analyzer-config.c
===
--- clang/test/Analysis/analyzer-config.c
+++ clang/test/Analysis/analyzer-config.c
@@ -37,6 +37,7 @@
 // CHECK-NEXT: debug.AnalysisOrder:EndFunction = false
 // CHECK-NEXT: debug.AnalysisOrder:LiveSymbols = false
 // CHECK-NEXT: debug.AnalysisOrder:NewAllocator = false
+// CHECK-NEXT: debug.AnalysisOrder:PointerEscape = false
 // CHECK-NEXT: debug.AnalysisOrder:PostCall = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtArraySubscriptExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtCXXNewExpr = false
@@ -97,4 +98,4 @@
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
 // CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 94
+// CHECK-NEXT: num-entries = 95
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -10,6 +10,7 @@
 //
 //===--===//
 
+#include "clang/AST/Decl.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "PrettyStackTraceLocationContext.h"
 #include "clang/AST/CXXInheritance.h"
@@ -592,9 +593,45 @@
   for (auto I : dstCallEvaluated)
 finishArgumentConstruction(dstArgumentCleanup, I, Call);
 
-  // Finally, run any post-call checks.
-  getCheckerManager().runCheckersForPostCall(Dst, dstArgumentCleanup,
+  ExplodedNodeSet dstPostCall;
+  getCheckerManager().runCheckersForPostCall(dstPostCall, dstArgumentCleanup,
  Call, *this);
+
+  // Escaping symbols conjured during invalidating the regions above.
+  // Note that, for inlined calls the nodes were put back into the worklist,
+  // so we can assume that every node belongs to a conservative call at this
+  // point.
+
+  // Run pointerEscape callback with the newly conjured symbols.
+  SmallVector, 8> Escaped;
+  for (auto I : dstPostCall) {
+NodeBuilder B(I, Dst, *currBldrCtx);
+ProgramStateRef State = I->getState();
+Escaped.clear();
+{
+  unsigned Arg = -1;
+  for (const ParmVarDecl *PVD : Call.parameters()) {
+++Arg;
+QualType ParamTy = PVD->getType();
+if (ParamTy.isNull() ||
+(!ParamTy->isPointerType() && !ParamTy->isReferenceType()))
+  continue;
+QualType Pointee = ParamTy->getPointeeType();
+if (Pointee.isConstQualified() || Pointee->isVoidType())
+  continue;
+if (const MemRegion *MR = Call.getArgSVal(Arg).getAsRegion())
+  Escaped.emplace_back(loc::MemRegionVal(MR), State->getSVal(MR, Pointee));
+  }
+}
+
+State = processPointerEscapedOnBind(State, Escaped, I->getLocationContext(),

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Thanks!! Nothing controversial remains here, right? :)




Comment at: clang/include/clang/StaticAnalyzer/Core/CheckerManager.h:91
+  /// that the analyzer cannot follow during a conservative call.
+  PSK_EscapeOnConservativeCall,
+

All three escape-on-calls are on conservatively evaluated calls only, so this 
name isn't very descriptive.

Maybe `PSK_EscapeOutParameters`?



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:2736
 SVal Val, const LocationContext *LCtx) 
{
-
-  // Cases (1) and (2).
-  const MemRegion *MR = Loc.getAsRegion();
-  if (!MR || !MR->hasStackStorage())
-return escapeValue(State, Val, PSK_EscapeOnBind);
-
-  // Case (3).
-  if (const auto *VR = dyn_cast(MR->getBaseRegion()))
-if (VR->hasStackParametersStorage() && VR->getStackFrame()->inTopFrame())
-  if (const auto *RD = VR->getValueType()->getAsCXXRecordDecl())
-if (!RD->hasTrivialDestructor())
-  return escapeValue(State, Val, PSK_EscapeOnBind);
-
-  // Case (4): in order to test that, generate a new state with the binding
-  // added. If it is the same state, then it escapes (since the store cannot
-  // represent the binding).
-  // Do this only if we know that the store is not supposed to generate the
-  // same state.
-  SVal StoredVal = State->getSVal(MR);
-  if (StoredVal != Val)
-if (State == (State->bindLoc(loc::MemRegionVal(MR), Val, LCtx)))
-  return escapeValue(State, Val, PSK_EscapeOnBind);
-
-  return State;
+  std::pair LocAndVal(Loc, Val);
+  return processPointerEscapedOnBind(State, LocAndVal, LCtx, PSK_EscapeOnBind,

Dunno, `make_pair`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 233235.
xazax.hun added a comment.

- Rebasing after reverting the pre-escaping.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h
  clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/expr-inspection.c
  clang/test/Analysis/pointer-escape-on-conservative-calls.c

Index: clang/test/Analysis/pointer-escape-on-conservative-calls.c
===
--- /dev/null
+++ clang/test/Analysis/pointer-escape-on-conservative-calls.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=debug.AnalysisOrder -analyzer-config debug.AnalysisOrder:PointerEscape=true -analyzer-config debug.AnalysisOrder:PostCall=true %s 2>&1 | FileCheck %s
+
+
+void f(int *);
+int *getMem();
+
+int main() {
+f(getMem());
+return 0;
+}
+
+// CHECK: PostCall (f)
+// CHECK-NEXT: PointerEscape
Index: clang/test/Analysis/expr-inspection.c
===
--- clang/test/Analysis/expr-inspection.c
+++ clang/test/Analysis/expr-inspection.c
@@ -50,5 +50,5 @@
 
 void test_field_dumps(struct S s, struct S *p) {
   clang_analyzer_dump_pointer(); // expected-warning{{}}
-  clang_analyzer_dump_pointer(>x); // expected-warning{{{reg_$0}.x}}
+  clang_analyzer_dump_pointer(>x); // expected-warning{{{reg_$1}.x}}
 }
Index: clang/test/Analysis/analyzer-config.c
===
--- clang/test/Analysis/analyzer-config.c
+++ clang/test/Analysis/analyzer-config.c
@@ -37,6 +37,7 @@
 // CHECK-NEXT: debug.AnalysisOrder:EndFunction = false
 // CHECK-NEXT: debug.AnalysisOrder:LiveSymbols = false
 // CHECK-NEXT: debug.AnalysisOrder:NewAllocator = false
+// CHECK-NEXT: debug.AnalysisOrder:PointerEscape = false
 // CHECK-NEXT: debug.AnalysisOrder:PostCall = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtArraySubscriptExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtCXXNewExpr = false
@@ -97,4 +98,4 @@
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
 // CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 94
+// CHECK-NEXT: num-entries = 95
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -10,6 +10,7 @@
 //
 //===--===//
 
+#include "clang/AST/Decl.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "PrettyStackTraceLocationContext.h"
 #include "clang/AST/CXXInheritance.h"
@@ -592,9 +593,45 @@
   for (auto I : dstCallEvaluated)
 finishArgumentConstruction(dstArgumentCleanup, I, Call);
 
-  // Finally, run any post-call checks.
-  getCheckerManager().runCheckersForPostCall(Dst, dstArgumentCleanup,
+  ExplodedNodeSet dstPostCall;
+  getCheckerManager().runCheckersForPostCall(dstPostCall, dstArgumentCleanup,
  Call, *this);
+
+  // Escaping symbols conjured during invalidating the regions above.
+  // Note that, for inlined calls the nodes were put back into the worklist,
+  // so we can assume that every node belongs to a conservative call at this
+  // point.
+
+  // Run pointerEscape callback with the newly conjured symbols.
+  SmallVector, 8> Escaped;
+  for (auto I : dstPostCall) {
+NodeBuilder B(I, Dst, *currBldrCtx);
+ProgramStateRef State = I->getState();
+Escaped.clear();
+{
+  unsigned Arg = -1;
+  for (const ParmVarDecl *PVD : Call.parameters()) {
+++Arg;
+QualType ParamTy = PVD->getType();
+if (ParamTy.isNull() ||
+(!ParamTy->isPointerType() && !ParamTy->isReferenceType()))
+  continue;
+QualType Pointee = ParamTy->getPointeeType();
+if (Pointee.isConstQualified() || Pointee->isVoidType())
+  continue;
+if (const MemRegion *MR = Call.getArgSVal(Arg).getAsRegion())
+  Escaped.emplace_back(loc::MemRegionVal(MR), State->getSVal(MR, Pointee));
+  }
+}
+
+State = processPointerEscapedOnBind(State, Escaped, I->getLocationContext(),
+PSK_EscapeOnConservativeCall, );
+
+if (State == I->getState())
+  Dst.insert(I);
+else
+  B.generateNode(I->getLocation(), State, I);
+  }
 }
 
 

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D71224#1778332 , @xazax.hun wrote:

> So basically what I am wonder/worrying about is the following:
>  The analyzer core will decide that the stack region is escaped and the 
> checkers has no word about this.


Yup, you got me. Pre-escaped locals are indeed material and beyond the 
checker's control. I don't seem to have any immediate solutions. I think we 
could postpone the work on pre-escaped locals (until we figure out how to do 
them correctly) if they're not immediately necessary to you (after all i was 
the one who suggested it). Or ignore the problem (depending on how we do our FP 
vs. FN trade-off).

In D71224#1778357 , @xazax.hun wrote:

> Consider the following two snippets:


Mm, these snippets don't have pre-escaped locals. Like, they accidentally do, 
but above i proposed to work around this by removing the first escape 
invocation (that happens during the call) and only doing it after the call. 
This way these locals don't have time to become pre-escaped. I think these are 
not a problem.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 233200.
xazax.hun added a comment.

- First take on trying to reuse `processPointerEscapedOnBind`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h
  clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/ponter-escape-on-conservative-calls.c

Index: clang/test/Analysis/ponter-escape-on-conservative-calls.c
===
--- /dev/null
+++ clang/test/Analysis/ponter-escape-on-conservative-calls.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=debug.AnalysisOrder -analyzer-config debug.AnalysisOrder:PointerEscape=true -analyzer-config debug.AnalysisOrder:PostCall=true %s 2>&1 | FileCheck %s
+
+
+void f(int *);
+
+int main() {
+int a;
+f();
+return 0;
+}
+
+// CHECK: PostCall
+// CHECK-NEXT: PointerEscape
Index: clang/test/Analysis/analyzer-config.c
===
--- clang/test/Analysis/analyzer-config.c
+++ clang/test/Analysis/analyzer-config.c
@@ -37,6 +37,7 @@
 // CHECK-NEXT: debug.AnalysisOrder:EndFunction = false
 // CHECK-NEXT: debug.AnalysisOrder:LiveSymbols = false
 // CHECK-NEXT: debug.AnalysisOrder:NewAllocator = false
+// CHECK-NEXT: debug.AnalysisOrder:PointerEscape = false
 // CHECK-NEXT: debug.AnalysisOrder:PostCall = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtArraySubscriptExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtCXXNewExpr = false
@@ -97,4 +98,4 @@
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
 // CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 94
+// CHECK-NEXT: num-entries = 95
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -10,6 +10,7 @@
 //
 //===--===//
 
+#include "clang/AST/Decl.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "PrettyStackTraceLocationContext.h"
 #include "clang/AST/CXXInheritance.h"
@@ -592,9 +593,45 @@
   for (auto I : dstCallEvaluated)
 finishArgumentConstruction(dstArgumentCleanup, I, Call);
 
-  // Finally, run any post-call checks.
-  getCheckerManager().runCheckersForPostCall(Dst, dstArgumentCleanup,
+  ExplodedNodeSet dstPostCall;
+  getCheckerManager().runCheckersForPostCall(dstPostCall, dstArgumentCleanup,
  Call, *this);
+
+  // Escaping symbols conjured during invalidating the regions above.
+  // Note that, for inlined calls the nodes were put back into the worklist,
+  // so we can assume that every node belongs to a conservative call at this
+  // point.
+
+  // Run pointerEscape callback with the newly conjured symbols.
+  SmallVector, 8> Escaped;
+  for (auto I : dstPostCall) {
+NodeBuilder B(I, Dst, *currBldrCtx);
+ProgramStateRef State = I->getState();
+Escaped.clear();
+{
+  unsigned Arg = -1;
+  for (const ParmVarDecl *PVD : Call.parameters()) {
+++Arg;
+QualType ParamTy = PVD->getType();
+if (ParamTy.isNull() ||
+(!ParamTy->isPointerType() && !ParamTy->isReferenceType()))
+  continue;
+QualType Pointee = ParamTy->getPointeeType();
+if (Pointee.isConstQualified() || Pointee->isVoidType())
+  continue;
+if (const MemRegion *MR = Call.getArgSVal(Arg).getAsRegion())
+  Escaped.emplace_back(loc::MemRegionVal(MR), State->getSVal(MR, Pointee));
+  }
+}
+
+State = processPointerEscapedOnBind(State, Escaped, I->getLocationContext(),
+PSK_EscapeOnConservativeCall, );
+
+if (State == I->getState())
+  Dst.insert(I);
+else
+  B.generateNode(I->getLocation(), State, I);
+  }
 }
 
 ProgramStateRef ExprEngine::bindReturnValue(const CallEvent ,
@@ -670,8 +707,7 @@
 // Conservatively evaluate call by invalidating regions and binding
 // a conjured return value.
 void ExprEngine::conservativeEvalCall(const CallEvent , NodeBuilder ,
-  ExplodedNode *Pred,
-  ProgramStateRef State) {
+  ExplodedNode *Pred, ProgramStateRef State) {
   State = 

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

I think I found the main problem with the current model, at least for the 
FuchsiaHandleCheck.

Consider the following two snippets:

  zx_handle_t *get_handle_address();
  void escape_store_to_escaped_region01() {
zx_handle_t sb;
if (zx_channel_create(0, get_handle_address(), ))
  return;
zx_handle_close(sb);
  }



  void leak() {
zx_handle_t sa, sb;
if (zx_channel_create(0, , ))
  return;
zx_handle_close(sb);
  }

In the first one I want the first handle to be escaped in the second one I do 
not want it to be escaped.

With my current proposed changes the checker will receive a pointer escape 
callback for both but it does not have enough info to differentiate between the 
two cases.

If I do not act upon this kind of escape I end up reporting a false positive in 
the first case. If I act on this escape I end up missing a true positive in the 
second case.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In D71224#1778303 , @NoQ wrote:

> In D71224#1778284 , @xazax.hun wrote:
>
> > So I was wondering if we got the default right. Maybe a checker should do 
> > more work to get the escaping rather than more work preventing it?
>
>
> But that's exactly how it works right now(?) If you don't define 
> `checkPointerEscape` you get no escaping, if you do extra work of defining it 
> you get the exact amount of escaping that you want.


So basically what I am wonder/worrying about is the following:
The analyzer core will decide that the stack region is escaped and the checkers 
has no word about this. And from that time on the checkers have to do extra 
work each time there is a store or conservative call to find out if this escape 
corresponds to a region that was escaped earlier unwillingly (from the 
checker's point of view).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D71224#1778284 , @xazax.hun wrote:

> So I was wondering if we got the default right. Maybe a checker should do 
> more work to get the escaping rather than more work preventing it?


But that's exactly how it works right now(?) If you don't define 
`checkPointerEscape` you get no escaping, if you do extra work of defining it 
you get the exact amount of escaping that you want.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In D71224#1778231 , @NoQ wrote:

> In D71224#1778204 , @xazax.hun wrote:
>
> > I don't think this is a good enough model currently. The problem is that, 
> > it does not play well with annotations. E.g. the checker can see a symbol 
> > escaping, but it does not have a whole lot of information how. For example, 
> > currently, there is no way to check if the output parameter through which 
> > the escape happened was annotated somehow.
>
>
> Hmm. If the function is annotated, it is hopefully "fully" annotated, or at 
> least the programmer doesn't mind adding more annotations to it. Given that 
> you have your `CallEvent` structure in `checkPointerEscape`, i hope you can 
> easily see if there are any annotations at all on the function, and if so, 
> suppress the current escape entirely. Or at least scan the annotated 
> parameters and suppress the escape for them.
>
> I guess it's still a problem if the *same* handle is also passed through a 
> parameter that *cannot* be annotated (eg., as part of a structure passed into 
> the call) and then actually getting released inside the call, but is it a 
> real problem for you?


Yeah, this was one of my idea as well. I think one of my main concerns is that 
I would except the majority of the escapes are simply being output parameters 
and only a minority are legitimate. So I was wondering if we got the default 
right. Maybe a checker should do more work to get the escaping rather than more 
work preventing it?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D71224#1778204 , @xazax.hun wrote:

> I don't think this is a good enough model currently. The problem is that, it 
> does not play well with annotations. E.g. the checker can see a symbol 
> escaping, but it does not have a whole lot of information how. For example, 
> currently, there is no way to check if the output parameter through which the 
> escape happened was annotated somehow.


Hmm. If the function is annotated, it is hopefully "fully" annotated, or at 
least the programmer doesn't mind adding more annotations to it. Given that you 
have your `CallEvent` structure in `checkPointerEscape`, i hope you can easily 
see if there are any annotations at all on the function, and if so, suppress 
the current escape entirely. Or at least scan the annotated parameters and 
suppress the escape for them.

I guess it's still a problem if the *same* handle is also passed through a 
parameter that *cannot* be annotated (eg., as part of a structure passed into 
the call) and then actually getting released inside the call, but is it a real 
problem for you?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In D71224#1778179 , @NoQ wrote:

> In any case, every checker is allowed to make their own decisions about 
> escaping. Escape on its own is not material, it's all about how the checker 
> reacts to escapes. Say, it's up to MallocChecker to decide whether the 
> function may or may not release memory that escapes on call.
>
> I think a valid approach would be to simply look up the function in your 
> `CallDescriptionMap` and then abort the `checkPointerEscape` callback when 
> it's found.
>
> Yet, it annoys me a bit that we didn't make everything magically work in an 
> "out of the box" manner. Can we eliminate the first pointer escape (that 
> happens before PostCall) but only keep the secondary escape?


I don't think this is a good enough model currently. The problem is that, it 
does not play well with annotations. E.g. the checker can see a symbol 
escaping, but it does not have a whole lot of information how. For example, 
currently, there is no way to check if the output parameter through which the 
escape happened was annotated somehow.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In any case, every checker is allowed to make their own decisions about 
escaping. Escape on its own is not material, it's all about how the checker 
reacts to escapes. Say, it's up to MallocChecker to decide whether the function 
may or may not release memory that escapes on call.

I think a valid approach would be to simply look up the function in your 
`CallDescriptionMap` and then abort the `checkPointerEscape` callback when it's 
found.

Yet, it annoys me a bit that we didn't make everything magically work in an 
"out of the box" manner. Can we eliminate the first pointer escape (that 
happens before PostCall) but only keep the secondary escape?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:624
+if (const MemRegion *MR = Call.getArgSVal(Arg).getAsRegion())
+  if (!MR->hasStackStorage())
+Escaped.push_back(State->getSVal(MR, Pointee));

xazax.hun wrote:
> xazax.hun wrote:
> > NoQ wrote:
> > > Ok, so this patch interacts with D71152 in a non-trivial manner. We 
> > > should re-use the logic that decides whether an escape on bind occurs.
> > Yeah, I just started to look into it and some skeletons fell out. 
> > 
> > Let's consider the following example:
> > ```
> > void leak() {
> >   zx_handle_t sa, sb;
> >   if (zx_channel_create(0, , ))
> > return;
> >   zx_handle_close(sb);
> > } 
> > ```
> > 
> > After using the same logic we can no longer detect the leak. The reason is, 
> > after `zx_channel_create` both `sa` and `sb` regions are escaped (and they 
> > become escaped during the conservative call) and then after the PostCall 
> > callback, when we attached the traits to our symbols, we will trigger 
> > pointer escape immediately, and lose the traits we just added.
> > 
> > I am not immediately sure what to do here. I feel like the main source of 
> > the problem is the fact that `zx_channel_create` should not escape anything 
> > and the checker has now way to communicate that to the analyzer core. Any 
> > ideas?
> Suddenly, I see four ways forward, none of which is optimal. Hopefully, you 
> can come up with something better.
> 1. Do not care about escaped local regions in this code path. So basically we 
> would tune the invalidation down a bit. 
> 2. Make it possible for modeling checkers to prevent escaping. It would be 
> great if there would be a way to do that without doing EvalCall.
> 3. Require the users to annotate out parameters. And we could assume that out 
> parameters do not escape.
> 4. Do some heuristics like if a checker just attached some info in a 
> PostCall, it is likely that we should not escape that symbol.
Or:
5. Since output arguments might be more common than escapes in practice, maybe 
we could just not escape local regions at all by default only if there is a 
special annotation or a modelling check asks for it. So users would get one 
more tool to suppress false positives but we would not overdo invalidation by 
default.  


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:624
+if (const MemRegion *MR = Call.getArgSVal(Arg).getAsRegion())
+  if (!MR->hasStackStorage())
+Escaped.push_back(State->getSVal(MR, Pointee));

xazax.hun wrote:
> NoQ wrote:
> > Ok, so this patch interacts with D71152 in a non-trivial manner. We should 
> > re-use the logic that decides whether an escape on bind occurs.
> Yeah, I just started to look into it and some skeletons fell out. 
> 
> Let's consider the following example:
> ```
> void leak() {
>   zx_handle_t sa, sb;
>   if (zx_channel_create(0, , ))
> return;
>   zx_handle_close(sb);
> } 
> ```
> 
> After using the same logic we can no longer detect the leak. The reason is, 
> after `zx_channel_create` both `sa` and `sb` regions are escaped (and they 
> become escaped during the conservative call) and then after the PostCall 
> callback, when we attached the traits to our symbols, we will trigger pointer 
> escape immediately, and lose the traits we just added.
> 
> I am not immediately sure what to do here. I feel like the main source of the 
> problem is the fact that `zx_channel_create` should not escape anything and 
> the checker has now way to communicate that to the analyzer core. Any ideas?
Suddenly, I see four ways forward, none of which is optimal. Hopefully, you can 
come up with something better.
1. Do not care about escaped local regions in this code path. So basically we 
would tune the invalidation down a bit. 
2. Make it possible for modeling checkers to prevent escaping. It would be 
great if there would be a way to do that without doing EvalCall.
3. Require the users to annotate out parameters. And we could assume that out 
parameters do not escape.
4. Do some heuristics like if a checker just attached some info in a PostCall, 
it is likely that we should not escape that symbol.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:624
+if (const MemRegion *MR = Call.getArgSVal(Arg).getAsRegion())
+  if (!MR->hasStackStorage())
+Escaped.push_back(State->getSVal(MR, Pointee));

NoQ wrote:
> Ok, so this patch interacts with D71152 in a non-trivial manner. We should 
> re-use the logic that decides whether an escape on bind occurs.
Yeah, I just started to look into it and some skeletons fell out. 

Let's consider the following example:
```
void leak() {
  zx_handle_t sa, sb;
  if (zx_channel_create(0, , ))
return;
  zx_handle_close(sb);
} 
```

After using the same logic we can no longer detect the leak. The reason is, 
after `zx_channel_create` both `sa` and `sb` regions are escaped (and they 
become escaped during the conservative call) and then after the PostCall 
callback, when we attached the traits to our symbols, we will trigger pointer 
escape immediately, and lose the traits we just added.

I am not immediately sure what to do here. I feel like the main source of the 
problem is the fact that `zx_channel_create` should not escape anything and the 
checker has now way to communicate that to the analyzer core. Any ideas?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:632
+  /// of some values.
+  ProgramStateRef escapeValue(ProgramStateRef State, ArrayRef Vs,
   PointerEscapeKind K) const;

Dunno, should we rename to `escapeValues()`?



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:614-616
+if (Arg >= FuncDecl->getNumParams())
+  break;
+QualType ParamTy = FuncDecl->getParamDecl(Arg)->getType();

I encourage `Call.parameters()`. This way you won't need to obtain a 
`FuncDecl`. In fact you won't even need it to be a `FunctionDecl`.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:624
+if (const MemRegion *MR = Call.getArgSVal(Arg).getAsRegion())
+  if (!MR->hasStackStorage())
+Escaped.push_back(State->getSVal(MR, Pointee));

Ok, so this patch interacts with D71152 in a non-trivial manner. We should 
re-use the logic that decides whether an escape on bind occurs.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 232982.
xazax.hun added a comment.

- Reuse `ExprEngine::escapeValue`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/ponter-escape-on-conservative-calls.c

Index: clang/test/Analysis/ponter-escape-on-conservative-calls.c
===
--- /dev/null
+++ clang/test/Analysis/ponter-escape-on-conservative-calls.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=debug.AnalysisOrder -analyzer-config debug.AnalysisOrder:PointerEscape=true -analyzer-config debug.AnalysisOrder:PostCall=true %s 2>&1 | FileCheck %s
+
+
+void f(int *);
+
+int main() {
+int a;
+f();
+return 0;
+}
+
+// CHECK: PostCall
+// CHECK-NEXT: PointerEscape
Index: clang/test/Analysis/analyzer-config.c
===
--- clang/test/Analysis/analyzer-config.c
+++ clang/test/Analysis/analyzer-config.c
@@ -37,6 +37,7 @@
 // CHECK-NEXT: debug.AnalysisOrder:EndFunction = false
 // CHECK-NEXT: debug.AnalysisOrder:LiveSymbols = false
 // CHECK-NEXT: debug.AnalysisOrder:NewAllocator = false
+// CHECK-NEXT: debug.AnalysisOrder:PointerEscape = false
 // CHECK-NEXT: debug.AnalysisOrder:PostCall = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtArraySubscriptExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtCXXNewExpr = false
@@ -97,4 +98,4 @@
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
 // CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 94
+// CHECK-NEXT: num-entries = 95
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -592,9 +592,47 @@
   for (auto I : dstCallEvaluated)
 finishArgumentConstruction(dstArgumentCleanup, I, Call);
 
-  // Finally, run any post-call checks.
-  getCheckerManager().runCheckersForPostCall(Dst, dstArgumentCleanup,
+  ExplodedNodeSet dstPostCall;
+  getCheckerManager().runCheckersForPostCall(dstPostCall, dstArgumentCleanup,
  Call, *this);
+
+  // Escaping symbols conjured during invalidating the regions above.
+  // Note that, for inlined calls the nodes were put back into the worklist,
+  // so we can assume that every node belongs to a conservative call at this
+  // point.
+
+  // Run pointerEscape callback with the newly conjured symbols.
+  SmallVector Escaped;
+  for (auto I : dstPostCall) {
+NodeBuilder B(I, Dst, *currBldrCtx);
+ProgramStateRef State = I->getState();
+Escaped.clear();
+const FunctionDecl *FuncDecl =
+dyn_cast_or_null(Call.getDecl());
+if (FuncDecl) {
+  for (unsigned Arg = 0; Arg < Call.getNumArgs(); ++Arg) {
+if (Arg >= FuncDecl->getNumParams())
+  break;
+QualType ParamTy = FuncDecl->getParamDecl(Arg)->getType();
+if (ParamTy.isNull() ||
+(!ParamTy->isPointerType() && !ParamTy->isReferenceType()))
+  continue;
+QualType Pointee = ParamTy->getPointeeType();
+if (Pointee.isConstQualified() || Pointee->isVoidType())
+  continue;
+if (const MemRegion *MR = Call.getArgSVal(Arg).getAsRegion())
+  if (!MR->hasStackStorage())
+Escaped.push_back(State->getSVal(MR, Pointee));
+  }
+}
+
+State = escapeValue(State, Escaped, PSK_EscapeOnConservativeCall);
+
+if (State == I->getState())
+  Dst.insert(I);
+else
+  B.generateNode(I->getLocation(), State, I);
+  }
 }
 
 ProgramStateRef ExprEngine::bindReturnValue(const CallEvent ,
@@ -670,8 +708,7 @@
 // Conservatively evaluate call by invalidating regions and binding
 // a conjured return value.
 void ExprEngine::conservativeEvalCall(const CallEvent , NodeBuilder ,
-  ExplodedNode *Pred,
-  ProgramStateRef State) {
+  ExplodedNode *Pred, ProgramStateRef State) {
   State = Call.invalidateRegions(currBldrCtx->blockCount(), State);
   State = bindReturnValue(Call, Pred->getLocationContext(), State);
 
Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1172,13 +1172,15 @@
   }
 }
 
-ProgramStateRef 

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:603
+  // point.
+  class CollectReachableSymbolsCallback final : public SymbolVisitor {
+ProgramStateRef State;

NoQ wrote:
> xazax.hun wrote:
> > NoQ wrote:
> > > xazax.hun wrote:
> > > > NoQ wrote:
> > > > > WDYT about re-using `ExprEngine::escapeValue()` by changing it to 
> > > > > accept `ArrayRef` instead of a single `SVal`?
> > > > It is not entirely the same. Here we only collect symbols from 
> > > > non-stack regions. There (as far as I understand) we collect all 
> > > > symbols. I could add a flag but at that point I wonder if it is worth 
> > > > the change.
> > > Umm, wait, right, so what are you doing in this callback to begin with? 
> > > The code says "gather all symbols that are encountered as immediate 
> > > values stored in traversed regions". Why not simply "gather all symbols 
> > > that are traversed from parameter regions"?
> > My understanding is the following but correct me if I am wrong:
> > 
> > ```
> > int *getConjuredSymbol();
> > 
> > call(getConjuredSymbol());
> > ```
> > 
> > So we have can talk about two symbols here. One symbol is what was returned 
> > by `getConjuredSymbol` and the other is the pointee, the symbol that we get 
> > when we dereference the result of `getConjuredSymbol`. And my understanding 
> > is that we want to invoke escape for the latter and not the former. 
> > `ExprEngine::escapeValue` invalidates the former but not the latter. The 
> > visitor here invalidates the latter but not the former.  This behavior 
> > solves most of my test cases in FuchsiaHandleChecker. 
> How about `escapeValue(getSVal(getArgSVal()))`? (the type for `getSVal()` 
> could be obtained from the AST).
Hmm, I believe that could work, thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:603
+  // point.
+  class CollectReachableSymbolsCallback final : public SymbolVisitor {
+ProgramStateRef State;

xazax.hun wrote:
> NoQ wrote:
> > xazax.hun wrote:
> > > NoQ wrote:
> > > > WDYT about re-using `ExprEngine::escapeValue()` by changing it to 
> > > > accept `ArrayRef` instead of a single `SVal`?
> > > It is not entirely the same. Here we only collect symbols from non-stack 
> > > regions. There (as far as I understand) we collect all symbols. I could 
> > > add a flag but at that point I wonder if it is worth the change.
> > Umm, wait, right, so what are you doing in this callback to begin with? The 
> > code says "gather all symbols that are encountered as immediate values 
> > stored in traversed regions". Why not simply "gather all symbols that are 
> > traversed from parameter regions"?
> My understanding is the following but correct me if I am wrong:
> 
> ```
> int *getConjuredSymbol();
> 
> call(getConjuredSymbol());
> ```
> 
> So we have can talk about two symbols here. One symbol is what was returned 
> by `getConjuredSymbol` and the other is the pointee, the symbol that we get 
> when we dereference the result of `getConjuredSymbol`. And my understanding 
> is that we want to invoke escape for the latter and not the former. 
> `ExprEngine::escapeValue` invalidates the former but not the latter. The 
> visitor here invalidates the latter but not the former.  This behavior solves 
> most of my test cases in FuchsiaHandleChecker. 
How about `escapeValue(getSVal(getArgSVal()))`? (the type for `getSVal()` could 
be obtained from the AST).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:648
+  continue;
+State->scanReachableSymbols(Call.getArgSVal(Arg), Scanner);
+  }

NoQ wrote:
> I guess technically, for our own sanity, it's worth it to-rescan the symbols 
> for every node in `dstPostCall`. But i'll be very surprised if they are 
> //actually// going to yield different results for every predecessor node.
I think it is possible. We use the state to get the pointee of some pointers, 
so in case the PostCall splits the state on the value of the outputs it would 
be reasonable to get different results. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:603
+  // point.
+  class CollectReachableSymbolsCallback final : public SymbolVisitor {
+ProgramStateRef State;

NoQ wrote:
> xazax.hun wrote:
> > NoQ wrote:
> > > WDYT about re-using `ExprEngine::escapeValue()` by changing it to accept 
> > > `ArrayRef` instead of a single `SVal`?
> > It is not entirely the same. Here we only collect symbols from non-stack 
> > regions. There (as far as I understand) we collect all symbols. I could add 
> > a flag but at that point I wonder if it is worth the change.
> Umm, wait, right, so what are you doing in this callback to begin with? The 
> code says "gather all symbols that are encountered as immediate values stored 
> in traversed regions". Why not simply "gather all symbols that are traversed 
> from parameter regions"?
My understanding is the following but correct me if I am wrong:

```
int *getConjuredSymbol();

call(getConjuredSymbol());
```

So we have can talk about two symbols here. One symbol is what was returned by 
`getConjuredSymbol` and the other is the pointee, the symbol that we get when 
we dereference the result of `getConjuredSymbol`. And my understanding is that 
we want to invoke escape for the latter and not the former. 
`ExprEngine::escapeValue` invalidates the former but not the latter. The 
visitor here invalidates the latter but not the former.  This behavior solves 
most of my test cases in FuchsiaHandleChecker. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:603
+  // point.
+  class CollectReachableSymbolsCallback final : public SymbolVisitor {
+ProgramStateRef State;

xazax.hun wrote:
> NoQ wrote:
> > WDYT about re-using `ExprEngine::escapeValue()` by changing it to accept 
> > `ArrayRef` instead of a single `SVal`?
> It is not entirely the same. Here we only collect symbols from non-stack 
> regions. There (as far as I understand) we collect all symbols. I could add a 
> flag but at that point I wonder if it is worth the change.
Umm, wait, right, so what are you doing in this callback to begin with? The 
code says "gather all symbols that are encountered as immediate values stored 
in traversed regions". Why not simply "gather all symbols that are traversed 
from parameter regions"?



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:648
+  continue;
+State->scanReachableSymbols(Call.getArgSVal(Arg), Scanner);
+  }

I guess technically, for our own sanity, it's worth it to-rescan the symbols 
for every node in `dstPostCall`. But i'll be very surprised if they are 
//actually// going to yield different results for every predecessor node.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:603
+  // point.
+  class CollectReachableSymbolsCallback final : public SymbolVisitor {
+ProgramStateRef State;

NoQ wrote:
> WDYT about re-using `ExprEngine::escapeValue()` by changing it to accept 
> `ArrayRef` instead of a single `SVal`?
It is not entirely the same. Here we only collect symbols from non-stack 
regions. There (as far as I understand) we collect all symbols. I could add a 
flag but at that point I wonder if it is worth the change.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:603
+  // point.
+  class CollectReachableSymbolsCallback final : public SymbolVisitor {
+ProgramStateRef State;

WDYT about re-using `ExprEngine::escapeValue()` by changing it to accept 
`ArrayRef` instead of a single `SVal`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 232968.
xazax.hun edited the summary of this revision.
xazax.hun added a comment.

- Added a test. More rigorous tests will come in  the FuchsiaHandleChecker.
- Added a new PSK_ entry.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
  clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/ponter-escape-on-conservative-calls.c

Index: clang/test/Analysis/ponter-escape-on-conservative-calls.c
===
--- /dev/null
+++ clang/test/Analysis/ponter-escape-on-conservative-calls.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=debug.AnalysisOrder -analyzer-config debug.AnalysisOrder:PointerEscape=true -analyzer-config debug.AnalysisOrder:PostCall=true %s 2>&1 | FileCheck %s
+
+
+void f(int *);
+
+int main() {
+int a;
+f();
+return 0;
+}
+
+// CHECK: PostCall
+// CHECK-NEXT: PointerEscape
Index: clang/test/Analysis/analyzer-config.c
===
--- clang/test/Analysis/analyzer-config.c
+++ clang/test/Analysis/analyzer-config.c
@@ -37,6 +37,7 @@
 // CHECK-NEXT: debug.AnalysisOrder:EndFunction = false
 // CHECK-NEXT: debug.AnalysisOrder:LiveSymbols = false
 // CHECK-NEXT: debug.AnalysisOrder:NewAllocator = false
+// CHECK-NEXT: debug.AnalysisOrder:PointerEscape = false
 // CHECK-NEXT: debug.AnalysisOrder:PostCall = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtArraySubscriptExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtCXXNewExpr = false
@@ -97,4 +98,4 @@
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
 // CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 94
+// CHECK-NEXT: num-entries = 95
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -592,9 +592,71 @@
   for (auto I : dstCallEvaluated)
 finishArgumentConstruction(dstArgumentCleanup, I, Call);
 
-  // Finally, run any post-call checks.
-  getCheckerManager().runCheckersForPostCall(Dst, dstArgumentCleanup,
+  ExplodedNodeSet dstPostCall;
+  getCheckerManager().runCheckersForPostCall(dstPostCall, dstArgumentCleanup,
  Call, *this);
+
+  // Escaping symbols conjured during invalidationg the regions above.
+  // Note that, for inlined calls the nodes were put back into the worklist,
+  // so we can assume that every node belongs to a conservative call at this
+  // point.
+  class CollectReachableSymbolsCallback final : public SymbolVisitor {
+ProgramStateRef State;
+InvalidatedSymbols 
+
+  public:
+explicit CollectReachableSymbolsCallback(ProgramStateRef State,
+ InvalidatedSymbols )
+: State(State), Symbols(Symbols) {}
+
+bool VisitSymbol(SymbolRef Sym) override { return true; }
+bool VisitMemRegion(const MemRegion *MR) override {
+  if (MR->hasStackStorage())
+return false;
+  QualType T;
+  if (const TypedRegion *TR = dyn_cast(MR))
+T = TR->getLocationType();
+  else if (const SymbolicRegion *SR = dyn_cast(MR))
+T = SR->getSymbol()->getType();
+  if (T->isVoidPointerType())
+return false;
+  SVal StoredVal = State->getSVal(MR);
+  if (SymbolRef Sym = StoredVal.getAsSymbol())
+Symbols.insert(Sym);
+  return true;
+}
+  };
+
+  // Run pointerEscape callback with the newly conjured symbols.
+  for (auto I : dstPostCall) {
+NodeBuilder B(I, Dst, *currBldrCtx);
+InvalidatedSymbols Symbols;
+ProgramStateRef State = I->getState();
+CollectReachableSymbolsCallback Scanner(State, Symbols);
+const FunctionDecl *FuncDecl =
+dyn_cast_or_null(Call.getDecl());
+if (FuncDecl) {
+  for (unsigned Arg = 0; Arg < Call.getNumArgs(); ++Arg) {
+if (Arg >= FuncDecl->getNumParams())
+  break;
+QualType ParamTy = FuncDecl->getParamDecl(Arg)->getType();
+if (ParamTy.isNull() ||
+(!ParamTy->isPointerType() && !ParamTy->isReferenceType()))
+  continue;
+if (ParamTy->getPointeeType().isConstQualified())
+  continue;
+State->scanReachableSymbols(Call.getArgSVal(Arg), Scanner);
+  }
+}
+
+State = getCheckerManager().runCheckersForPointerEscape(
+State, Symbols, , PSK_EscapeOnConservativeCall, nullptr);
+
+if (State == I->getState())
+  Dst.insert(I);
+else
+  B.generateNode(I->getLocation(), State, I);
+  }
 }
 
 ProgramStateRef 

[PATCH] D71224: [analyzer] Escape symbols stored into specific region after a conservative evalcall.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 232955.
xazax.hun added a comment.

- Do not track explicitly if a call was conservative.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71224/new/

https://reviews.llvm.org/D71224

Files:
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

Index: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -10,6 +10,7 @@
 //
 //===--===//
 
+#include "clang/AST/Type.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "PrettyStackTraceLocationContext.h"
 #include "clang/AST/CXXInheritance.h"
@@ -18,6 +19,8 @@
 #include "clang/Analysis/ConstructionContext.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/Store.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Support/SaveAndRestore.h"
@@ -576,15 +579,14 @@
 
   // Run any pre-call checks using the generic call interface.
   ExplodedNodeSet dstPreVisit;
-  getCheckerManager().runCheckersForPreCall(dstPreVisit, Pred,
-Call, *this);
+  getCheckerManager().runCheckersForPreCall(dstPreVisit, Pred, Call, *this);
 
   // Actually evaluate the function call.  We try each of the checkers
   // to see if the can evaluate the function call, and get a callback at
   // defaultEvalCall if all of them fail.
   ExplodedNodeSet dstCallEvaluated;
-  getCheckerManager().runCheckersForEvalCall(dstCallEvaluated, dstPreVisit,
- Call, *this);
+  getCheckerManager().runCheckersForEvalCall(
+  dstCallEvaluated, dstPreVisit, Call, *this);
 
   // If there were other constructors called for object-type arguments
   // of this call, clean them up.
@@ -592,9 +594,72 @@
   for (auto I : dstCallEvaluated)
 finishArgumentConstruction(dstArgumentCleanup, I, Call);
 
-  // Finally, run any post-call checks.
-  getCheckerManager().runCheckersForPostCall(Dst, dstArgumentCleanup,
+  ExplodedNodeSet dstPostCall;
+  getCheckerManager().runCheckersForPostCall(dstPostCall, dstArgumentCleanup,
  Call, *this);
+
+  // Escaping symbols conjured during invalidationg the regions above.
+  // Note that, for inlined calls the nodes were put back into the worklist,
+  // so we can assume that every node belongs to a conservative call at this
+  // point.
+  class CollectReachableSymbolsCallback final : public SymbolVisitor {
+ProgramStateRef State;
+InvalidatedSymbols 
+
+  public:
+explicit CollectReachableSymbolsCallback(ProgramStateRef State,
+ InvalidatedSymbols )
+: State(State), Symbols(Symbols) {}
+
+bool VisitSymbol(SymbolRef Sym) override { return true; }
+bool VisitMemRegion(const MemRegion *MR) override {
+  if (MR->hasStackStorage())
+return false;
+  QualType T;
+  if (const TypedRegion *TR = dyn_cast(MR))
+T = TR->getLocationType();
+  else if (const SymbolicRegion *SR = dyn_cast(MR))
+T = SR->getSymbol()->getType();
+  if (T->isVoidPointerType())
+return false;
+  SVal StoredVal = State->getSVal(MR);
+  if (SymbolRef Sym = StoredVal.getAsSymbol())
+Symbols.insert(Sym);
+  return true;
+}
+  };
+
+  // Run pointerEscape callback with the newly conjured symbols.
+  for (auto I : dstPostCall) {
+NodeBuilder B(I, Dst, *currBldrCtx);
+InvalidatedSymbols Symbols;
+ProgramStateRef State = I->getState();
+CollectReachableSymbolsCallback Scanner(State, Symbols);
+const FunctionDecl *FuncDecl =
+dyn_cast_or_null(Call.getDecl());
+if (FuncDecl) {
+  for (unsigned Arg = 0; Arg < Call.getNumArgs(); ++Arg) {
+if (Arg >= FuncDecl->getNumParams())
+  break;
+QualType ParamTy = FuncDecl->getParamDecl(Arg)->getType();
+if (ParamTy.isNull() ||
+(!ParamTy->isPointerType() && !ParamTy->isReferenceType()))
+  continue;
+if (ParamTy->getPointeeType().isConstQualified())
+  continue;
+State->scanReachableSymbols(Call.getArgSVal(Arg), Scanner);
+  }
+}
+
+// TODO: the PSK is a lie.
+State = getCheckerManager().runCheckersForPointerEscape(
+State, Symbols, , PSK_DirectEscapeOnCall, nullptr);
+
+if (State != I->getState())
+  B.generateNode(I->getLocation(), State, I);
+else
+  Dst.insert(I);
+  }
 }
 
 ProgramStateRef ExprEngine::bindReturnValue(const CallEvent ,
@@ -644,8 +709,8 @@
 ITraits.setTrait(TargetR,