[PATCH] D139534: [analyzer] Don't escape local static memregions on bind

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Ok screw it, my static example is still broken. There's technically still a leak in it because there's no time for the other code to kick in between `p = (int *)malloc(sizeof(int));` and `p = 0;` to read the value from `p`.

But in practice there could be a lot of things going on in between, that with your patch might not trigger enough escapes. Let me disconnect from the original test case and try to build a few examples more carefully.

  void foo() {
    static int *p;
    escape(&p);
    p = malloc(sizeof(int));
    free_whatever_escaped();
    p = 0; // no-leak
  }

This isn't a leak, regardless of whether the variable is static or not. Currently we display a leak when the variable is non-static (bad) but don't display a leak when the variable is static (good). IIUC your patch might break this. The important part is that in this case there's no direct connection between the tracked pointer `p` and the escaped region `&p` because `p` isn't stored in `&p` until the next line. So if you remove invalidation of `p` on `free_whatever_escaped();`, it'll cause a new false positive in the static case.

  void foo(cond) {
    static int *p;
    if (cond) {
      p = malloc(sizeof(int));
      free_whatever_escaped();
      p = 0; // no-leak
    }
    escape(&p);
  }

In this case there's no leak as long as the first invocation of the function always has `cond` set to false. In this case, again, there's no direct connection between `&p` and the return value of `malloc()` when direct escape of `&p` happens. On top of that, the direct escape of `&p` isn't observed at all until much later in the analysis. But just because the local is static, and the function can be called multiple times, the escape at the end of the analysis may "affect" our decision-making at the beginning of the analysis. I suspect your patch breaks this example as well.

So basically in order to do the right thing here, you need to make sure there are no direct escapes of that static variable *anywhere* in the function. But as long as there's no direct escapes, you're probably good to go. With non-static locals you only need to observe escapes *before* the leak (but possibly before the allocation as well), which we currently don't do a good job at anyway.

Then, again, it's possible that your patch improves FP rates than what we currently have, just by being a different trade-off, given how artificial my examples are, but we'll need some data to figure it out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139534