[PATCH] D108753: [analyzer] MallocChecker: Add notes from NoOwnershipChangeVisitor only when a function "intents", but doesn't change ownership, enable by default

[Kristóf Umann via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9d359f6c7386: [analyzer] MallocChecker: Add notes from 
NoOwnershipChangeVisitor only when a… (authored by Szelethus).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108753

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/test/Analysis/NewDeleteLeaks.cpp
  clang/test/Analysis/analyzer-config.c

Index: clang/test/Analysis/analyzer-config.c
===
--- clang/test/Analysis/analyzer-config.c
+++ clang/test/Analysis/analyzer-config.c
@@ -117,7 +117,7 @@
 // CHECK-NEXT: suppress-null-return-paths = true
 // CHECK-NEXT: track-conditions = true
 // CHECK-NEXT: track-conditions-debug = false
-// CHECK-NEXT: unix.DynamicMemoryModeling:AddNoOwnershipChangeNotes = false
+// CHECK-NEXT: unix.DynamicMemoryModeling:AddNoOwnershipChangeNotes = true
 // CHECK-NEXT: unix.DynamicMemoryModeling:Optimistic = false
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: verbose-report-filename = false
Index: clang/test/Analysis/NewDeleteLeaks.cpp
===
--- clang/test/Analysis/NewDeleteLeaks.cpp
+++ clang/test/Analysis/NewDeleteLeaks.cpp
@@ -20,15 +20,16 @@
 
 bool coin();
 
+// TODO: AST analysis of sink would reveal that it doesn't intent to free the
+// allocated memory, but in this instance, its also the only function with
+// the ability to do so, we should see a note here.
 namespace memory_allocated_in_fn_call {
 
 void sink(int *P) {
-} // ownership-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+}
 
 void foo() {
   sink(new int(5)); // expected-note {{Memory is allocated}}
-// ownership-note@-1 {{Calling 'sink'}}
-// ownership-note@-2 {{Returning from 'sink'}}
 } // expected-warning {{Potential memory leak [cplusplus.NewDeleteLeaks]}}
 // expected-note@-1 {{Potential memory leak}}
 
@@ -109,17 +110,14 @@
 
 } // namespace memory_shared_with_ptr_of_same_lifetime
 
-// TODO: We don't want a note here. sink() doesn't seem like a function that
-// even attempts to take care of any memory ownership problems.
 namespace memory_passed_into_fn_that_doesnt_intend_to_free {
 
 void sink(int *P) {
-} // ownership-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+}
 
 void foo() {
   int *ptr = new int(5); // expected-note {{Memory is allocated}}
-  sink(ptr); // ownership-note {{Calling 'sink'}}
- // ownership-note@-1 {{Returning from 'sink'}}
+  sink(ptr);
 } // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
 // expected-note@-1 {{Potential leak}}
 
Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -52,6 +52,8 @@
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ParentMap.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Analysis/ProgramPoint.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceManager.h"
@@ -791,9 +793,28 @@
 return "";
   }
 
+  bool doesFnIntendToHandleOwnership(const Decl *Callee, ASTContext &ACtx) {
+using namespace clang::ast_matchers;
+const FunctionDecl *FD = dyn_cast(Callee);
+if (!FD)
+  return false;
+// TODO: Operator delete is hardly the only deallocator -- Can we reuse
+// isFreeingCall() or something thats already here?
+auto Deallocations = match(
+stmt(hasDescendant(cxxDeleteExpr().bind("delete"))
+ ), *FD->getBody(), ACtx);
+// TODO: Ownership my change with an attempt to store the allocated memory.
+return !Deallocations.empty();
+  }
+
   virtual bool
   wasModifiedInFunction(const ExplodedNode *CallEnterN,
 const ExplodedNode *CallExitEndN) override {
+if (!doesFnIntendToHandleOwnership(
+CallExitEndN->getFirstPred()->getLocationContext()->getDecl(),
+CallExitEndN->getState()->getAnalysisManager().getASTContext()))
+  return true;
+
 if (CallEnterN->getState()->get(Sym) !=
 CallExitEndN->getState()->get(Sym))
   return true;
Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
===
--- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -493,8 +493,8 @@
   "that neither deallocated it, o

[PATCH] D108753: [analyzer] MallocChecker: Add notes from NoOwnershipChangeVisitor only when a function "intents", but doesn't change ownership, enable by default

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D108753#2988632 , @NoQ wrote:

> This looks good!
>
> I guess one way to make this //even more// conservative would be to match the 
> variable inside the delete-expression to the one we expect to get deallocated.

Yeah, I thought about that, but one counter argument to that is that once an 
intent to do some deallocation is already there, its possible that the wrong 
variable was deleted, or the deallocation wasn't thorough enough.

Thanks for the reviews!


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

https://reviews.llvm.org/D108753

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

[PATCH] D108753: [analyzer] MallocChecker: Add notes from NoOwnershipChangeVisitor only when a function "intents", but doesn't change ownership, enable by default

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.

This looks good!

I guess one way to make this //even more// conservative would be to match the 
variable inside the delete-expression to the one we expect to get deallocated.


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

https://reviews.llvm.org/D108753

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

[PATCH] D108753: [analyzer] MallocChecker: Add notes from NoOwnershipChangeVisitor only when a function "intents", but doesn't change ownership, enable by default

[Gabor Marton via Phabricator via cfe-commits]

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

Okay, LGTM! Thanks!


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

https://reviews.llvm.org/D108753

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

[PATCH] D108753: [analyzer] MallocChecker: Add notes from NoOwnershipChangeVisitor only when a function "intents", but doesn't change ownership, enable by default

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 370553.
Szelethus added a comment.

indent->intent


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

https://reviews.llvm.org/D108753

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/test/Analysis/NewDeleteLeaks.cpp
  clang/test/Analysis/analyzer-config.c

Index: clang/test/Analysis/analyzer-config.c
===
--- clang/test/Analysis/analyzer-config.c
+++ clang/test/Analysis/analyzer-config.c
@@ -116,7 +116,7 @@
 // CHECK-NEXT: suppress-null-return-paths = true
 // CHECK-NEXT: track-conditions = true
 // CHECK-NEXT: track-conditions-debug = false
-// CHECK-NEXT: unix.DynamicMemoryModeling:AddNoOwnershipChangeNotes = false
+// CHECK-NEXT: unix.DynamicMemoryModeling:AddNoOwnershipChangeNotes = true
 // CHECK-NEXT: unix.DynamicMemoryModeling:Optimistic = false
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: verbose-report-filename = false
Index: clang/test/Analysis/NewDeleteLeaks.cpp
===
--- clang/test/Analysis/NewDeleteLeaks.cpp
+++ clang/test/Analysis/NewDeleteLeaks.cpp
@@ -20,15 +20,16 @@
 
 bool coin();
 
+// TODO: AST analysis of sink would reveal that it doesn't intent to free the
+// allocated memory, but in this instance, its also the only function with
+// the ability to do so, we should see a note here.
 namespace memory_allocated_in_fn_call {
 
 void sink(int *P) {
-} // ownership-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+}
 
 void foo() {
   sink(new int(5)); // expected-note {{Memory is allocated}}
-// ownership-note@-1 {{Calling 'sink'}}
-// ownership-note@-2 {{Returning from 'sink'}}
 } // expected-warning {{Potential memory leak [cplusplus.NewDeleteLeaks]}}
 // expected-note@-1 {{Potential memory leak}}
 
@@ -109,17 +110,14 @@
 
 } // namespace memory_shared_with_ptr_of_same_lifetime
 
-// TODO: We don't want a note here. sink() doesn't seem like a function that
-// even attempts to take care of any memory ownership problems.
 namespace memory_passed_into_fn_that_doesnt_intend_to_free {
 
 void sink(int *P) {
-} // ownership-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+}
 
 void foo() {
   int *ptr = new int(5); // expected-note {{Memory is allocated}}
-  sink(ptr); // ownership-note {{Calling 'sink'}}
- // ownership-note@-1 {{Returning from 'sink'}}
+  sink(ptr);
 } // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
 // expected-note@-1 {{Potential leak}}
 
Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -52,6 +52,8 @@
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ParentMap.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Analysis/ProgramPoint.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceManager.h"
@@ -791,9 +793,28 @@
 return "";
   }
 
+  bool doesFnIntendToHandleOwnership(const Decl *Callee, ASTContext &ACtx) {
+using namespace clang::ast_matchers;
+const FunctionDecl *FD = dyn_cast(Callee);
+if (!FD)
+  return false;
+// TODO: Operator delete is hardly the only deallocator -- Can we reuse
+// isFreeingCall() or something thats already here?
+auto Deallocations = match(
+stmt(hasDescendant(cxxDeleteExpr().bind("delete"))
+ ), *FD->getBody(), ACtx);
+// TODO: Ownership my change with an attempt to store the allocated memory.
+return !Deallocations.empty();
+  }
+
   virtual bool
   wasModifiedInFunction(const ExplodedNode *CallEnterN,
 const ExplodedNode *CallExitEndN) override {
+if (!doesFnIntendToHandleOwnership(
+CallExitEndN->getFirstPred()->getLocationContext()->getDecl(),
+CallExitEndN->getState()->getAnalysisManager().getASTContext()))
+  return true;
+
 if (CallEnterN->getState()->get(Sym) !=
 CallExitEndN->getState()->get(Sym))
   return true;
Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
===
--- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -493,8 +493,8 @@
   "that neither deallocated it, or have taken responsibility "
   "of the ownership are noted, similarly to "
   "NoStoreFuncVisitor.",
-  "false",
-  InAlpha,
+  "true

[PATCH] D108753: [analyzer] MallocChecker: Add notes from NoOwnershipChangeVisitor only when a function "intents", but doesn't change ownership, enable by default

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:281-288
 /// Tells if the callee is one of the builtin new/delete operators, including
 /// placement operators and other standard overloads.
 static bool isStandardNewDelete(const FunctionDecl *FD);
 static bool isStandardNewDelete(const CallEvent &Call) {
   if (!Call.getDecl() || !isa(Call.getDecl()))
 return false;
   return isStandardNewDelete(cast(Call.getDecl()));

These serve to identify new/delete operators. I've done a lot of work to make 
this better, but I don't remember why `isFreeingCall` doesn't call these.



Comment at: clang/test/Analysis/NewDeleteLeaks.cpp:23
 
+// TODO: AST analysis of sink would reveal that it doesn't indent to free the
+// allocated memory, but in this instance, its also the only function with

martong wrote:
> intent
> 
> BTW, why is this TODO here? It is obvious that `sink` does not have a 
> `delete`, so we don't expect any warning. Or am I missing something?
Well, its debatable, I suppose, but in this case, `sink` is noteworthy, as no 
other function had the ability to take care of this memory.

https://lists.llvm.org/pipermail/cfe-dev/2021-June/068469.html
> Kristof's case is interesting in a different manner. If taken literally, it 
> doesn't satisfy our criteria of "there exists another execution path on which 
> it did actually do ${Something}". We probably shouldn't emit a note every 
> time the function simply accepts the value of interest by pointer, because 
> this doesn't necessarily prove the intent to delete the pointer; there are a 
> million other reasons to accept a pointer. However, Kristof's case does still 
> deserve a note because sink() is the *only* function that had a chance to 
> delete the pointer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108753

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

[PATCH] D108753: [analyzer] MallocChecker: Add notes from NoOwnershipChangeVisitor only when a function "intents", but doesn't change ownership, enable by default

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:802
+// TODO: Operator delete is hardly the only deallocator -- Can we reuse
+// isFreeingCall() or something thats already here?
+auto Deallocations = match(

Sounds like a good idea to reuse `isFreeingCall`. 

But interestingly `delete` is not listed there! Nor `new` in 
`AllocatingMemFnMap`. So, perhaps we need yet another abstraction. Maybe a 
function that iterates over `FreeingMemFnMap` collects the functions and then 
adds `delete` as well?



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:806
+ ), *FD->getBody(), ACtx);
+// TODO: Ownership my change with an attempt to store the allocated memory.
+return !Deallocations.empty();

Good point!



Comment at: clang/test/Analysis/NewDeleteLeaks.cpp:23
 
+// TODO: AST analysis of sink would reveal that it doesn't indent to free the
+// allocated memory, but in this instance, its also the only function with

intent

BTW, why is this TODO here? It is obvious that `sink` does not have a `delete`, 
so we don't expect any warning. Or am I missing something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108753

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

[PATCH] D108753: [analyzer] MallocChecker: Add notes from NoOwnershipChangeVisitor only when a function "intents", but doesn't change ownership, enable by default

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, martong, steakhal, balazske, ASDenysPetrov, 
vsavchenko.
Szelethus added a project: clang.
Herald added subscribers: manas, gamesh411, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun, 
whisperity, yaxunl.
Szelethus requested review of this revision.
Herald added a subscriber: cfe-commits.

D105819  Added NoOwnershipChangeVisitor, but 
it is only registered when an off-by-default, hidden checker option was 
enabled. The reason behind this was that it grossly overestimated the set of 
functions that really needed a note:

  std::string getTrainName(const Train *T) {
return T->name;
  } // note: Retuning without changing the ownership of or deallocating memory
  // Umm... I mean duh? Nor would I expect this function to do anything like 
that...
  
  void foo() {
Train *T = new Train("Land Plane");
print(getTrainName(T)); // note: calling getTrainName / returning from 
getTrainName
  } // warn: Memory leak

This patch adds a heuristic that guesses that any function that has an explicit 
`operator delete` call could have be responsible for deallocating the memory 
that ended up leaking. This is wy too conservative (see the TODOs in the 
new function), but it safer to err on the side of too little than too much, and 
would allow us to enable the option by default *now*, and add refinements 
one-by-one.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108753

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/test/Analysis/NewDeleteLeaks.cpp
  clang/test/Analysis/analyzer-config.c

Index: clang/test/Analysis/analyzer-config.c
===
--- clang/test/Analysis/analyzer-config.c
+++ clang/test/Analysis/analyzer-config.c
@@ -116,7 +116,7 @@
 // CHECK-NEXT: suppress-null-return-paths = true
 // CHECK-NEXT: track-conditions = true
 // CHECK-NEXT: track-conditions-debug = false
-// CHECK-NEXT: unix.DynamicMemoryModeling:AddNoOwnershipChangeNotes = false
+// CHECK-NEXT: unix.DynamicMemoryModeling:AddNoOwnershipChangeNotes = true
 // CHECK-NEXT: unix.DynamicMemoryModeling:Optimistic = false
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
Index: clang/test/Analysis/NewDeleteLeaks.cpp
===
--- clang/test/Analysis/NewDeleteLeaks.cpp
+++ clang/test/Analysis/NewDeleteLeaks.cpp
@@ -20,15 +20,16 @@
 
 bool coin();
 
+// TODO: AST analysis of sink would reveal that it doesn't indent to free the
+// allocated memory, but in this instance, its also the only function with
+// the ability to do so, we should see a note here.
 namespace memory_allocated_in_fn_call {
 
 void sink(int *P) {
-} // ownership-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+}
 
 void foo() {
   sink(new int(5)); // expected-note {{Memory is allocated}}
-// ownership-note@-1 {{Calling 'sink'}}
-// ownership-note@-2 {{Returning from 'sink'}}
 } // expected-warning {{Potential memory leak [cplusplus.NewDeleteLeaks]}}
 // expected-note@-1 {{Potential memory leak}}
 
@@ -109,17 +110,14 @@
 
 } // namespace memory_shared_with_ptr_of_same_lifetime
 
-// TODO: We don't want a note here. sink() doesn't seem like a function that
-// even attempts to take care of any memory ownership problems.
 namespace memory_passed_into_fn_that_doesnt_intend_to_free {
 
 void sink(int *P) {
-} // ownership-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+}
 
 void foo() {
   int *ptr = new int(5); // expected-note {{Memory is allocated}}
-  sink(ptr); // ownership-note {{Calling 'sink'}}
- // ownership-note@-1 {{Returning from 'sink'}}
+  sink(ptr);
 } // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
 // expected-note@-1 {{Potential leak}}
 
Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -52,6 +52,7 @@
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ParentMap.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Analysis/ProgramPoint.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceManager.h"
@@ -737,6 +738,7 @@
 // Definition of NoOwnershipChangeVisitor.
 //===--===//
 
+#include "clang/ASTMatchers/ASTMatchFinder.h"
 namespace {
 class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor {
   SymbolRef Sym;
@@ -791,9 +793,28 @@
 return "";