[Bug analyzer/115436] New: False positive with -Wanalyzer-malloc-leak

[Bug analyzer/115436] New: False positive with -Wanalyzer-malloc-leak

[rickobranimir at gmail dot com]

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

            Bug ID: 115436
           Summary: False positive with -Wanalyzer-malloc-leak
           Product: gcc
           Version: 14.1.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: analyzer
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: rickobranimir at gmail dot com
  Target Milestone: ---

Hello,
I've been playing with the gcc -fanalyzer. It works great, really impressive
stuff.
But I think I have found a bug with it.

This is a simplest cast where I get the wrong analysis:

This is a command that I run:
```
gcc -c -fanalyzer main.c
```

INPUT:
```c
// File: main.c
#include <stdbool.h>
#include <stdlib.h>
#include <string.h>

typedef struct {
  char* str;
  size_t len, cap;
} my_str;

static bool my_str_realloc(my_str* s, size_t new_cap) {
  if (s->cap == 0) {
    s->str = malloc(new_cap > 8 ? new_cap : 8);
    if (s->str == NULL) return false;
    s->cap = new_cap > 8 ? new_cap : 8;
    return true;
  }
  if (s->cap < new_cap) {
    char * newS = realloc(s->str, new_cap);
    if (newS == NULL) return false;
    s->str = newS;
    s->cap = (unsigned int)new_cap;
    return true;
  }
  return false;
}

static bool my_str_push_char(my_str* s, char c) {
  if (s->len >= s->cap) if (!my_str_realloc(s, s->cap * 2)) return false;
  s->str[s->len++] = c;
  return true;
}

int foo(my_str* s) {
  my_str_push_char(s, '/');
  my_str_push_char(s, '/');
  return 0;
}
```

OUTPUT:
```
main.c: In function ‘my_str_realloc’:
main.c:12:12: warning: leak of ‘*s.str’ [CWE-401] [-Wanalyzer-malloc-leak]
   12 |     s->str = malloc(new_cap > 8 ? new_cap : 8);
      |     ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ‘foo’: events 1-2
    |
    |   35 | int foo(my_str* s) {
    |      |     ^~~
    |      |     |
    |      |     (1) entry to ‘foo’
    |   36 |   my_str_push_char(s, '/');
    |      |   ~~~~~~~~~~~~~~~~~~~~~~~~
    |      |   |
    |      |   (2) calling ‘my_str_push_char’ from ‘foo’
    |
    +--> ‘my_str_push_char’: events 3-6
           |
           |   29 | static bool my_str_push_char(my_str* s, char c) {
           |      |             ^~~~~~~~~~~~~~~~
           |      |             |
           |      |             (3) entry to ‘my_str_push_char’
           |   30 |   if (s->len >= s->cap) if (!my_str_realloc(s, s->cap * 2))
return false;
           |      |      ~                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |      |                       |                  |
           |      |      |                       |                  (5) ...to
here
           |      |      |                       (6) calling ‘my_str_realloc’
from ‘my_str_push_char’
           |      |      (4) following ‘true’ branch...
           |
           +--> ‘my_str_realloc’: events 7-13
                  |
                  |   10 | static bool my_str_realloc(my_str* s, size_t
new_cap) {
                  |      |             ^~~~~~~~~~~~~~
                  |      |             |
                  |      |             (7) entry to ‘my_str_realloc’
                  |   11 |   if (s->cap == 0) {
                  |      |      ~      
                  |      |      |
                  |      |      (8) following ‘true’ branch...
                  |   12 |     s->str = malloc(new_cap > 8 ? new_cap : 8);
                  |      |              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                  |      |              |
                  |      |              (9) ...to here
                  |      |              (10) allocated here
                  |   13 |     if (s->str == NULL) return false;
                  |      |        ~    
                  |      |        |
                  |      |        (11) assuming ‘*s.str’ is non-NULL
                  |      |        (12) following ‘false’ branch...
                  |   14 |     s->cap = new_cap > 8 ? new_cap : 8;
                  |      |              ~~~~~~~~~~~~~~~~~~~~~~~~~
                  |      |                                    |
                  |      |                                    (13) ...to here
                  |
           <------+
           |
         ‘my_str_push_char’: events 14-16
           |
           |   30 |   if (s->len >= s->cap) if (!my_str_realloc(s, s->cap * 2))
return false;
           |      |                            ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |                            | |
           |      |                            | (14) returning to
‘my_str_push_char’ from ‘my_str_realloc’
           |      |                            (15) following ‘false’ branch...
           |   31 |   s->str[s->len++] = c;
           |      |   ~~~~~~                      
           |      |    |
           |      |    (16) ...to here
           |
    <------+
    |
  ‘foo’: events 17-18
    |
    |   36 |   my_str_push_char(s, '/');
    |      |   ^~~~~~~~~~~~~~~~~~~~~~~~
    |      |   |
    |      |   (17) returning to ‘foo’ from ‘my_str_push_char’
    |   37 |   my_str_push_char(s, '/');
    |      |   ~~~~~~~~~~~~~~~~~~~~~~~~
    |      |   |
    |      |   (18) calling ‘my_str_push_char’ from ‘foo’
    |
    +--> ‘my_str_push_char’: events 19-22
           |
           |   29 | static bool my_str_push_char(my_str* s, char c) {
           |      |             ^~~~~~~~~~~~~~~~
           |      |             |
           |      |             (19) entry to ‘my_str_push_char’
           |   30 |   if (s->len >= s->cap) if (!my_str_realloc(s, s->cap * 2))
return false;
           |      |      ~                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |      |                       |                  |
           |      |      |                       |                  (21) ...to
here
           |      |      |                       (22) calling ‘my_str_realloc’
from ‘my_str_push_char’
           |      |      (20) following ‘true’ branch...
           |
           +--> ‘my_str_realloc’: events 23-26
                  |
                  |   10 | static bool my_str_realloc(my_str* s, size_t
new_cap) {
                  |      |             ^~~~~~~~~~~~~~
                  |      |             |
                  |      |             (23) entry to ‘my_str_realloc’
                  |   11 |   if (s->cap == 0) {
                  |      |      ~      
                  |      |      |
                  |      |      (24) following ‘true’ branch...
                  |   12 |     s->str = malloc(new_cap > 8 ? new_cap : 8);
                  |      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                  |      |            | |
                  |      |            | (25) ...to here
                  |      |            (26) ‘*s.str’ leaks here; was allocated
at (10)
                  |
```

