Re: Missed warning (-Wuse-after-free)

[David Malcolm <dmalcolm@redhat.com>]

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