[Bug analyzer/106473] New: -Wanalyzer-malloc-leak false positive regression when returning heap-allocation through nested pointers

[Bug analyzer/106473] New: -Wanalyzer-malloc-leak false positive regression when returning heap-allocation through nested pointers

[raimue at codingfarm dot de]

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

            Bug ID: 106473
           Summary: -Wanalyzer-malloc-leak false positive regression when
                    returning heap-allocation through nested pointers
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: analyzer
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: raimue at codingfarm dot de
  Target Milestone: ---

Source:

void foo(char **args[], int *argc) {
    *argc = 1;
    (*args)[0] = __builtin_malloc(42);
}


Compiler output:

$ gcc-12 -Wall -fanalyzer -c -o foo.o foo.c
foo.c: In function 'foo':
foo.c:4:1: warning: leak of '<unknown>' [CWE-401] [-Wanalyzer-malloc-leak]
    4 | }
      | ^
  'foo': events 1-2
    |
    |    3 |     (*args)[0] = __builtin_malloc(42);
    |      |                  ^~~~~~~~~~~~~~~~~~~~
    |      |                  |
    |      |                  (1) allocated here
    |    4 | }
    |      | ~                 
    |      | |
    |      | (2) '<unknown>' leaks here; was allocated at (1)
    |


Notes:
This is only reported with the write to argc happening first, which should be
considered completely unrelated to args. Reordering the two statements resolves
the analyzer report.


Tested versions:

gcc 10.3: FAIL
gcc 11.2: OK
gcc 12.0: FAIL

I therefore consider this a regression as it was not reported by gcc 11.


Compiler Explorer link:
  https://gcc.godbolt.org/z/zGanPa3fs

[Bug analyzer/106473] [12/13 Regression] -Wanalyzer-malloc-leak false positive regression when returning heap-allocation through nested pointers

[dmalcolm at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
            Summary|-Wanalyzer-malloc-leak      |[12/13 Regression]
                   |false positive regression   |-Wanalyzer-malloc-leak
                   |when returning              |false positive regression
                   |heap-allocation through     |when returning
                   |nested pointers             |heap-allocation through
                   |                            |nested pointers
   Last reconfirmed|                            |2022-11-22
             Status|UNCONFIRMED                 |ASSIGNED

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

[Bug analyzer/106473] [12/13 Regression] -Wanalyzer-malloc-leak false positive regression when returning heap-allocation through nested pointers

[cvs-commit at gcc dot gnu.org]

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

--- Comment #2 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:ce917b0422c145779b83e005afd8433c0c86fb06

commit r13-4276-gce917b0422c145779b83e005afd8433c0c86fb06
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Wed Nov 23 20:43:33 2022 -0500

    analyzer: revamp of heap-allocated regions [PR106473]

    PR analyzer/106473 reports a false positive from -Wanalyzer-malloc-leak
    on:

      void foo(char **args[], int *argc) {
          *argc = 1;
          (*args)[0] = __builtin_malloc(42);
      }

    The issue is that at the write to *argc we don't know if argc could
    point within *args, and so we conservatiely set *args to be unknown.
    At the write "(*args)[0] = __builtin_malloc(42)" we have the result of
    the allocation written through an unknown pointer, so we mark the
    heap_allocated_region as having escaped.
    Unfortunately, within store::canonicalize we overzealously purge the
    heap allocated region, losing the information that it has escaped, and
    thus errnoeously report a leak.

    The first part of the fix is to update store::canonicalize so that it
    doesn't purge heap_allocated_regions that are marked as escaping.

    Doing so fixes the leak false positive, but leads to various state
    explosions relating to anywhere we have a malloc/free pair in a loop,
    where the analysis of the iteration appears to only have been reaching
    a fixed state due to a bug in the state merger code that was erroneously
    merging state about the region allocated in one iteration with that
    of another.  On touching that, the analyzer fails to reach a fixed state
    on any loops containing a malloc/free pair, since each analysis of a
    malloc was creating a new heap_allocated_region instance.

    Hence the second part of the fix is to revamp how heap_allocated_regions
    are managed within the analyzer.  Rather than create a new one at each
    analysis of a malloc call, instead we reuse them across the analysis,
    only creating a new one if the current path's state is referencing all
    of the existing ones.  Hence the heap_allocated_region instances get
    used in a fixed order along every analysis path, so e.g. at:

      if (flag)
        p = malloc (4096);
      else
        p = malloc (1024);

    both paths now use the same heap_allocated_region for their malloc
    calls - but we still end up with two enodes after the CFG merger, by
    rejecting merger of states with non-equal dynamic extents.

    gcc/analyzer/ChangeLog:
            PR analyzer/106473
            * call-summary.cc
            (call_summary_replay::convert_region_from_summary_1): Update for
            change to creation of heap-allocated regions.
            * program-state.cc (test_program_state_1): Likewise.
            (test_program_state_merging): Likewise.
            * region-model-impl-calls.cc (kf_calloc::impl_call_pre): Likewise.
            (kf_malloc::impl_call_pre): Likewise.
            (kf_operator_new::impl_call_pre): Likewise.
            (kf_realloc::impl_call_postsuccess_with_move::update_model):
Likewise.
            * region-model-manager.cc
            (region_model_manager::create_region_for_heap_alloc): Convert
            to...
            (region_model_manager::get_or_create_region_for_heap_alloc):
            ...this, reusing an existing region if it's unreferenced in the
            client state.
            * region-model-manager.h (region_model_manager::get_num_regions):
New.
             (region_model_manager::create_region_for_heap_alloc): Convert
to...
             (region_model_manager::get_or_create_region_for_heap_alloc):
...this.
            * region-model.cc (region_to_value_map::can_merge_with_p): Reject
            merger when the values are different.
            (region_model::create_region_for_heap_alloc): Convert to...
            (region_model::get_or_create_region_for_heap_alloc): ...this.
            (region_model::get_referenced_base_regions): New.
            (selftest::test_state_merging):  Update for change to creation of
            heap-allocated regions.
            (selftest::test_malloc_constraints): Likewise.
            (selftest::test_malloc): Likewise.
            * region-model.h: Include "sbitmap.h".
            (region_model::create_region_for_heap_alloc): Convert to...
            (region_model::get_or_create_region_for_heap_alloc): ...this.
            (region_model::get_referenced_base_regions): New decl.
            * store.cc (store::canonicalize): Don't purge a heap-allocated
region
            that's been marked as escaping.

    gcc/testsuite/ChangeLog:
            PR analyzer/106473
            * gcc.dg/analyzer/aliasing-pr106473.c: New test.
            * gcc.dg/analyzer/allocation-size-2.c: Add
            -fanalyzer-fine-grained".
            * gcc.dg/analyzer/allocation-size-3.c: Likewise.
            * gcc.dg/analyzer/explode-1.c: Mark leak with XFAIL.
            * gcc.dg/analyzer/explode-3.c: New test.
            * gcc.dg/analyzer/malloc-reuse.c: New test.

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

[Bug analyzer/106473] [12/13 Regression] -Wanalyzer-malloc-leak false positive regression when returning heap-allocation through nested pointers

[dmalcolm at gcc dot gnu.org]

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

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

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

--- Comment #3 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
This should be fixed on trunk for GCC 13 by the above patch.

Unfortunately the patch feels too invasive to backport to GCC 12.

I'm going to mark this as resolved.

[Bug analyzer/106473] [12/13 Regression] -Wanalyzer-malloc-leak false positive regression when returning heap-allocation through nested pointers

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |13.0
           Keywords|                            |diagnostic