[Bug analyzer/99774] New: False positive from -Wanalyzer-malloc-leak in loop (qemu:libvhost-user.c)

[Bug analyzer/99774] New: False positive from -Wanalyzer-malloc-leak in loop (qemu:libvhost-user.c)

[dmalcolm at gcc dot gnu.org]

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

            Bug ID: 99774
           Summary: False positive from -Wanalyzer-malloc-leak in loop
                    (qemu:libvhost-user.c)
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: analyzer
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: dmalcolm at gcc dot gnu.org
  Target Milestone: ---

Created attachment 50472
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50472&action=edit
Reduced reproducer

I got a report about a false leak warning from the analyzer on some code in
qemu.

I'm attaching a reduced reproducer from qemu which triggers the issue:

$ ./xgcc -B. -S -fanalyzer ../../src/libvhost-user-1.c
../../src/libvhost-user-1.c: In function ‘vu_check_queue_inflights’:
../../src/libvhost-user-1.c:52:51: warning: leak of ‘*vq.resubmit_list’
[CWE-401] [-Wanalyzer-malloc-leak]
   52 |         vq->resubmit_list[vq->resubmit_num].index = i; /* { dg-bogus
"leak" } */
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
  ‘vu_check_queue_inflights’: events 1-11
    |
    |   44 |   if (vq->inuse) {
    |      |      ^
    |      |      |
    |      |      (1) following ‘true’ branch...
    |   45 |     vq->resubmit_list = calloc(vq->inuse,
sizeof(VuVirtqInflightDesc));
    |      |                        
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |                         |        |
    |      |                         |        (2) ...to here
    |      |                         (3) allocated here
    |   46 |     if (!vq->resubmit_list) {
    |      |        ~
    |      |        |
    |      |        (4) assuming ‘*vq.resubmit_list’ is non-NULL
    |      |        (5) following ‘false’ branch...
    |......
    |   50 |     for (i = 0; i < vq->inflight->desc_num; i++) {
    |      |          ~~~~~  ~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |            |      |
    |      |            |      (7) following ‘true’ branch...
    |      |            |      (9) following ‘true’ branch...
    |      |            (6) ...to here
    |   51 |       if (vq->inflight->desc[i].inflight) {
    |      |           ~~~~~~~~~~~~
    |      |             |
    |      |             (8) ...to here
    |      |             (10) ...to here
    |   52 |         vq->resubmit_list[vq->resubmit_num].index = i; /* {
dg-bogus "leak" } */
    |      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |                                                   |
    |      |                                                   (11)
‘*vq.resubmit_list’ leaks here; was allocated at (3)
    |

[Bug analyzer/99774] False positive from -Wanalyzer-malloc-leak in loop (qemu:libvhost-user.c)

[dmalcolm at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2021-03-25
             Status|UNCONFIRMED                 |ASSIGNED

[Bug analyzer/99774] False positive from -Wanalyzer-malloc-leak in loop (qemu:libvhost-user.c)

[cvs-commit at gcc dot gnu.org]

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

--- Comment #1 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:3a66c289a3f395e50de79424e1e6f401a4dc1ab7

commit r11-8046-g3a66c289a3f395e50de79424e1e6f401a4dc1ab7
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Thu Apr 8 09:46:03 2021 -0400

    analyzer: fix leak false +ves due to maybe-clobbered regions
[PR99042,PR99774]

    Prior to this patch, program_state::detect_leaks worked by finding all
    live svalues in the old state and in the new state, and calling
    on_svalue_leak for each svalue that has changed from being live to
    not being live.

    PR analyzer/99042 and PR analyzer/99774 both describe false leak
    diagnostics from -fanalyzer (a false FILE * leak in git, and a false
    malloc leak in qemu, respectively).

    In both cases the root cause of the false leak diagnostic relates to
    svalues no longer being explicitly bound in the store due to regions
    being conservatively clobbered, due to an unknown function being
    called, or due to a write through a pointer that could alias the
    region, respectively.

    We have a transition from an svalue being explicitly live to not
    being explicitly live - but only because the store is being
    conservative, clobbering the binding.  The leak detection is looking
    for transitions from "definitely live" to "not definitely live",
    when it should be looking for transitions from "definitely live"
    to "definitely not live".

    This patch introduces a new class to temporarily capture information
    about svalues that were explicitly live, but for which a region bound
    to them got clobbered for conservative reasons.  This new
    "uncertainty_t" class is passed around to capture the data long enough
    for use in program_state::detect_leaks, where it is used to only
    complain about svalues that were definitely live and are now both
    not definitely live *or* possibly-live i.e. definitely not-live.

    The class also captures for which svalues we can't meaningfully track
    sm-state anymore, and resets the svalues back to the "start" state.

    Together, these changes fix the false leak reports.

    gcc/analyzer/ChangeLog:
            PR analyzer/99042
            PR analyzer/99774
            * engine.cc
            (impl_region_model_context::impl_region_model_context): Add
            uncertainty param and use it to initialize m_uncertainty.
            (impl_region_model_context::get_uncertainty): New.
            (impl_sm_context::get_fndecl_for_call): Add NULL for new
            uncertainty param when constructing impl_region_model_context.
            (impl_sm_context::get_state): Likewise.
            (impl_sm_context::set_next_state): Likewise.
            (impl_sm_context::warn): Likewise.
            (exploded_node::on_stmt): Add uncertainty param
            and use it when constructing impl_region_model_context.
            (exploded_node::on_edge): Add uncertainty param and pass
            to on_edge call.
            (exploded_node::detect_leaks): Create uncertainty_t and pass to
            impl_region_model_context.
            (exploded_graph::get_or_create_node): Create uncertainty_t and
            pass to prune_for_point.
            (maybe_process_run_of_before_supernode_enodes): Create
            uncertainty_t and pass to impl_region_model_context.
            (exploded_graph::process_node): Create uncertainty_t instances and
            pass around as needed.
            * exploded-graph.h
            (impl_region_model_context::impl_region_model_context): Add
            uncertainty param.
            (impl_region_model_context::get_uncertainty): New decl.
            (impl_region_model_context::m_uncertainty): New field.
            (exploded_node::on_stmt): Add uncertainty param.
            (exploded_node::on_edge): Likewise.
            * program-state.cc (sm_state_map::on_liveness_change): Get
            uncertainty from context and use it to unset sm-state from
            svalues as appropriate.
            (program_state::on_edge): Add uncertainty param and use it when
            constructing impl_region_model_context.  Fix indentation.
            (program_state::prune_for_point): Add uncertainty param and use it
            when constructing impl_region_model_context.
            (program_state::detect_leaks): Get any uncertainty from ctxt and
            use it to get maybe-live svalues for dest_state, rather than
            definitely-live ones; use this when determining which svalues
            have leaked.
            (selftest::test_program_state_merging): Create uncertainty_t and
            pass to impl_region_model_context.
            * program-state.h (program_state::on_edge): Add uncertainty param.
            (program_state::prune_for_point): Likewise.
            * region-model-impl-calls.cc (call_details::get_uncertainty): New.
            (region_model::impl_call_memcpy): Pass uncertainty to
            mark_region_as_unknown call.
            (region_model::impl_call_memset): Likewise.
            (region_model::impl_call_strcpy): Likewise.
            * region-model-reachability.cc (reachable_regions::handle_sval):
            Also add sval to m_mutable_svals.
            * region-model.cc (region_model::on_assignment): Pass any
            uncertainty from ctxt to the store::set_value call.
            (region_model::handle_unrecognized_call): Get any uncertainty from
            ctxt and use it to record mutable svalues at the unknown call.
            (region_model::get_reachable_svalues): Add uncertainty param and
            use it to mark any maybe-bound svalues as being reachable.
            (region_model::set_value): Pass any uncertainty from ctxt to the
            store::set_value call.
            (region_model::mark_region_as_unknown): Add uncertainty param and
            pass it on to the store::mark_region_as_unknown call.
            (region_model::update_for_call_summary): Add uncertainty param and
            pass it on to the region_model::mark_region_as_unknown call.
            * region-model.h (call_details::get_uncertainty): New decl.
            (region_model::get_reachable_svalues): Add uncertainty param.
            (region_model::mark_region_as_unknown): Add uncertainty param.
            (region_model_context::get_uncertainty): New vfunc.
            (noop_region_model_context::get_uncertainty): New vfunc
            implementation.
            * store.cc (dump_svalue_set): New.
            (uncertainty_t::dump_to_pp): New.
            (uncertainty_t::dump): New.
            (binding_cluster::clobber_region): Pass NULL for uncertainty to
            remove_overlapping_bindings.
            (binding_cluster::mark_region_as_unknown): Add uncertainty param
            and pass it to remove_overlapping_bindings.
            (binding_cluster::remove_overlapping_bindings): Add uncertainty
param.
            Use it to record any svalues that were in clobbered bindings.
            (store::set_value): Add uncertainty param.  Pass it to
            binding_cluster::mark_region_as_unknown when handling symbolic
            regions.
            (store::mark_region_as_unknown): Add uncertainty param and pass it
            to binding_cluster::mark_region_as_unknown.
            (store::remove_overlapping_bindings): Add uncertainty param and
            pass it to binding_cluster::remove_overlapping_bindings.
            * store.h (binding_cluster::mark_region_as_unknown): Add
            uncertainty param.
            (binding_cluster::remove_overlapping_bindings): Likewise.
            (store::set_value): Likewise.
            (store::mark_region_as_unknown): Likewise.

    gcc/testsuite/ChangeLog:
            PR analyzer/99042
            PR analyzer/99774
            * gcc.dg/analyzer/pr99042.c: New test.
            * gcc.dg/analyzer/pr99774-1.c: New test.
            * gcc.dg/analyzer/pr99774-2.c: New test.

[Bug analyzer/99774] False positive from -Wanalyzer-malloc-leak in loop (qemu:libvhost-user.c)

[dmalcolm at gcc dot gnu.org]

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

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

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

--- Comment #2 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Should be fixed by the above commit.