I don't think that path that analyzer chose is valid, because if fist
```my_str_push_char(s, '/');``` triggers realloc, second call can not trigger
malloc. In fact, second call should not even call realloc...

```
% gcc --version          
gcc (GCC) 14.1.1 20240507
Copyright (C) 2024 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
```
also tested on: gcc (GCC) 14.1.1 20240522

[Bug analyzer/115436] False positive with -Wanalyzer-malloc-leak

[dmalcolm at gcc dot gnu.org]

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

David Malcolm <dmalcolm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2024-06-11
     Ever confirmed|0                           |1

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

I think there *might* be a true positive here for the case where s->cap ==
0x80000000, so that s->cap * 2 becomes 0 due to overflow; should my_str_realloc
be checking for s->str being null for the "needs malloc" case?

Otherwise, confirmed with trunk using Compiler Explorer:
https://godbolt.org/z/c3vEYf6do

Looks like
(a) it's not "realizing" that s->cap must be non-zero after the first alloc
(with the caveat about overflow noted above)

(b) there's a definite bug in binding_map, where __analyzer_dump () shows an
overlapping concrete binding:

clusters within root region
  cluster for: (*INIT_VAL(s_2(D)))
    ESCAPED
    key:   {bytes 0-7}
    value: 'char *' {UNKNOWN(char *)}
    key:   {bytes 0-23}
    value: 'struct my_str' {UNKNOWN(struct my_str)}
    key:   {bytes 16-23}
    value: 'unsigned int' {UNKNOWN(unsigned int)}

where the binding for bytes 0-23 overlaps that for bytes 0-7.

[Bug analyzer/115436] False positive with -Wanalyzer-malloc-leak

[rickobranimir at gmail dot com]

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

--- Comment #2 from Branimir Ričko <rickobranimir at gmail dot com> ---
```
I think there *might* be a true positive here for the case where s->cap ==
0x80000000, so that s->cap * 2 becomes 0 due to overflow; should my_str_realloc
be checking for s->str being null for the "needs malloc" case?
```

Yea, you are right. When I was making the example shorter, I must have made it
too short...

This should not be affected by overflows: https://godbolt.org/z/dhPcz8Kj4

Analyzer says that on first call to push, *len* is >= *cap*.
That means that cap is 0.
And then it mallocs on first call.
But then it also deduces that on second call *len* can be >= *cap*.
    This can not be because *cap* was set to 8 on the first call and *len* to
1.

```
(b) there's a definite bug in binding_map, where __analyzer_dump () shows an
overlapping concrete binding:

clusters within root region
  cluster for: (*INIT_VAL(s_2(D)))
    ESCAPED
    key:   {bytes 0-7}
    value: 'char *' {UNKNOWN(char *)}
    key:   {bytes 0-23}
    value: 'struct my_str' {UNKNOWN(struct my_str)}
    key:   {bytes 16-23}
    value: 'unsigned int' {UNKNOWN(unsigned int)}

where the binding for bytes 0-23 overlaps that for bytes 0-7.
```

Idk what this is or should be, but to me it looks like `struct my_str` should
overlap with `char *`.
`struct my_str` is *s
and
`char *` is `s->str`

Or am I missing something?