analyzer: fix leak false +ve seen in haproxy's cfgparse.c [PR109059]
authorDavid Malcolm <dmalcolm@redhat.com>
Fri, 10 Mar 2023 16:55:44 +0000 (11:55 -0500)
committerDavid Malcolm <dmalcolm@redhat.com>
Fri, 10 Mar 2023 16:55:44 +0000 (11:55 -0500)
commit14f5e56a8a766c6f48c2a07b301fce2db1a19a3c
tree849a273f33dcfe0ece6f2d4ab737ae03b8cc656a
parent2b2340e236c0bba8aaca358ea25a5accd8249fbd
analyzer: fix leak false +ve seen in haproxy's cfgparse.c [PR109059]

If a bound region gets overwritten with UNKNOWN due to being
possibly-aliased during a write, that could have been the only
region keeping its value live, in which case we could falsely report
a leak.  This is hidden somewhat by the "uncertainty" mechanism for
cases where the write happens in the same stmt as the last reference
to the value goes away, but not in the general case, which occurs
in PR analyzer/109059, which falsely complains about a leak whilst
haproxy updates a doubly-linked list.

The whole "uncertainty_t" class seems broken to me now; I think we need
to track (in the store) what values could have escaped to the external
part of the program.  We do this to some extent for pointers by tracking
the region as escaped, though we're failing to do this for this case:
even though there could still be other pointers to the region,
eventually they go away; we want to capture the fact that the external
part of the state is still keeping it live.  Also, this doesn't work for
non-pointer svalues, such as for detecting file-descriptor leaks.

As both a workaround and a step towards eventually removing
"class uncertainty_t" this patch updates the "mark_region_as_unknown"
code called by possibly-aliased set_value so that when old values are
removed, any base region pointed to them is marked as escaped, fixing
the leak false positive.

The patch has this effect on my integration tests of -fanalyzer:

  Comparison:
    GOOD: 129        (19.20% -> 20.22%)
     BAD: 543 -> 509 (-34)

where there's a big improvement in -Wanalyzer-malloc-leak:

  -Wanalyzer-malloc-leak:
    GOOD: 61       (45.19% -> 54.95%)
     BAD: 74 -> 50 (-24)
     Known false positives: 25 -> 2 (-23)
       haproxy-2.7.1: 24 ->  1 (-23)
     Suspected false positives: 49 -> 48 (-1)
       coreutils-9.1: 32 -> 31 (-1)

and some churn in the other warnings:

  -Wanalyzer-use-of-uninitialized-value:
     GOOD: 0
      BAD: 81 -> 80 (-1)
  -Wanalyzer-file-leak:
     GOOD: 0
      BAD: 10 -> 11 (+1)
  -Wanalyzer-out-of-bounds:
     GOOD: 0
      BAD: 24 -> 22 (-2)

gcc/analyzer/ChangeLog:
PR analyzer/109059
* region-model.cc (region_model::mark_region_as_unknown): Gather a
set of maybe-live svalues and call on_maybe_live_values with it.
* store.cc (binding_map::remove_overlapping_bindings): Add new
"maybe_live_values" param; add any removed svalues to it.
(binding_cluster::clobber_region): Add NULL as new param of
remove_overlapping_bindings.
(binding_cluster::mark_region_as_unknown): Add "maybe_live_values"
param and pass it to remove_overlapping_bindings.
(binding_cluster::maybe_get_compound_binding): Add NULL for new
param of binding_map::remove_overlapping_bindings.
(binding_cluster::remove_overlapping_bindings): Add
"maybe_live_values" param and pass to
binding_map::remove_overlapping_bindings.
(store::set_value): Capture a set of maybe-live svalues, and call
on_maybe_live_values with it.
(store::on_maybe_live_values): New.
(store::mark_region_as_unknown): Add "maybe_live_values" param
and pass it to binding_cluster::mark_region_as_unknown.
(store::remove_overlapping_bindings): Pass NULL for new param of
binding_cluster::remove_overlapping_bindings.
* store.h (binding_map::remove_overlapping_bindings): Add
"maybe_live_values" param.
(binding_cluster::mark_region_as_unknown): Likewise.
(binding_cluster::remove_overlapping_bindings): Likewise.
(store::mark_region_as_unknown): Likewise.
(store::on_maybe_live_values): New decl.

gcc/testsuite/ChangeLog:
PR analyzer/109059
* gcc.dg/analyzer/flex-with-call-summaries.c: Remove xfail.
* gcc.dg/analyzer/leak-pr109059-1.c: New test.
* gcc.dg/analyzer/leak-pr109059-2.c: New test.

Signed-off-by: David Malcolm <dmalcolm@redhat.com>
gcc/analyzer/region-model.cc
gcc/analyzer/store.cc
gcc/analyzer/store.h
gcc/testsuite/gcc.dg/analyzer/flex-with-call-summaries.c
gcc/testsuite/gcc.dg/analyzer/leak-pr109059-1.c [new file with mode: 0644]
gcc/testsuite/gcc.dg/analyzer/leak-pr109059-2.c [new file with mode: 0644]