Static analyzer: possible memory leak false positive?

Hi all,

Before spamming Bugzilla, I'd like an opinion on this possible false positive in the static analyzer.

See http://scan.freebsd.your.org/freebsd-head/bin.df/2012-09-12-amd64/report-WwB2qk.html#EndPath

The analyzer is complaining about a memory leak of mntbuf two lines before the end of the main() function.

I'm not sure this is actually a false positive since it relies on the implicit behavior that the OS reclaims the memory regardless, but the warning does seem of minor value.

Kind regards,
Erik Cederstrand

I wouldn't classify it as false positive. E.g. it is perfectly sane to
redefine main as macro to use in a larger program.

Joerg

I think it is reasonable to expect the analyzer not to warn in this particular case.

However, constructing a rule on when the error should be suppressed is tricky. One possibility is that the analyzer should dismiss the warning only if it can prove that nothing allocates memory after the leak. For example, if main calls foo() after the leak is detected, we should warn, unless we can prove that foo() does not allocate memory. This rule would silence the warning in this case, but might not work in case where foo() is called after the leak and the analyzer does not know what the effect of foo() will be.

Joerg, I think that, since the analyzer looks at preprocessed code, redefinition of main would not be an issue.

Cheers,
Anna.

I think it is reasonable to expect the analyzer not to warn in this particular case.

FWIW, I plan to fix this case. But, it would be nice to differentiate
the output from a legitimate one.

However, constructing a rule on when the error should be suppressed is tricky. One possibility is that the analyzer should dismiss the warning only if it can prove that nothing allocates memory after the leak. For example, if main calls foo() after the leak is detected, we should warn, unless we can prove that foo() does not allocate memory. This rule would silence the warning in this case, but might not work in case where foo() is called after the leak and the analyzer does not know what the effect of foo() will be.

+1

Joerg, I think that, since the analyzer looks at preprocessed code, redefinition of main would not be an issue.

I believe his point is that since someone may redefine main, and
therefore inherit a leak, which is non-existent in the original code.
In other words, it may not be an issue for this program, but its still
nice to cleanup after oneself.

It turns out it was a bit more complicated than this. The memory pointed to by mntbuf is allocated by getmntinfo() from FreeBSD libc (original report: http://scan.freebsd.your.org/freebsd-head/bin.df/2012-09-12-amd64/report-WwB2qk.html#EndPath). The man page for this function says: "The memory allocated by getmntinfo() cannot be free(3)'d by the application." (getmntinfo).

I believe the reason for this is that the memory is shared between calls: fxr.watson.org: FREEBSD-LIBC sys/gen/getmntinfo.c

In this specific case, it should be OK to free the memory anyway since malloc'ed memory only lives for the duration of the application, but someone on the FreeBSD list pointed out that an alternative libc implementation might instead implement getmntinfo() with file-backed mmap, in which case we can't free it.

I'm uncertain how to proceed here, so I'd like an opinion :slight_smile:

Thanks,
Erik

Erik,

The analyzer specifically complains about memory allocated by malloc here:

196 | /* just the filesystems specified on the command line */ |

  • | - |
    197 | mntbuf = malloc(argc * sizeof(*mntbuf)); |

So the call to free could be conditioned on the value of “!*argv” as well.

Cheers,
Anna.

Hi Anna,

Erik,

The analyzer specifically complains about memory allocated by malloc here:
196 /* just the filesystems specified on the command line */
197 mntbuf = malloc(argc * sizeof(*mntbuf));

So the call to free could be conditioned on the value of "!*argv" as well.

Thanks for the hint! I tried doing this, but it's still complaining. It seems the analyzer is confused about the value of argv. First, it takes this decision:

191 if (!*argv) {
2 ← Taking false branch →

So *argv must be evaluated as True. Then it does this:

205 for (; *argv; argv++) {
6 ← Loop condition is false. Execution continues on line 280 →

Now *argv is evaluated as False, right? I can't see that argv is touched along the way.

Thanks,
Erik

The process is exiting here, it shouldn't waste any time freeing memory.

Possible solution for this case:

- Have the analyzer treat return from main like it treats a call to exit(),
  at least by default (technically main could be called from another
  function).

- Have an analyzer_free() function which acts like free to the analyzer
  but is a no-op to the compiler. The experimental malloc checker supports
  this:

  static __inline__ __attribute__(( __always_inline__ ))
  __attribute__(( __ownership_takes__( __malloc__, 1 )))
  void
  analyzer_free( __attribute__(( __unused__ )) void *__p ) {
  }

  Then call analyzer_free(mntbuf) instead of free(mntbuf).

- Declare mntbuf static which tells the analyzer the memory is allowed to
  persist for the lifetime of the process.

- Have the analyzer treat return from main like it treats a call to exit(),
  at least by default (technically main could be called from another
  function).

This is my preference. Having a default on tunable that treaks main()
like exit().

- Have an analyzer_free() function which acts like free to the analyzer
  but is a no-op to the compiler. The experimental malloc checker supports
  this:

I would very much prefer not this option. It requires special annotation for the
analyzer to work correctly. csa has been very good about avoiding this
until now.

- Declare mntbuf static which tells the analyzer the memory is allowed to
  persist for the lifetime of the process.

This seems more of a workaround for an analyzer bug than a solution in
the general case.

Erik,

This is a bug. I opened http://llvm.org/bugs/show_bug.cgi?id=13974

You could use a flag to simplify the code for the analyzer as a workaround:

unsigned flag = *argv;
// use the flag in conditions instead of *argv.

Anna.

- Have the analyzer treat return from main like it treats a call to exit(),
at least by default (technically main could be called from another
function).

This is my preference. Having a default on tunable that treaks main()
like exit().

This might silence too many leak reports. Ex: If main allocates memory in the very beginning, which is not used later on, followed by all the processing, followed by return. We have a bug report tracking this internally, I'll update with the suggestion.

Thanks!
Anna.