[gcc r12-7790] analyzer: fix accessing wrong stack frame on interprocedural return [PR104979]

[gcc r12-7790] analyzer: fix accessing wrong stack frame on interprocedural return [PR104979]

[David Malcolm]

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>

Diff:
---
 gcc/analyzer/engine.cc                            |  4 +-
 gcc/analyzer/region-model.cc                      | 50 +++++++++++++----------
 gcc/analyzer/region-model.h                       |  7 ++--
 gcc/analyzer/region.cc                            | 50 ++++++++++++++++++++---
 gcc/analyzer/region.h                             |  6 ++-
 gcc/testsuite/gcc.dg/analyzer/boxed-malloc-1-29.c | 36 ----------------
 gcc/testsuite/gcc.dg/analyzer/boxed-malloc-1.c    |  9 ++++
 gcc/testsuite/gcc.dg/analyzer/stale-frame-1.c     | 29 +++++++++++++
 8 files changed, 120 insertions(+), 71 deletions(-)

diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc
index caa8796b494..0f40e064a22 100644
--- a/gcc/analyzer/engine.cc
+++ b/gcc/analyzer/engine.cc
@@ -5717,11 +5717,11 @@ impl_run_checkers (logger *logger)
   FOR_EACH_FUNCTION_WITH_GIMPLE_BODY (node)
     node->get_untransformed_body ();
 
-  engine eng (logger);
-
   /* Create the supergraph.  */
   supergraph sg (logger);
 
+  engine eng (&sg, logger);
+
   state_purge_map *purge_map = NULL;
 
   if (flag_analyzer_state_purge)
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 29d3122edb3..44bd8026a35 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -2081,7 +2081,7 @@ region_model::get_lvalue_1 (path_var pv, region_model_context *ctxt) const
 	int stack_index = pv.m_stack_depth;
 	const frame_region *frame = get_frame_at_index (stack_index);
 	gcc_assert (frame);
-	return frame->get_region_for_local (m_mgr, expr);
+	return frame->get_region_for_local (m_mgr, expr, ctxt);
       }
 
     case COMPONENT_REF:
@@ -3696,20 +3696,11 @@ void
 region_model::update_for_return_gcall (const gcall *call_stmt,
 				       region_model_context *ctxt)
 {
-  /* Get the region for the result of the call, within the caller frame.  */
-  const region *result_dst_reg = NULL;
+  /* Get the lvalue for the result of the call, passing it to pop_frame,
+     so that pop_frame can determine the region with respect to the
+     *caller* frame.  */
   tree lhs = gimple_call_lhs (call_stmt);
-  if (lhs)
-    {
-      /* Normally we access the top-level frame, which is:
-         path_var (expr, get_stack_depth () - 1)
-         whereas here we need the caller frame, hence "- 2" here.  */
-      gcc_assert (get_stack_depth () >= 2);
-      result_dst_reg = get_lvalue (path_var (lhs, get_stack_depth () - 2),
-           			   ctxt);
-    }
-
-  pop_frame (result_dst_reg, NULL, ctxt);
+  pop_frame (lhs, NULL, ctxt);
 }
 
 /* Extract calling information from the superedge and update the model for the 
@@ -3928,8 +3919,9 @@ region_model::get_current_function () const
 
 /* Pop the topmost frame_region from this region_model's stack;
 
-   If RESULT_DST_REG is non-null, copy any return value from the frame
-   into RESULT_DST_REG's region.
+   If RESULT_LVALUE is non-null, copy any return value from the frame
+   into the corresponding region (evaluated with respect to the *caller*
+   frame, rather than the called frame).
    If OUT_RESULT is non-null, copy any return value from the frame
    into *OUT_RESULT.
 
@@ -3938,7 +3930,7 @@ region_model::get_current_function () const
    POISON_KIND_POPPED_STACK svalues.  */
 
 void
