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

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

[David Malcolm]

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.

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
Pushed to trunk as r12-7790-g4cebae0924248b.

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>
---
 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.dg/analyzer/boxed-malloc-1-29.c       | 36 -------------
 .../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(-)
 delete mode 100644 gcc/testsuite/gcc.dg/analyzer/boxed-malloc-1-29.c

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;
+}
-- 
2.26.3