[Bug analyzer/105087] New: fanalyzer double free false positive with vasprintf

[Bug analyzer/105087] New: fanalyzer double free false positive with vasprintf

[bcl at redhat dot com]

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

            Bug ID: 105087
           Summary: fanalyzer double free false positive with vasprintf
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: analyzer
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: bcl at redhat dot com
  Target Milestone: ---

Created attachment 52703
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52703&action=edit
Minimal reproducer main.c

While building parted I've run into what seems like a bug. I've created a
minimal reproducer which I'll attach. In parted we have a wrapper around
vasprintf called zasprintf, and it looks like the analyzer doesn't understand
that vasprintf returns a new allocation on each call which is passed back out
of the function.

Compiling it with 'gcc -o main -g -fanalyzer -fdump-analyzer -Wall ./main.c'

results in a complaint that there is a double free of buf:

./main.c: In function ‘run_test’:
./main.c:48:5: warning: double-‘free’ of ‘bar’ [CWE-415]
[-Wanalyzer-double-free]
   48 |     free(bar);
      |     ^~~~~~~~~
  ‘main’: events 1-2
    |
    |   54 | int main(int argc, char **argv) {
    |      |     ^~~~
    |      |     |
    |      |     (1) entry to ‘main’
    |   55 |     return run_test();
    |      |            ~~~~~~~~~~
    |      |            |
    |      |            (2) calling ‘run_test’ from ‘main’
    |
    +--> ‘run_test’: events 3-4
           |
           |   21 | int run_test() {
           |      |     ^~~~~~~~
           |      |     |
           |      |     (3) entry to ‘run_test’
           |......
           |   29 |     buf = zasprintf("i = %d", i);
           |      |           ~~~~~~~~~~~~~~~~~~~~~~
           |      |           |
           |      |           (4) calling ‘zasprintf’ from ‘run_test’
           |
           +--> ‘zasprintf’: events 5-7
                  |
                  |   11 | zasprintf (const char *format, ...)
                  |      | ^~~~~~~~~
                  |      | |
                  |      | (5) entry to ‘zasprintf’
                  |......
                  |   18 |   return r < 0 ? NULL : resultp;
                  |      |          ~~~~~~~~~~~~~~~~~~~~~~
                  |      |                       |
                  |      |                       (6) following ‘true’ branch
(when ‘r >= 0’)...
                  |      |                       (7) ...to here
                  |
           <------+
           |
         ‘run_test’: events 8-9
           |
           |   29 |     buf = zasprintf("i = %d", i);
           |      |           ^~~~~~~~~~~~~~~~~~~~~~
           |      |           |
           |      |           (8) returning to ‘run_test’ from ‘zasprintf’
           |......
           |   34 |     bar = zasprintf("i = %d - %d", i, i - 13);
           |      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |           |
           |      |           (9) calling ‘zasprintf’ from ‘run_test’
           |
           +--> ‘zasprintf’: events 10-12
                  |
                  |   11 | zasprintf (const char *format, ...)
                  |      | ^~~~~~~~~
                  |      | |
                  |      | (10) entry to ‘zasprintf’
                  |......
                  |   18 |   return r < 0 ? NULL : resultp;
                  |      |          ~~~~~~~~~~~~~~~~~~~~~~
                  |      |                       |
                  |      |                       (11) following ‘true’ branch
(when ‘r >= 0’)...
                  |      |                       (12) ...to here
                  |
           <------+
           |
         ‘run_test’: events 13-14
           |
           |   34 |     bar = zasprintf("i = %d - %d", i, i - 13);
           |      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |           |
           |      |           (13) returning to ‘run_test’ from ‘zasprintf’
           |......
           |   40 |     baz = zasprintf("No i's here");
           |      |           ~~~~~~~~~~~~~~~~~~~~~~~~
           |      |           |
           |      |           (14) calling ‘zasprintf’ from ‘run_test’
           |
           +--> ‘zasprintf’: events 15-17
                  |
                  |   11 | zasprintf (const char *format, ...)
                  |      | ^~~~~~~~~
                  |      | |
                  |      | (15) entry to ‘zasprintf’
                  |......
                  |   18 |   return r < 0 ? NULL : resultp;
                  |      |          ~~~~~~~~~~~~~~~~~~~~~~
                  |      |                       |
                  |      |                       (16) following ‘true’ branch
(when ‘r >= 0’)...
                  |      |                       (17) ...to here
                  |
           <------+
           |
         ‘run_test’: events 18-20
           |
           |   40 |     baz = zasprintf("No i's here");
           |      |           ^~~~~~~~~~~~~~~~~~~~~~~~
           |      |           |
           |      |           (18) returning to ‘run_test’ from ‘zasprintf’
           |......
           |   47 |     free(buf);
           |      |     ~~~~~~~~~
           |      |     |
           |      |     (19) first ‘free’ here
           |   48 |     free(bar);
           |      |     ~~~~~~~~~
           |      |     |
           |      |     (20) second ‘free’ here; first ‘free’ was at (19)
           |

[Bug analyzer/105087] fanalyzer double free false positive with vasprintf

[dmalcolm at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2022-03-28
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1

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

FWIW it's treating all three of buf, bar, and baz as having the same
conjured_svalue (and, surprisingly, from the __builtin_va_end call due to it
treating args as having escaped at the vasprintf call).

EN 386:
preds: EN: 378
succs: EN: 396
callstring: [(SN: 17 -> SN: 2 in main)]
before (SN: 15 stmt: 1): 
free (bar_15);
48 |     free(bar);
   |     ^~~~~~~~~

rmodel:
stack depth: 2
  frame (index 1): frame: ‘run_test’@2
  frame (index 0): frame: ‘main’@1
clusters within root region
  cluster for: (*INIT_VAL(argv)): CONJURED(__builtin_va_end (&args);,
(*INIT_VAL(argv))) (ESCAPED) (TOUCHED)
clusters within frame: ‘main’@1
  cluster for: _3: CONJURED(_3 = run_test ();, _3)
clusters within frame: ‘run_test’@2
  cluster for: bar_15: CONJURED(__builtin_va_end (&args);, resultp)
  cluster for: baz_19: CONJURED(__builtin_va_end (&args);, resultp)
m_called_unknown_fn: TRUE
constraint_manager:
  equiv classes:
  constraints:
malloc: 
  0x4f72180: CONJURED(__builtin_va_end (&args);, resultp): freed (‘bar_15’)

Looks like I need to "teach" -fanalyzer about vasprintf (and va_start/end, for
that matter)

[Bug analyzer/105087] fanalyzer double free false positive with vasprintf

[dmalcolm at gcc dot gnu.org]

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

--- Comment #2 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
#include "analyzer-decls.h"

extern void *inner_alloc (void);

void * __attribute__((noinline))
outer_alloc (void)
{
  return inner_alloc ();
}

void test_1 (void)
{
  void *p, *q;

  p = outer_alloc ();
  q = outer_alloc ();
  __analyzer_eval (p == q); // analyzer correctly thinks this is unknown
}

[Bug analyzer/105087] fanalyzer double free false positive with vasprintf

[dmalcolm at gcc dot gnu.org]

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

--- Comment #3 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
#include "analyzer-decls.h"

extern void inner_alloc (void **);

void * __attribute__((noinline))
outer_alloc (void)
{
  void *result;
  inner_alloc (&result);
  return result;
}

void test_1 (void)
{
  void *p, *q;

  p = outer_alloc ();
  q = outer_alloc ();
  __analyzer_eval (p == q); // bug: analyzer thinks this is true
}

[Bug analyzer/105087] fanalyzer double free false positive with vasprintf

[dmalcolm at gcc dot gnu.org]

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

--- Comment #4 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Am testing a fix.

[Bug analyzer/105087] fanalyzer double free false positive with vasprintf

[cvs-commit at gcc dot gnu.org]

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

--- Comment #5 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:3734527dfa0d10a50aee2f088d37320000fd65bf

commit r12-7869-g3734527dfa0d10a50aee2f088d37320000fd65bf
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Mon Mar 28 20:41:23 2022 -0400

    analyzer: ensure that we purge state when reusing a conjured_svalue
[PR105087]

    PR analyzer/105087 describes a false positive from
    -Wanalyzer-double-free in which the analyzer erroneously considers two
    successive inlined vasprintf calls to have allocated the same pointer.

    The root cause is that the result written back from vasprintf is a
    conjured_svalue, and that we normally purge state when reusing a
    conjured_svalue, but various places in the code were calling
    region_model_manager::get_or_create_conjured_svalue but failing to
    then call region_model::purge_state_involving on the result.

    This patch fixes things by moving responsibility for calling
    region_model::purge_state_involving into
    region_model_manager::get_or_create_conjured_svalue, so that it is
    always called when reusing a conjured_svalue, fixing the false positive.

    gcc/analyzer/ChangeLog:
            PR analyzer/105087
            * analyzer.h (class conjured_purge): New forward decl.
            * region-model-asm.cc (region_model::on_asm_stmt): Add
            conjured_purge param to calls binding_cluster::on_asm and
            region_model_manager::get_or_create_conjured_svalue.
            * region-model-impl-calls.cc
            (call_details::get_or_create_conjured_svalue): Likewise for call
            to region_model_manager::get_or_create_conjured_svalue.
            (region_model::impl_call_fgets): Remove call to
            region_model::purge_state_involving, as this is now done
            implicitly by call_details::get_or_create_conjured_svalue.
            (region_model::impl_call_fread): Likewise.
            (region_model::impl_call_strchr): Pass conjured_purge param to
            call to region_model_manager::get_or_create_conjured_svalue.
            * region-model-manager.cc (conjured_purge::purge): New.
            (region_model_manager::get_or_create_conjured_svalue): Add
            param "p".  Use it to purge state when reusing an existing
            conjured_svalue.
            * region-model.cc (region_model::on_call_pre): Replace call to
            region_model::purge_state_involving with passing conjured_purge
            to region_model_manager::get_or_create_conjured_svalue.
            (region_model::handle_unrecognized_call): Pass conjured_purge to
            store::on_unknown_fncall.
            * region-model.h
            (region_model_manager::get_or_create_conjured_svalue): Add param
            "p".
            * store.cc (binding_cluster::on_unknown_fncall): Likewise.  Pass
            it on to region_model_manager::get_or_create_conjured_svalue.
            (binding_cluster::on_asm): Likewise.
            (store::on_unknown_fncall): Add param "p" and pass it on to
            binding_cluster::on_unknown_fncall.
            * store.h (binding_cluster::on_unknown_fncall): Add param p.
            (binding_cluster::on_asm): Likewise.
            (store::on_unknown_fncall): Likewise.
            * svalue.h (class conjured_purge): New.

    gcc/testsuite/ChangeLog:
            * gcc.dg/analyzer/pr105087-1.c: New test.
            * gcc.dg/analyzer/pr105087-2.c: New test.
            * gcc.dg/analyzer/vasprintf-1.c: New test.

    Signed-off-by: David Malcolm <dmalcolm@redhat.com>

[Bug analyzer/105087] fanalyzer double free false positive with vasprintf

[dmalcolm at gcc dot gnu.org]

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

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

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

--- Comment #6 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Should be fixed on trunk for GCC 12 by the above commit.

Please let me know if you run into other false positives building parted with
-fanalyzer.