[Bug analyzer/109059] New: -Wanalyzer-malloc-leak false +ve seen on haproxy's cfgparse.c: cfg_register_postparser

[Bug analyzer/109059] New: -Wanalyzer-malloc-leak false +ve seen on haproxy's cfgparse.c: cfg_register_postparser

[dmalcolm at gcc dot gnu.org]

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

            Bug ID: 109059
           Summary: -Wanalyzer-malloc-leak false +ve seen on haproxy's
                    cfgparse.c: cfg_register_postparser
           Product: gcc
           Version: 13.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: ---

Given:
----------------------------------------------------------------------------
/* Reduced from haproxy-2.7.1's cfgparse.c.  */

typedef __SIZE_TYPE__ size_t;

extern void*
calloc(size_t __nmemb, size_t __size)
  __attribute__((__nothrow__, __leaf__))
  __attribute__((__malloc__)) __attribute__((__alloc_size__(1, 2)));

struct list
{
  struct list* n;
  struct list* p;
};

struct cfg_postparser
{
  struct list list;
  char* name;
  int (*func)();
};

extern struct list postparsers;

int
cfg_register_postparser(char* name, int (*func)())
{
  struct cfg_postparser* cp;

  cp = calloc(1, sizeof(*cp));
  if (!cp) {
    /* [...snip...] */
    return 0;
  }
  cp->name = name;
  cp->func = func;

  ({
    (&cp->list)->p = (&postparsers)->p;
    (&cp->list)->p->n = (&postparsers)->p = (&cp->list);
    (&cp->list)->n = (&postparsers);
    (&cp->list);
  });

  return 1;
}
----------------------------------------------------------------------------

...we currently emit this false positive:

----------------------------------------------------------------------------
../../src/haproxy-cfgparse-leak.c: In function ‘cfg_register_postparser’:
../../src/haproxy-cfgparse-leak.c:45:10: warning: leak of ‘cp’ [CWE-401]
[-Wanalyzer-malloc-leak] [num_events: 5]
   45 |   return 1;
      |          ^
  ‘cfg_register_postparser’: events 1-5
    |
    |   30 |   cp = calloc(1, sizeof(*cp));
    |      |        ^~~~~~~~~~~~~~~~~~~~~~
    |      |        |
    |      |        (1) allocated here
    |   31 |   if (!cp) {
    |      |      ~  
    |      |      |
    |      |      (2) assuming ‘cp’ is non-NULL
    |      |      (3) following ‘false’ branch (when ‘cp’ is non-NULL)...
    |......
    |   35 |   cp->name = name;
    |      |   ~~~~~~~~~~~~~~~
    |      |            |
    |      |            (4) ...to here
    |......
    |   45 |   return 1;
    |      |          ~
    |      |          |
    |      |          (5) ‘cp’ leaks here; was allocated at (1)
    |
----------------------------------------------------------------------------

   Trunk:  https://godbolt.org/z/WG6W16r1d
GCC 12.2: https://godbolt.org/z/d9afTcKqx
GCC 11.3: https://godbolt.org/z/aoaM7Mcze
GCC 10.4: https://godbolt.org/z/jY65dTccE

[Bug analyzer/109059] -Wanalyzer-malloc-leak false +ve seen on haproxy's cfgparse.c: cfg_register_postparser

[cvs-commit at gcc dot gnu.org]

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

--- 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:14f5e56a8a766c6f48c2a07b301fce2db1a19a3c

commit r13-6589-g14f5e56a8a766c6f48c2a07b301fce2db1a19a3c
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Fri Mar 10 11:55:44 2023 -0500

    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>

[Bug analyzer/109059] -Wanalyzer-malloc-leak false +ve seen on haproxy's cfgparse.c: cfg_register_postparser

[dmalcolm at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2023-03-10
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |ASSIGNED

--- Comment #2 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Fixed on trunk by the above patch.

Keeping open to track backporting this.

[Bug analyzer/109059] [11/12 Regression] -Wanalyzer-malloc-leak false +ve seen on haproxy's cfgparse.c: cfg_register_postparser

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |11.5