[Bug analyzer/104979] New: False positive from -Wanalyzer-malloc-leak with cast within boxed pointer

[Bug analyzer/104979] New: False positive from -Wanalyzer-malloc-leak with cast within boxed pointer

[dmalcolm at gcc dot gnu.org]

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

            Bug ID: 104979
           Summary: False positive from -Wanalyzer-malloc-leak with cast
                    within boxed pointer
           Product: gcc
           Version: 12.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:

#include <stdlib.h>

typedef struct boxed_ptr { void *value; } boxed_ptr;

boxed_ptr
boxed_malloc (size_t sz)
{
  boxed_ptr result;
  result.value = malloc (sz);
  return result;
}

boxed_ptr
boxed_free (boxed_ptr ptr)
{
  free (ptr.value);
}

const boxed_ptr boxed_null = {NULL};

struct link
{
  boxed_ptr m_ptr;
};

boxed_ptr test_29 (void)
{
  boxed_ptr res = boxed_malloc (sizeof (struct link));
  if (!res.value)
    return boxed_null;
  ((struct link *)res.value)->m_ptr = boxed_malloc (sizeof (struct link));
  return res;
}

-fanalyzer emits (incorrectly, I think):

<source>: In function 'boxed_malloc':
<source>:10:10: warning: leak of '<return-value>.value' [CWE-401]
[-Wanalyzer-malloc-leak]
   10 |   return result;
      |          ^~~~~~
  'test_29': events 1-4
    |
    |   26 | boxed_ptr test_29 (void)
    |      |           ^~~~~~~
    |      |           |
    |      |           (1) entry to 'test_29'
    |......
    |   29 |   if (!res.value)
    |      |      ~     
    |      |      |
    |      |      (2) following 'false' branch...
    |   30 |     return boxed_null;
    |   31 |   ((struct link *)res.value)->m_ptr = boxed_malloc (sizeof (struct
link));
    |      |                   ~~~~~~~~~          
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |                      |                |
    |      |                      |                (4) calling 'boxed_malloc'
from 'test_29'
    |      |                      (3) ...to here
    |
    +--> 'boxed_malloc': events 5-7
           |
           |    6 | boxed_malloc (size_t sz)
           |      | ^~~~~~~~~~~~
           |      | |
           |      | (5) entry to 'boxed_malloc'
           |......
           |    9 |   result.value = malloc (sz);
           |      |                  ~~~~~~~~~~~
           |      |                  |
           |      |                  (6) allocated here
           |   10 |   return result;
           |      |          ~~~~~~
           |      |          |
           |      |          (7) '<return-value>.value' leaks here; was
allocated at (6)
           |

https://godbolt.org/z/1e9n8dnvM

[Bug analyzer/104979] False positive from -Wanalyzer-malloc-leak with cast within boxed pointer

[cvs-commit at gcc dot gnu.org]

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

--- 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:4cebae0924248beb2077894c6dc725c306fc0a69

commit r12-7790-g4cebae0924248beb2077894c6dc725c306fc0a69
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Wed Mar 23 17:40:29 2022 -0400

    analyzer: fix accessing wrong stack frame on interprocedural return
[PR104979]

    PR analyzer/104979 reports a leak false positive when handling an
    interprocedural return to a caller:

      LHS = CALL(ARGS);

    where the LHS is a certain non-trivial compound expression.

    The root cause is that parts of the LHS were being erroneously
    evaluated with respect to the stack frame of the called function,
    rather than tha of the caller.  When LHS contained a local variable
    within the caller as part of certain nested expressions, this local
    variable was looked for within the called frame, rather than that of the
    caller.  This lookup in the wrong stack frame led to the local variable
    being treated as uninitialized, and thus the write to LHS was considered
    as writing to a garbage location, leading to the return value being
    lost, and thus being considered as a leak.

    The region_model code uses the analyzer's path_var class to try to
    extend the tree type with stack depth information.  Based on the above,
    I think that the path_var class is fundamentally broken, but it's used
    in a few other places in the analyzer, so I don't want to rip it out
    until the next stage 1.

    In the meantime, this patch reworks how region_model::pop_frame works so
    that the destination region for an interprocedural return value is
    computed after the frame is popped, so that the region_model has the
    stack frame for the *caller* at that point.  Doing so fixes the issue.

    I attempted a more ambitious fix which moved the storing of the return
    svalue into the destination region from region_model::pop_region into
    region_model::update_for_return_gcall, with pop_frame returning the
    return svalue.  Unfortunately, this regressed g++.dg/analyzer/pr93212.C,
    which returns a pointer into a stale frame.
    unbind_region_and_descendents and poison_any_pointers_to_descendents are
    only set up to poison regions with bindings into the stale frame, not
    individual svalues, and updating that became more invasive than I'm
    comfortable with in stage 4.

    The patch also adds assertions to verify that we have the correct
    function when looking up locals/SSA names in a stack frame.  There
    doesn't seem to be a general-purpose way to get at the function of an
    SSA name, so the assertions go from SSA name to def-stmt to basic_block,
    and from there use the analyzer's supergraph to get the function from
    the basic_block.  If there's a simpler way to do this, please let me know.

    gcc/analyzer/ChangeLog:
            PR analyzer/104979
            * engine.cc (impl_run_checkers): Create the engine after the
            supergraph, and pass the supergraph to the engine.
            * region-model.cc (region_model::get_lvalue_1): Pass ctxt to
            frame_region::get_region_for_local.
            (region_model::update_for_return_gcall): Pass the lvalue for the
            result to pop_frame as a tree, rather than as a region.
            (region_model::pop_frame): Update for above change, determining
            the destination region after the frame is popped and thus with
            respect to the caller frame rather than the called frame.
            Likewise, set the value of the region to the return value after
            the frame is popped.
            (engine::engine): Add supergraph pointer.
            (selftest::test_stack_frames): Set the DECL_CONTECT of PARM_DECLs.
            (selftest::test_get_representative_path_var): Likewise.
            (selftest::test_state_merging): Likewise.
            * region-model.h (region_model::pop_frame): Convert first param
            from a const region * to a tree.
            (engine::engine): Add param "sg".
            (engine::m_sg): New field.
            * region.cc: Include "analyzer/sm.h" and
            "analyzer/program-state.h".
            (frame_region::get_region_for_local): Add "ctxt" param.
            Add assertions that VAR_DECLs are locals, and that expr is for the
            correct function.
            * region.h (frame_region::get_region_for_local): Add "ctxt" param.

    gcc/testsuite/ChangeLog:
            PR analyzer/104979
            * gcc.dg/analyzer/boxed-malloc-1-29.c: Deleted test, moving the
            now fixed test_29 to...
            * gcc.dg/analyzer/boxed-malloc-1.c: ...here.
            * gcc.dg/analyzer/stale-frame-1.c: Add test coverage.

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

[Bug analyzer/104979] False positive from -Wanalyzer-malloc-leak with cast within boxed pointer

[dmalcolm at gcc dot gnu.org]

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

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

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

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