[Bug analyzer/101713] New: -Wanalyzer-malloc-leak false positive with GNU coreutils hash table code

[Bug analyzer/101713] New: -Wanalyzer-malloc-leak false positive with GNU coreutils hash table code

[eggert at cs dot ucla.edu]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101713

            Bug ID: 101713
           Summary: -Wanalyzer-malloc-leak false positive with GNU
                    coreutils hash table code
           Product: gcc
           Version: 11.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: analyzer
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: eggert at cs dot ucla.edu
  Target Milestone: ---

Because of GCC bug 94458 we've been disabling -Wanalyzer-malloc-leak when
compiling Gnulib-based code such as GNU coreutils. Since GCC bug 94458 is fixed
I thought I'd try enabling that warning. I simplified the first false positive
I ran into (in gnulib/lib/exclude.c) to the following:

void free (void *);
char *xstrdup (char const *)
  __attribute__ ((__malloc__)) __attribute__ ((__malloc__ (free, 1)))
  __attribute__ ((__returns_nonnull__));
void *hash_insert (void const *entry);

void
addpat (char *pattern)
{
  char *str = xstrdup (pattern);
  hash_insert (str);
}

For this example, the command 'gcc -fanalyzer -Wanalyzer-too-complex -O2 -S
t1.i' outputs the following diagnostic, which is a false alarm because 'str'
has been put into a hash table and has not leaked. Omitting the 'const' from
the declaration of the 'entry' formal parameter makes the false alarm go away,
but we shouldn't have to omit the 'const'. For now, I think we'll continue to
disable -Wanalyzer-too-complex in Gnulib-derived code.

t1.i: In function ‘addpat’:
t1.i:12:1: warning: leak of ‘str’ [CWE-401] [-Wanalyzer-malloc-leak]
   12 | }
      | ^
  ‘addpat’: events 1-2
    |
    |   10 |   char *str = xstrdup (pattern);
    |      |               ^~~~~~~~~~~~~~~~~
    |      |               |
    |      |               (1) allocated here
    |   11 |   hash_insert (str);
    |   12 | }
    |      | ~
    |      | |
    |      | (2) ‘str’ leaks here; was allocated at (1)
    |

[Bug analyzer/101713] -Wanalyzer-malloc-leak false positive with GNU coreutils hash table code

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101713

--- Comment #1 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Thanks for reporting this issue.

The analyzer sees that "str" is passed to hash_insert as a const pointer, and
therefore assumes that hash_insert cannot free it, that hash_insert is merely
borrowing a pointer to str, rather than taking ownership.

Am I right in thinking that there's a cast somewhere inside the hash table code
that at some point casts away the const from the pointer and frees it?

[Bug analyzer/101713] -Wanalyzer-malloc-leak false positive with GNU coreutils hash table code

[eggert at cs dot ucla.edu]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101713

--- Comment #2 from eggert at cs dot ucla.edu ---
(In reply to David Malcolm from comment #1)

> Am I right in thinking that there's a cast somewhere inside the hash table
> code that at some point casts away the const from the pointer and frees it?

Yes there's a cast to void *, but no it doesn't free the storage. Instead, the
caller later is supposed to take ownership of the storage if it removes the
item from the hashtable, e.g., the caller can later do this:

   void *entry = hash_remove (pattern);
   free (entry);

where ENTRY will be the same pointer as STR in the sample code I gave earlier.
This means there's no leak in addpat per se.

[Bug analyzer/101713] -Wanalyzer-malloc-leak false positive with GNU coreutils hash table code

[egallager at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101713

Eric Gallager <egallager at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2024-04-02

--- Comment #3 from Eric Gallager <egallager at gcc dot gnu.org> ---
Confirming due to the link to it from gnulib's manywarnings.m4