[Bug c/99193] New: Bogus "should have been deallocated with 'free' but was deallocated with 'realloc' [CWE-762] [-Werror=analyzer-mismatching-deallocation]"

[Bug c/99193] New: Bogus "should have been deallocated with 'free' but was deallocated with 'realloc' [CWE-762] [-Werror=analyzer-mismatching-deallocation]"

[rjones at redhat dot com]

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

            Bug ID: 99193
           Summary: Bogus "should have been deallocated with 'free' but
                    was deallocated with 'realloc' [CWE-762]
                    [-Werror=analyzer-mismatching-deallocation]"
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: rjones at redhat dot com
  Target Milestone: ---

https://github.com/libguestfs/libguestfs/blob/f19fd566f6387ce7e4d82409528c9dde374d25e0/daemon/command.c#L115

This fails to compile with:
gcc -DHAVE_CONFIG_H -I. -I..  -DCAML_NAME_SPACE -I/usr/lib64/ocaml
-I/usr/lib64/ocaml/hivex -I../gnulib/lib -I../gnulib/lib -I../lib -I../lib
-I../common/errnostring -I../common/errnostring -I../common/protocol
-I../common/protocol -I../common/utils -I../common/utils  -fanalyzer
-fno-common -Wall -Warith-conversion -Wbad-function-cast -Wcast-align=strict
-Wdate-time -Wdisabled-optimization -Wdouble-promotion -Wduplicated-branches
-Wduplicated-cond -Wextra -Wformat-signedness -Winit-self -Winvalid-pch
-Wlogical-op -Wmissing-declarations -Wmissing-include-dirs -Wmissing-prototypes
-Wnested-externs -Wnull-dereference -Wold-style-definition -Wopenmp-simd
-Wpointer-arith -Wstrict-overflow -Wstrict-prototypes -Wsuggest-attribute=cold
-Wsuggest-attribute=format -Wsuggest-attribute=noreturn -Wsuggest-final-methods
-Wsuggest-final-types -Wsync-nand -Wtrampolines -Wuninitialized
-Wunknown-pragmas -Wunused-macros -Wvariadic-macros
-Wvector-operation-performance -Wwrite-strings -Warray-bounds=2
-Wattribute-alias=2 -Wformat-overflow=2 -Wformat=2 -Wformat-truncation=2
-Wimplicit-fallthrough=5 -Wshift-overflow=2 -Wunused-const-variable=2
-Wno-analyzer-double-free -Wno-analyzer-malloc-leak
-Wno-analyzer-null-dereference -Wno-analyzer-use-after-free
-Wno-unused-parameter -Wno-missing-field-initializers -fdiagnostics-show-option
-Wframe-larger-than=6000 -Wstack-usage=10000 -Wimplicit-fallthrough=4
-Wformat-truncation=1 -Wformat-overflow=1 -Wno-pragmas -Werror
-I/usr/include/tirpc  -I/usr/include/libxml2      -O2 -g -fPIC
-fno-strict-overflow -Wno-strict-overflow -MT guestfsd-command.o -MD -MP -MF
.deps/guestfsd-command.Tpo -c -o guestfsd-command.o `test -f 'command.c' ||
echo './'`command.c

