[PATCH] D158156: [analyzer] Add C++ array delete checker

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

Add a test where we have multiple imp derived to base casts but only one leads to a bug. Do it for both ways, once that the first is the offending, and once the second.



================
Comment at: clang/docs/analyzer/checkers.rst:1793-1803
+.. code-block:: cpp
+
+ Base *create() {
+   Base *x = new Derived[10]; // note: conversion from derived to base
+                              //       happened here
+   return x;
+ }
----------------
Discookie wrote:
> steakhal wrote:
> > Make sure in the example the `create` is related (e.g. called/used).
> > Also, refrain from using `sink` in the docs. It's usually used in the context of taint analysis.
> Changed the example - should I change the DeleteWithNonVirtualDtor example as well? That has the same issues as you said here.
I have no opinion. You could.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:126
+
+  if (!DerivedClass->isDerivedFrom(BaseClass))
+    return;
----------------
Is this transitive?

BTW inheritance can only be expressed if the class is a definition, right?
Thus passing this should imply has definition.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:143
+  R->markInteresting(BaseClassRegion);
+  R->addVisitor(std::make_unique<PtrCastVisitor>());
+  C.emitReport(std::move(R));
----------------
I think addVisitor already takes a template param, and it will call make unique for you.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:192
+
+  const Stmt *S = N->getStmtForDiagnostics();
+  if (!S)
----------------
Aren't you actually interested in N->getLocation().getAs<StmtPoint>().getStmt()?

The diag stmt can be fuzzy, but the PP is exact.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:196
+
+  const auto *CastE = dyn_cast<CastExpr>(S);
+  if (!CastE)
----------------
If you dyncast to ImplicitCastExpr, couldn't you have done it here?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:202
+  // Explicit casts can have different CastKinds.
+  if (const auto *ImplCastE = dyn_cast<ImplicitCastExpr>(CastE)) {
+    if (ImplCastE->getCastKind() != CK_DerivedToBase)
----------------
How do you know that that this castexpr corresponds to the region for which you report the bug? To mez this might be some unrelated castexpr.
I was expecting the offending memregion being passed to the visitor, that it can check against.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:217
+  // Stop traversal on this path.
+  Satisfied = true;
+
----------------
There are so many early returns going on. I feel, we could miss the program point where it should have been marked satisfied. After this point, the visitor will never or should never emit a note.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:224
+                             N->getLocationContext());
+  return std::make_shared<PathDiagnosticEventPiece>(Pos, OS.str(), true);
+}
----------------
Use named argument passing.


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

https://reviews.llvm.org/D158156