[PATCH] D86445: [analyzer][RFC] Simplify MetadataSymbol representation and resolve a CStringChecker FIXME

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2429
     if (SymbolRef Sym = Len.getAsSymbol()) {
       if (SR.isDead(Sym))
+        Entries = F.remove(Entries, MR);
----------------
martong wrote:
> steakhal wrote:
> > NoQ wrote:
> > > Ok, this doesn't look correct (looks like it never was). It's liveness of the region that's important to us, not liveness of the symbol. Because it's the liveness of the region that causes the program to be able to access the map entry.
> > Let's say we have this:
> > ```lang=C++
> > void foo() {
> >   char *p = malloc(12);
> >   // strlen(p); // no-leak if strlen called, leak warning otherwise...
> > } // expected-warning {{leak}}
> > ```
> > If we were marking the region live here, we would miss the `leak` warning. That warning is only triggered if the region of the `p` becomes dead. Which will never become dead if we have a cstring length symbol associated to that region.
> > I came to this conclusion after implementing your suggested edit above (checking regions instead of symbols).
> Is it possible to have two string length symbols that are associated to the same region? Could that cause any problem?
> E.g. what will happen below? Will we remove `MR` twice? 
> ```
> char *p = "asdf"
> char *q = p + 1;
> strlen(p); strlen(q);
> ```
> Which will never become dead if we have a cstring length symbol associated to that region.

That's not how it's supposed to work. Symbols don't keep their associated regions alive, only regions keep symbols associated with them alive.

Don't manipulate region liveness manually at all here. Regardless of `strlen()`, the heap region dies with `p` which is the last variable to refer to it. Rely on it to garbage-collect the symbol for `strlen(p)` but don't actively mutate it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86445