command.c: In function ‘commandrf’:
command.c:136:22: error: ‘argv’ should have been deallocated with ‘free’ but
was deallocated with ‘realloc’ [CWE-762]
[-Werror=analyzer-mismatching-deallocation]
  136 |     const char **p = realloc (argv, sizeof (char *) * (++i));
      |                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ‘commandrf’: events 1-4
    |
    |  125 |   argv = malloc (sizeof (char *) * i);
    |      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |          |
    |      |          (1) allocated here (expects deallocation with ‘free’)
    |  126 |   if (argv == NULL) {
    |      |      ~    
    |      |      |
    |      |      (2) assuming ‘argv’ is non-NULL
    |      |      (3) following ‘false’ branch...
    |......
    |  130 |   argv[0] = (char *) name;
    |      |   ~~~~    
    |      |   |
    |      |   (4) ...to here
    |
  ‘commandrf’: events 5-7
    |
    |  135 |   while ((s = va_arg (args, char *)) != NULL) {
    |      |                                      ^
    |      |                                      |
    |      |                                      (5) following ‘true’ branch
(when ‘s’ is non-NULL)...
    |  136 |     const char **p = realloc (argv, sizeof (char *) * (++i));
    |      |     ~~~~~            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |     |                |
    |      |     |                (7) deallocated with ‘realloc’ here;
allocation at (1) expects deallocation with ‘free’
    |      |     (6) ...to here
    |
cc1: all warnings being treated as errors
make[1]: *** [Makefile:3261: guestfsd-command.o] Error 1

This error appears to be bogus.

argv has __attribute__((cleanup)) which will call free(3) on return
paths out of the function, such as realloc failing.  If realloc
is successful then the old argv is freed and the new allocation
is assigned to argv.

[Bug c/99193] Bogus "should have been deallocated with 'free' but was deallocated with 'realloc' [CWE-762] [-Werror=analyzer-mismatching-deallocation]"

[rjones at redhat dot com]

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

--- Comment #1 from Richard W.M. Jones <rjones at redhat dot com> ---
Created attachment 50232
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50232&action=edit
preprocessed source (xz compressed)

[Bug c/99193] Bogus "should have been deallocated with 'free' but was deallocated with 'realloc' [CWE-762] [-Werror=analyzer-mismatching-deallocation]"

[rjones at redhat dot com]

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

Richard W.M. Jones <rjones at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Version|unknown                     |11.0

--- Comment #2 from Richard W.M. Jones <rjones at redhat dot com> ---
Version of GCC:

gcc-11.0.0-0.19.fc35.x86_64

[Bug c/99193] Bogus "should have been deallocated with 'free' but was deallocated with 'realloc' [CWE-762] [-Werror=analyzer-mismatching-deallocation]"

[rjones at redhat dot com]

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

--- Comment #3 from Richard W.M. Jones <rjones at redhat dot com> ---
This seems to be the same thing at a different place in the code:

https://github.com/libguestfs/libguestfs/blob/f19fd566f6387ce7e4d82409528c9dde374d25e0/df/main.c#L404

  CC       virt_df-main.o
main.c: In function 'make_display_name':
main.c:404:11: error: 'ret' should have been deallocated with 'free' but was
deallocated with 'realloc' [CWE-762]
[-Werror=analyzer-mismatching-deallocation]
  404 |     ret = realloc (ret, len + pluses + 1);
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  'make_display_name': event 1
    |
    |  378 | make_display_name (struct drv *drvs)
    |      | ^~~~~~~~~~~~~~~~~
    |      | |
    |      | (1) entry to 'make_display_name'
    |
  'make_display_name': event 2
    |
    |  382 |   assert (drvs != NULL);
    |      |   ^~~~~~
    |      |   |
    |      |   (2) following 'true' branch (when 'drvs' is non-NULL)...
    |
  'make_display_name': events 3-4
    |
    |  385 |   if (drvs->next == NULL)
    |      |   ^~ ~
    |      |   |  |
    |      |   |  (4) following 'false' branch...
    |      |   (3) ...to here
    |
  'make_display_name': event 5
    |
    |cc1:
    | (5): ...to here
    |
  'make_display_name': events 6-8
    |
    |  396 |     while (drvs->next != NULL) {
    |      |                       ^
    |      |                       |
    |      |                       (6) following 'true' branch...
    |  397 |       drvs = drvs->next;
    |      |       ~~~~             
    |      |       |
    |      |       (7) ...to here
    |......
    |  401 |     ret = single_drive_display_name (drvs);
    |      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |           |
    |      |           (8) calling 'single_drive_display_name' from
'make_display_name'
    |
    +--> 'single_drive_display_name': event 9
           |
           |  322 | single_drive_display_name (struct drv *drvs)
           |      | ^~~~~~~~~~~~~~~~~~~~~~~~~
           |      | |
           |      | (9) entry to 'single_drive_display_name'
           |
         'single_drive_display_name': event 10
           |
           |  327 |   assert (drvs != NULL);
           |      |   ^~~~~~
           |      |   |
           |      |   (10) following 'true' branch (when 'drvs' is non-NULL)...
           |
         'single_drive_display_name': event 11
           |
           |  328 |   assert (drvs->next == NULL);
           |      |   ^~~~~~
           |      |   |
           |      |   (11) ...to here
           |
         'single_drive_display_name': event 12
           |
           |  328 |   assert (drvs->next == NULL);
           |      |   ^~~~~~
           |      |   |
           |      |   (12) following 'true' branch...
           |
         'single_drive_display_name': events 13-19
           |
           |  330 |   switch (drvs->type) {
           |      |   ^~~~~~
           |      |   |
           |      |   (13) ...to here
           |      |   (14) following 'case 1:' branch...
           |......
           |  342 |   case drv_uri:
           |      |   ~~~~
           |      |   |
           |      |   (15) ...to here
           |  343 |     name = strdup (drvs->uri.orig_uri);
           |  344 |     if (name == NULL)
           |      |        ~
           |      |        |
           |      |        (16) following 'false' branch (when 'name' is
non-NULL)...
           |......
           |  349 |     p = strrchr (name, '/');
           |      |     ~
           |      |     |
           |      |     (17) ...to here
           |......
           |  370 |   if (!name)
           |      |      ~
           |      |      |
           |      |      (18) following 'false' branch (when 'name' is
non-NULL)...
           |......
           |  373 |   return name;
           |      |   ~~~~~~
           |      |   |
           |      |   (19) ...to here
           |
    <------+
    |
  'make_display_name': events 20-21
    |
    |  401 |     ret = single_drive_display_name (drvs);
    |      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |           |
    |      |           (20) returning to 'make_display_name' from
'single_drive_display_name'
    |......
    |  404 |     ret = realloc (ret, len + pluses + 1);
    |      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |           |
    |      |           (21) deallocated with 'realloc' here
    |
cc1: all warnings being treated as errors

[Bug c/99193] Bogus "should have been deallocated with 'free' but was deallocated with 'realloc' [CWE-762] [-Werror=analyzer-mismatching-deallocation]"

[rjones at redhat dot com]

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

--- Comment #4 from Richard W.M. Jones <rjones at redhat dot com> ---
And another:
https://github.com/libguestfs/libguestfs/blob/f19fd566f6387ce7e4d82409528c9dde374d25e0/daemon/debug.c#L115

debug.c: In function 'debug_help':
debug.c:129:9: error: 'r' should have been deallocated with 'free' but was
deallocated with 'realloc' [CWE-762]
[-Werror=analyzer-mismatching-deallocation]
  129 |     p = realloc (r, len + 1);        /* +1 for the final NUL */
      |         ^~~~~~~~~~~~~~~~~~~~
  'debug_help': events 1-4
    |
    |  120 |   r = strdup ("Commands supported:");
    |      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |       |
    |      |       (1) allocated here (expects deallocation with 'free')
    |  121 |   if (!r) {
    |      |      ~ 
    |      |      |
    |      |      (2) assuming 'r' is non-NULL
    |      |      (3) following 'false' branch (when 'r' is non-NULL)...
    |......
    |  126 |   len = strlen (r);
    |      |   ~~~  
    |      |   |
    |      |   (4) ...to here
    |
  'debug_help': events 5-7
    |
    |  127 |   for (i = 0; cmds[i].cmd != NULL; ++i) {
    |      |                           ^
    |      |                           |
    |      |                           (5) following 'true' branch...
    |  128 |     len += strlen (cmds[i].cmd) + 1; /* space + new command */
    |      |     ~~~                    
    |      |     |
    |      |     (6) ...to here
    |  129 |     p = realloc (r, len + 1);        /* +1 for the final NUL */
    |      |         ~~~~~~~~~~~~~~~~~~~~
    |      |         |
    |      |         (7) deallocated with 'realloc' here; allocation at (1)
expects deallocation with 'free'
    |
cc1: all warnings being treated as errors

[Bug analyzer/99193] Bogus "should have been deallocated with 'free' but was deallocated with 'realloc' [CWE-762] [-Werror=analyzer-mismatching-deallocation]"

[msebor at gcc dot gnu.org]

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

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |msebor at gcc dot gnu.org
           Assignee|unassigned at gcc dot gnu.org      |dmalcolm at gcc dot gnu.org
          Component|c                           |analyzer

--- Comment #5 from Martin Sebor <msebor at gcc dot gnu.org> ---
Reassigning to analyzer.

[Bug analyzer/99193] Bogus "should have been deallocated with 'free' but was deallocated with 'realloc' [CWE-762] [-Werror=analyzer-mismatching-deallocation]"

[dmalcolm at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2021-02-22

--- Comment #6 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Thanks for filing this; confirmed.  I'm working on a fix.

[Bug analyzer/99193] Bogus "should have been deallocated with 'free' but was deallocated with 'realloc' [CWE-762] [-Werror=analyzer-mismatching-deallocation]"

[dmalcolm at gcc dot gnu.org]

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

--- Comment #7 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
I'm testing a workaround for this; I've filed bug 99260 to cover other issues
with realloc(3) in the analyzer.

[Bug analyzer/99193] Bogus "should have been deallocated with 'free' but was deallocated with 'realloc' [CWE-762] [-Werror=analyzer-mismatching-deallocation]"

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:

https://gcc.gnu.org/g:a6baafcac5308be1a5d92c0b2a179495b7a24b52

commit r11-7381-ga6baafcac5308be1a5d92c0b2a179495b7a24b52
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Wed Feb 24 19:55:40 2021 -0500

    analyzer: fix false positive on realloc [PR99193]

    PR analyzer/99193 describes various false positives from
    -Wanalyzer-mismatching-deallocation on realloc(3) calls
    of the form:

        |   31 |   void *p = malloc (1024);
        |      |             ^~~~~~~~~~~~~
        |      |             |
        |      |             (1) allocated here (expects deallocation with
âfreeâ)
        |   32 |   void *q = realloc (p, 4096);
        |      |             ~~~~~~~~~~~~~~~~~
        |      |             |
        |      |             (2) deallocated with âreallocâ here;
allocation at (1) expects deallocation with âfreeâ
        |

    The underlying issue is that the analyzer has no knowledge of
    realloc(3), and realloc has awkward semantics.

    Unfortunately, the analyzer is currently structured so that each call
    statement can only have at most one successor state; there is no
    way to "bifurcate" the state, or have N-way splits into multiple
    outcomes.  The existing "on_stmt" code works on a copy of the next
    state, updating it in place, rather than copying it and making any
    necessary changes.  I did this as an optimization to avoid unnecessary
    copying of state objects, but it makes it hard to support multiple
    outcomes.  (ideally our state objects would be immutable and thus
    support trivial copying, alternatively, C++11 move semantics may
    help here)

    I attempted a few approaches to implementing bifurcation within the
    existing state-update framework, but they were messy and thus likely
    buggy; a proper implementation would rework state-updating to
    generate copies, but this would be a major change, and seems too
    late for GCC 11.

    As a workaround, this patch implements enough of realloc(3) to
    suppress the false positives.

    This fixes the false positives in PR analyzer/99193.
    I've filed PR analyzer/99260 to track "properly" implementing realloc(3).

    gcc/analyzer/ChangeLog:
            PR analyzer/99193
            * region-model-impl-calls.cc (region_model::impl_call_realloc):
New.
            * region-model.cc (region_model::on_call_pre): Call it.
            * region-model.h (region_model::impl_call_realloc): New decl.
            * sm-malloc.cc (enum wording): Add WORDING_REALLOCATED.
            (malloc_state_machine::m_realloc): New field.
            (use_after_free::describe_state_change): Add case for
            WORDING_REALLOCATED.
            (use_after_free::describe_final_event): Likewise.
            (malloc_state_machine::malloc_state_machine): Initialize
            m_realloc.
            (malloc_state_machine::on_stmt): Handle realloc by calling...
            (malloc_state_machine::on_realloc_call): New.

    gcc/testsuite/ChangeLog:
            PR analyzer/99193
            * gcc.dg/analyzer/pr99193-1.c: New test.
            * gcc.dg/analyzer/pr99193-2.c: New test.
            * gcc.dg/analyzer/pr99193-3.c: New test.
            * gcc.dg/analyzer/realloc-1.c: New test.

[Bug analyzer/99193] Bogus "should have been deallocated with 'free' but was deallocated with 'realloc' [CWE-762] [-Werror=analyzer-mismatching-deallocation]"

[dmalcolm at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #9 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
The false positive should be fixed by the above commit.

As noted above, this is just a workaround, and PR analyzer/99260 tracks
"properly" implementing realloc(3).

Marking this bug as resolved.

[Bug analyzer/99193] Bogus "should have been deallocated with 'free' but was deallocated with 'realloc' [CWE-762] [-Werror=analyzer-mismatching-deallocation]"

[rjones at redhat dot com]

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

--- Comment #10 from Richard W.M. Jones <rjones at redhat dot com> ---
Great stuff, I'll give this a go when GCC is updated
in Fedora Rawhide.

[Bug analyzer/99193] Bogus "should have been deallocated with 'free' but was deallocated with 'realloc' [CWE-762] [-Werror=analyzer-mismatching-deallocation]"

[dmalcolm at gcc dot gnu.org]

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

--- Comment #11 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
BTW, looking at the 
  #pragma GCC diagnostic ignored "-Wanalyzer-null-argument"
at
https://github.com/libguestfs/libguestfs/blob/f19fd566f6387ce7e4d82409528c9dde374d25e0/df/main.c#L317
I'm guessing that this was due to PR analyzer/99196, due to the:

    name = strdup (drvs->uri.orig_uri);
    if (name == NULL)
      error (EXIT_FAILURE, errno, "strdup");
    p = strrchr (name, '/');

where the analyzer didn't "know" that the error call would terminate the
program; so hopefully this is also fixed now.

[Bug analyzer/99193] Bogus "should have been deallocated with 'free' but was deallocated with 'realloc' [CWE-762] [-Werror=analyzer-mismatching-deallocation]"

[rjones at redhat dot com]

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

--- Comment #12 from Richard W.M. Jones <rjones at redhat dot com> ---
There are quite a lot of these - I will try removing them
when we get the updated GCC in Fedora.