On Thu, 2023-02-16 at 15:35 +0100, Alejandro Colomar via Gcc wrote:
> Hi!
>
> I was preparing an example program of a use-after-realloc bug,
> when I found that GCC doesn't warn in a case where it should.
>
>
> alx@debian:~/tmp$ cat realloc.c
> #include <stdint.h>
> #include <stdlib.h>
> #include <stdio.h>
> #include <string.h>
> #include <unistd.h>
>
> static inline char *
> xstrdup(const char *s)
> {
> char *p;
>
> p = strdup(s);
> if (p == NULL)
> exit(EXIT_FAILURE);
> return p;
> }
>
> static inline char *
> strnul(const char *s)
> {
> return (char *) s + strlen(s);
> }
>
> int
> main(void)
> {
> char *p, *q;
>
> p = xstrdup("");
> q = strnul(p);
>
> if (p == q)
> puts("equal before");
> else
> exit(EXIT_FAILURE); // It's an empty string; this
> won't happen
>
> printf("p = %p; q = %p\n", p, q);
>
> p = realloc(p, UINT16_MAX);
> if (p == NULL)
> exit(EXIT_FAILURE);
> puts("realloc()");
>
> if (p == q) { // Use after realloc. I'd expect a warning
> here.
> puts("equal after");
> } else {
> /* Can we get here?
> Let's see the options:
>
> - realloc(3) fails:
> We exit immediately. We don't arrive
> here.
>
> - realloc(3) doesn't move the memory:
> p == q, as before
>
> - realloc(3) moved the memory:
> p is guaranteed to be a unique
> pointer,
> and q is now an invalid pointer. It
> is
> Undefined Behavior to read `q`, so `p
> == q`
> is UB.
>
> As we see, there's no _defined_ path where this
> can happen
> */
> printf("PID = %i\n", (int) getpid());
> }
>
> printf("p = %p; q = %p\n", p, q);
> }
> alx@debian:~/tmp$ cc -Wall -Wextra realloc.c -O3 -fanalyzer
> realloc.c: In function ‘main’:
> realloc.c:67:9: warning: pointer ‘p’ may be used after ‘realloc’ [-
> Wuse-after-free]
> 67 | printf("p = %p; q = %p\n", p, q);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> realloc.c:39:13: note: call to ‘realloc’ here
> 39 | p = realloc(p, UINT16_MAX);
> | ^~~~~~~~~~~~~~~~~~~~~~
> alx@debian:~/tmp$ ./a.out
> equal before
> p = 0x55bff80802a0; q = 0x55bff80802a0
> realloc()
> PID = 25222
> p = 0x55bff80806d0; q = 0x55bff80802a0
>
>
> Did I miss anything?
GCC's -fanalyzer will warn if you dereference q, so e.g. adding:
printf("*q = %i\n", *q);
gives a warning:
https://godbolt.org/z/6qx4afb3E
<source>: In function 'main':
<source>:65:29: warning: use after 'free' of 'q' [CWE-416]
[-Wanalyzer-use-after-free]
65 | printf("*q = %i\n", *q);
| ^~
'main': events 1-2
|
| 25 | main(void)
| | ^~~~
| | |
| | (1) entry to 'main'
|......
| 29 | p = xstrdup("");
| | ~~~~~~~~~~~
| | |
| | (2) calling 'xstrdup' from 'main'
|
+--> 'xstrdup': events 3-5
|
| 8 | xstrdup(const char *s)
| | ^~~~~~~
| | |
| | (3) entry to 'xstrdup'
|......
| 13 | if (p == NULL)
| | ~
| | |
| | (4) following 'false' branch (when 'p' is
non-NULL)...
| 14 | exit(EXIT_FAILURE);
| 15 | return p;
| | ~
| | |
| | (5) ...to here
|
<------+
|
'main': events 6-15
|
| 29 | p = xstrdup("");
| | ^~~~~~~~~~~
| | |
| | (6) returning to 'main' from 'xstrdup'
|......
| 32 | if (p == q)
| | ~
| | |
| | (7) following 'true' branch (when 'p == q')...
| 33 | puts("equal before");
| | ~~~~~~~~~~~~~~~~~~~~
| | |
| | (8) ...to here
|......
| 39 | p = realloc(p, UINT16_MAX);
| | ~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (9) freed here
| | (10) when 'realloc' succeeds, moving buffer
| 40 | if (p == NULL)
| | ~
| | |
| | (11) following 'false' branch (when 'p' is non-NULL)...
| 41 | exit(EXIT_FAILURE);
| 42 | puts("realloc()");
| | ~~~~~~~~~~~~~~~~~
| | |
| | (12) ...to here
| 43 |
| 44 | if (p == q) { // Use after realloc. I'd expect a warning
here.
| | ~
| | |
| | (13) following 'false' branch (when 'p != q')...
|......
| 64 | printf("PID = %i\n", (int) getpid());
| | ~~~~~~~~
| | |
| | (14) ...to here
| 65 | printf("*q = %i\n", *q);
| | ~~
| | |
| | (15) use after 'free' of 'q'; freed at
(9)
|
I'm not convinced that it's useful to the end-user to warn about the
"use of q itself" case.
Dave
