public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed
* [Bug analyzer/115436] New: False positive with -Wanalyzer-malloc-leak
@ 2024-06-11 15:10 rickobranimir at gmail dot com
2024-06-11 22:14 ` [Bug analyzer/115436] " dmalcolm at gcc dot gnu.org
2024-06-12 7:14 ` rickobranimir at gmail dot com
0 siblings, 2 replies; 3+ messages in thread
From: rickobranimir at gmail dot com @ 2024-06-11 15:10 UTC (permalink / raw)
To: gcc-bugs
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* [Bug analyzer/115436] False positive with -Wanalyzer-malloc-leak
2024-06-11 15:10 [Bug analyzer/115436] New: False positive with -Wanalyzer-malloc-leak rickobranimir at gmail dot com
@ 2024-06-11 22:14 ` dmalcolm at gcc dot gnu.org
2024-06-12 7:14 ` rickobranimir at gmail dot com
1 sibling, 0 replies; 3+ messages in thread
From: dmalcolm at gcc dot gnu.org @ 2024-06-11 22:14 UTC (permalink / raw)
To: gcc-bugs
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
* [Bug analyzer/115436] False positive with -Wanalyzer-malloc-leak
2024-06-11 15:10 [Bug analyzer/115436] New: False positive with -Wanalyzer-malloc-leak rickobranimir at gmail dot com
2024-06-11 22:14 ` [Bug analyzer/115436] " dmalcolm at gcc dot gnu.org
@ 2024-06-12 7:14 ` rickobranimir at gmail dot com
1 sibling, 0 replies; 3+ messages in thread
From: rickobranimir at gmail dot com @ 2024-06-12 7:14 UTC (permalink / raw)
To: gcc-bugs
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?
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-06-12 7:14 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-06-11 15:10 [Bug analyzer/115436] New: False positive with -Wanalyzer-malloc-leak rickobranimir at gmail dot com
2024-06-11 22:14 ` [Bug analyzer/115436] " dmalcolm at gcc dot gnu.org
2024-06-12 7:14 ` rickobranimir at gmail dot com
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).