[PATCH] D47135: [analyzer] A checker for dangling internal buffer pointers in C++

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.

Looks great, thanks! I think you should add a check for UnknownVal and commit.



================
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:65
+  if (Call.isCalled(CStrFn)) {
+    SymbolRef RawPtr = Call.getReturnValue().getAsSymbol();
+    State = State->set<RawPtrMap>(TypedR, RawPtr);
----------------
xazax.hun wrote:
> xazax.hun wrote:
> > I wonder if we can always get a symbol.
> > I can think of two cases when the call above could fail:
> > * Non-standard implementation that does not return a pointer
> > * The analyzer able to inline stuff and the returned value is a constant (a specific address that is shared between all empty strings in some implementation?)
> > 
> > Even though I do find any of the above likely. @NoQ what do you think? Does this worth a check?
> Sorry for the spam. Unfortunately, it is not possible to edit the comments.
> I do *not* find any of the above likely.
We should almost always check if any value is an `UnknownVal`. The engine simply doesn't give any guarantees: it can give up any time and fall back to `UnknownVal`.


================
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:73
+    if (State->contains<RawPtrMap>(TypedR)) {
+      const SymbolRef *StrBufferPtr = State->get<RawPtrMap>(TypedR);
+      const Expr *Origin = Call.getOriginExpr();
----------------
xazax.hun wrote:
> xazax.hun wrote:
> > What if no symbol is associated with the region? Won't this return null that we dereference later on?
> Oh, never mind this one, I did not notice the `contains` call above.
The interesting part here is what happens if no expression is associated with the call. For instance, if the call is an automatic destructor at the end of scope. I hope it works, but i'm not sure how Origin is used.


================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1661
+  case AF_CXXNewArray:
+  case AF_InternalBuffer: {
     if (IsALeakCheck) {
----------------
rnkovacs wrote:
> Is tying this new family to NewDeleteChecker reasonable? I did it because it was NewDelete that warned naturally before creating the new `AllocationFamily`. Should I introduce a new checker kind in MallocChecker for this purpose instead?
Uhm, this code is weird.

I think you can add an "external" ("associated", "plugin") CheckKind that always warns.


https://reviews.llvm.org/D47135