Re: [PATCH] add support for -Wmismatched-dealloc

[David Malcolm <dmalcolm@redhat.com>]

On Tue, 2021-01-12 at 09:59 +0100, Florian Weimer wrote:
> * Martin Sebor via Libc-alpha:
> 
> > > realpath only returns a pointer to the heap if RESOLVED is null,
> > > so
> > > the annotation is wrong here.
> > This is intentional.  When realpath() returns the last argument
> > (when it's nonnull) passing the returned pointer to free will not
> > be diagnosed but passing it to some other deallocator not
> > associated
> > with the function will be.  That means for example that passing
> > a pointer allocated by C++ operator new() to realpath() and then
> > deleting the pointer returned from the function as opposed to
> > the argument will trigger a false positive.  I decided this was
> > an okay trade-off because unless the function allocates memory
> > I expect the returned pointer to be ignored (similarly to how
> > the pointer returned from memcpy is ignored).  If you don't like
> > the odds I can remove the attribute from the function until we
> > have one that captures this conditional return value (I'd like
> > to add one in GCC 12).
> 
> Maybe David can comment on how this interacts with his static
> analyzer
> work.  

BTW, the -fanalyzer part of support for
__attribute__((malloc(deallocator))) is in gcc 11 as of yesterday:
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=c7e276b869bdeb4a95735c1f037ee1a5f629de3d

> In all other cases, the attribute means that the pointer needs to
> be freed to avoid a resource leak.

Indeed, the analyzer doesn't have any special knowledge of realpath and
the conditional behavior.  Given that annotation it will assume that
the returned value is either a pointer that needs to be freed, or NULL
on a failure, with no knowledge that the 2nd argument could be
returned.

I haven't tested Martin's patch, but I tried this example:

$ cat t.c
#include <stdio.h>

#define PATH_MAX 4096
void free(void *ptr);
char *realpath(const char *path, char *resolved_path)
  __attribute__ ((malloc (free)))
  __attribute__ ((__warn_unused_result__));

void test_1 (const char *path)
{
  char buf[PATH_MAX];
  char *result = realpath (path, buf);
  printf ("result: %s\n", result);
}

void test_2 (const char *path)
{
  char buf[PATH_MAX];
  char *result = realpath (path, buf);
  printf ("result: %s\n", result);
  free (result);
}

I believe test_1 is correct (although redundant in its use of "result"
rather than buf, and can output a truncated path).

I believe test_2 is a crasher bug: a "free" of on-stack "buf".

Compiling with GCC 11 (with the __attribute__((malloc (DEALLOCATOR)))
support:

$ ./xgcc -B. -c -fanalyzer t.c
t.c: In function ‘test_1’:
t.c:14:1: warning: leak of ‘result’ [CWE-401] [-Wanalyzer-malloc-leak]
   14 | }
      | ^
  ‘test_1’: events 1-2
    |
    |   12 |   char *result = realpath (path, buf);
    |      |                  ^~~~~~~~~~~~~~~~~~~~
    |      |                  |
    |      |                  (1) allocated here
    |   13 |   printf ("result: %s\n", result);
    |   14 | }
    |      | ~                 
    |      | |
    |      | (2) ‘result’ leaks here; was allocated at (1)
    |

Here it falsely complains about test_1; it doesn't "know" that buf is
returned by realpath and assumes that result needs to be freed.

In test_2, there's a free of result == buf i.e. a free of an on-stack
buffer, which it doesn't complain about, treating "result" as a malloc-
ed pointer (as specified by the attribute).

So I don't think this attribute should be applied to realpath.

  If we suddenly apply it pointers
> which can only conditionally be freed, that reduces the value of
> those
> annotations, I think.

FWIW, the analyzer already special-cases some functions; see the
various region_model::impl_call_* functions in:
 https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/analyzer/region-model-impl-calls.cc

There's a case for doing this for stuff in POSIX, which would apply
here, I think.

As noted above, I haven't tested Martin's glibc patch (I don't think
I'm subscribed to this list).

> Thanks,
> Florian

Hope this is constructive
Dave