[analyzer] Wrong warning location of memory leak

[mvpant]

// clang --analyze -Xanalyzer -analyzer-output=text

#include <stdint.h>
#include <stdlib.h>
#include <stdbool.h>

void leak(bool v1) {
  void* v3 = malloc(1);
  if (v3 != NULL) {
    int v5 = 0; // <--- warning: Potential leak of memory pointed to by 'v3' [unix.Malloc]
    if (v1) {
      return; // <--- Expected warning location
    }
  }
  return;
}

void leak2(bool v1) {
  void* v3 = malloc(1);
  if (v3 != NULL) {
    if (v1) { // <--- warning: Potential leak of memory pointed to by 'v3' [unix.Malloc]
      return; // <--- Expected warning location
    }
  }
  return;
}

void caller() {
  leak(1);
  leak2(1);
  return;
}

Godbolt example

[llvmbot]

@llvm/issue-subscribers-clang-static-analyzer

Author: Michael (mvpant)

```c // clang --analyze -Xanalyzer -analyzer-output=text

#include <stdint.h>
#include <stdlib.h>
#include <stdbool.h>

void leak(bool v1) {
void* v3 = malloc(1);
if (v3 != NULL) {
int v5 = 0; // <--- warning: Potential leak of memory pointed to by 'v3' [unix.Malloc]
if (v1) {
return; // <--- Expected warning location
}
}
return;
}

void leak2(bool v1) {
void* v3 = malloc(1);
if (v3 != NULL) {
if (v1) { // <--- warning: Potential leak of memory pointed to by 'v3' [unix.Malloc]
return; // <--- Expected warning location
}
}
return;
}

void caller() {
leak(1);
leak2(1);
return;
}

[Godbolt example](https://godbolt.org/z/8qGP347Yv)
</details>

[Flandini]

Looks like the leak is getting reported on a path immediately after v3 is dead. In MallocChecker::checkDeadSymbols, HandleLeak is getting called on the conjured symbol returned by malloc at these warning locations because it is dead.

It's not a wrong spot to warn, the question is if this is the best spot to warn or if a different warning message would help

[steakhal]

Looks like the leak is getting reported on a path immediately after v3 is dead. In MallocChecker::checkDeadSymbols, HandleLeak is getting called on the conjured symbol returned by malloc at these warning locations because it is dead.

It's not a wrong spot to warn, the question is if this is the best spot to warn or if a different warning message would help

Your assessment is correct. To me, it looks good, but I'm already used to weirdly placed leak diags.
If you know better places to shift this fiag, I'm all ears.

[steakhal]

Btw, why do you expect for the first leak case the diag at the first return statement? If I'd follow the reasoning, wouldn't be equally expected at the other return statement too?

[Flandini]

@steakhal

To me, it looks good, but I'm already used to weirdly placed leak diags.
If you know better places to shift this fiag, I'm all ears.

What about changing the warning in these dead symbol cases from Potential leak of memory pointed to by 'v3' to

• 'v3' points to memory that is potentially leaked along all paths starting here
  or
• 'v3' points to memory that is potentially leaked along all paths starting here and may be leaked along other paths

[mvpant]

Looks like the leak is getting reported on a path immediately after v3 is dead.

Makes sense.

It's not a wrong spot to warn, the question is if this is the best spot to warn or if a different warning message would help

I would say that that emitting a warning at sink location is more intuitive.

Btw, why do you expect for the first leak case the diag at the first return statement? If I'd follow the reasoning, wouldn't be equally expected at the other return statement too?

I don’t think it’s necessary to emit a warning at every sink location to catch a bug; that could make things too noisy in functions with multiple exits. Getting at least one warning at right place should be enough to spot the bug easily.

What about changing the warning

This would at least improve readability.