-region_model::pop_frame (const region *result_dst_reg,
+region_model::pop_frame (tree result_lvalue,
 			 const svalue **out_result,
 			 region_model_context *ctxt)
 {
@@ -3948,11 +3940,10 @@ region_model::pop_frame (const region *result_dst_reg,
   const frame_region *frame_reg = m_current_frame;
   tree fndecl = m_current_frame->get_function ()->decl;
   tree result = DECL_RESULT (fndecl);
+  const svalue *retval = NULL;
   if (result && TREE_TYPE (result) != void_type_node)
     {
-      const svalue *retval = get_rvalue (result, ctxt);
-      if (result_dst_reg)
-	set_value (result_dst_reg, retval, ctxt);
+      retval = get_rvalue (result, ctxt);
       if (out_result)
 	*out_result = retval;
     }
@@ -3960,6 +3951,14 @@ region_model::pop_frame (const region *result_dst_reg,
   /* Pop the frame.  */
   m_current_frame = m_current_frame->get_calling_frame ();
 
+  if (result_lvalue && retval)
+    {
+      /* Compute result_dst_reg using RESULT_LVALUE *after* popping
+	 the frame, but before poisoning pointers into the old frame.  */
+      const region *result_dst_reg = get_lvalue (result_lvalue, ctxt);
+      set_value (result_dst_reg, retval, ctxt);
+    }
+
   unbind_region_and_descendents (frame_reg,POISON_KIND_POPPED_STACK);
 }
 
@@ -4366,8 +4365,8 @@ rejected_ranges_constraint::dump_to_pp (pretty_printer *pp) const
 
 /* engine's ctor.  */
 
-engine::engine (logger *logger)
-: m_mgr (logger)
+engine::engine (const supergraph *sg, logger *logger)
+: m_sg (sg), m_mgr (logger)
 {
 }
 
@@ -5138,16 +5137,20 @@ test_stack_frames ()
   tree a = build_decl (UNKNOWN_LOCATION, PARM_DECL,
 		       get_identifier ("a"),
 		       integer_type_node);
+  DECL_CONTEXT (a) = parent_fndecl;
   tree b = build_decl (UNKNOWN_LOCATION, PARM_DECL,
 		       get_identifier ("b"),
 		       integer_type_node);
+  DECL_CONTEXT (b) = parent_fndecl;
   /* "x" and "y" in a child frame.  */
   tree x = build_decl (UNKNOWN_LOCATION, PARM_DECL,
 		       get_identifier ("x"),
 		       integer_type_node);
+  DECL_CONTEXT (x) = child_fndecl;
   tree y = build_decl (UNKNOWN_LOCATION, PARM_DECL,
 		       get_identifier ("y"),
 		       integer_type_node);
+  DECL_CONTEXT (y) = child_fndecl;
 
   /* "p" global.  */
   tree p = build_global_decl ("p", ptr_type_node);
@@ -5258,6 +5261,7 @@ test_get_representative_path_var ()
   tree n = build_decl (UNKNOWN_LOCATION, PARM_DECL,
 		       get_identifier ("n"),
 		       integer_type_node);
+  DECL_CONTEXT (n) = fndecl;
 
   region_model_manager mgr;
   test_region_model_context ctxt;
@@ -5470,12 +5474,14 @@ test_state_merging ()
   tree a = build_decl (UNKNOWN_LOCATION, PARM_DECL,
 		       get_identifier ("a"),
 		       integer_type_node);
+  DECL_CONTEXT (a) = test_fndecl;
   tree addr_of_a = build1 (ADDR_EXPR, ptr_type_node, a);
 
   /* Param "q", a pointer.  */
   tree q = build_decl (UNKNOWN_LOCATION, PARM_DECL,
 		       get_identifier ("q"),
 		       ptr_type_node);
+  DECL_CONTEXT (q) = test_fndecl;
 
   program_point point (program_point::origin ());
   region_model_manager mgr;
diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h
index 8d92d7c97a3..d9e21432032 100644
--- a/gcc/analyzer/region-model.h
+++ b/gcc/analyzer/region-model.h
@@ -658,7 +658,7 @@ class region_model
 			    region_model_context *ctxt);
   const frame_region *get_current_frame () const { return m_current_frame; }
   function * get_current_function () const;
-  void pop_frame (const region *result_dst,
+  void pop_frame (tree result_lvalue,
 		  const svalue **out_result,
 		  region_model_context *ctxt);
   int get_stack_depth () const;
@@ -1262,14 +1262,15 @@ private:
 class engine
 {
 public:
-  engine (logger *logger = NULL);
+  engine (const supergraph *sg = NULL, logger *logger = NULL);
+  const supergraph *get_supergraph () { return m_sg; }
   region_model_manager *get_model_manager () { return &m_mgr; }
 
   void log_stats (logger *logger) const;
 
 private:
+  const supergraph *m_sg;
   region_model_manager m_mgr;
-
 };
 
 } // namespace ana
diff --git a/gcc/analyzer/region.cc b/gcc/analyzer/region.cc
index 5ac24fb9f9b..2020ef4e4b5 100644
--- a/gcc/analyzer/region.cc
+++ b/gcc/analyzer/region.cc
@@ -59,6 +59,8 @@ along with GCC; see the file COPYING3.  If not see
 #include "analyzer/store.h"
 #include "analyzer/region.h"
 #include "analyzer/region-model.h"
+#include "analyzer/sm.h"
+#include "analyzer/program-state.h"
 
 #if ENABLE_ANALYZER
 
@@ -842,13 +844,49 @@ frame_region::dump_to_pp (pretty_printer *pp, bool simple) const
 
 const decl_region *
 frame_region::get_region_for_local (region_model_manager *mgr,
-				    tree expr) const
+				    tree expr,
+				    const region_model_context *ctxt) const
 {
-  // TODO: could also check that VAR_DECLs are locals
-  gcc_assert (TREE_CODE (expr) == PARM_DECL
-	      || TREE_CODE (expr) == VAR_DECL
-	      || TREE_CODE (expr) == SSA_NAME
-	      || TREE_CODE (expr) == RESULT_DECL);
+  if (CHECKING_P)
+    {
+      /* Verify that EXPR is a local or SSA name, and that it's for the
+	 correct function for this stack frame.  */
+      gcc_assert (TREE_CODE (expr) == PARM_DECL
+		  || TREE_CODE (expr) == VAR_DECL
+		  || TREE_CODE (expr) == SSA_NAME
+		  || TREE_CODE (expr) == RESULT_DECL);
+      switch (TREE_CODE (expr))
+	{
+	default:
+	  gcc_unreachable ();
+	case VAR_DECL:
+	  gcc_assert (!is_global_var (expr));
+	  /* Fall through.  */
+	case PARM_DECL:
+	case RESULT_DECL:
+	  gcc_assert (DECL_CONTEXT (expr) == m_fun->decl);
+	  break;
+	case SSA_NAME:
+	  {
+	    if (tree var = SSA_NAME_VAR (expr))
+	      {
+		if (DECL_P (var))
+		  gcc_assert (DECL_CONTEXT (var) == m_fun->decl);
+	      }
+	    else if (ctxt)
+	      if (const extrinsic_state *ext_state = ctxt->get_ext_state ())
+		if (const supergraph *sg
+		    = ext_state->get_engine ()->get_supergraph ())
+		  {
+		    const gimple *def_stmt = SSA_NAME_DEF_STMT (expr);
+		    const supernode *snode
+		      = sg->get_supernode_for_stmt (def_stmt);
+		    gcc_assert (snode->get_function () == m_fun);
+		  }
+	  }
+	  break;
+	}
+    }
 
   /* Ideally we'd use mutable here.  */
   map_t &mutable_locals = const_cast <map_t &> (m_locals);
diff --git a/gcc/analyzer/region.h b/gcc/analyzer/region.h
index 2f987e49fa8..dbeb485e81f 100644
--- a/gcc/analyzer/region.h
+++ b/gcc/analyzer/region.h
@@ -312,8 +312,10 @@ public:
   int get_index () const { return m_index; }
   int get_stack_depth () const { return m_index + 1; }
 
-  const decl_region *get_region_for_local (region_model_manager *mgr,
-					   tree expr) const;
+  const decl_region *
+  get_region_for_local (region_model_manager *mgr,
+			tree expr,
+			const region_model_context *ctxt) const;
 
   unsigned get_num_locals () const { return m_locals.elements (); }
 
diff --git a/gcc/testsuite/gcc.dg/analyzer/boxed-malloc-1-29.c b/gcc/testsuite/gcc.dg/analyzer/boxed-malloc-1-29.c
deleted file mode 100644
index 9e38f97fc8e..00000000000
--- a/gcc/testsuite/gcc.dg/analyzer/boxed-malloc-1-29.c
+++ /dev/null
@@ -1,36 +0,0 @@
-/* Isolating this false positive from boxed-malloc-1.c since it's
-   reported within boxed_malloc.  */
-
-#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; /* { dg-bogus "leak" "leak false +ve (PR analyzer/104979)" { xfail *-*-* } } */
-}
-
-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;
-}
diff --git a/gcc/testsuite/gcc.dg/analyzer/boxed-malloc-1.c b/gcc/testsuite/gcc.dg/analyzer/boxed-malloc-1.c
index 5428f2baf49..435fb4f01c3 100644
--- a/gcc/testsuite/gcc.dg/analyzer/boxed-malloc-1.c
+++ b/gcc/testsuite/gcc.dg/analyzer/boxed-malloc-1.c
@@ -334,6 +334,15 @@ 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;
+}
+
 void test_31 (void)
 {
   struct link tmp;
diff --git a/gcc/testsuite/gcc.dg/analyzer/stale-frame-1.c b/gcc/testsuite/gcc.dg/analyzer/stale-frame-1.c
index 04221479bf9..c3b7186e97a 100644
--- a/gcc/testsuite/gcc.dg/analyzer/stale-frame-1.c
+++ b/gcc/testsuite/gcc.dg/analyzer/stale-frame-1.c
@@ -13,3 +13,32 @@ int test_1 (void)
   called_by_test_1 ();
   return *global_ptr; /* { dg-warning "dereferencing pointer 'global_ptr' to within stale stack frame" } */
 }
+
+static void __attribute__((noinline))
+called_by_test_2 (int **out)
+{
+  int i = 42;
+  *out = &i;  
+}
+
+int test_2 (void)
+{
+  int *ptr;
+  called_by_test_2 (&ptr);
+  return *ptr; /* { dg-warning "dereferencing pointer 'ptr' to within stale stack frame" } */
+}
+
+static int __attribute__((noinline))
+called_by_test_3 (int **out)
+{
+  int i = 42;
+  *out = &i;
+  return i;
+}
+
+int test_3 (void)
+{
+  int *lhs_ptr;
+  *lhs_ptr = called_by_test_3 (&lhs_ptr); /* { dg-warning "use of uninitialized value 'lhs_ptr'" } */
+  return *lhs_ptr;
+}