analyzer: ensure that we purge state when reusing a conjured_svalue [PR105087]

PR analyzer/105087 describes a false positive from
-Wanalyzer-double-free in which the analyzer erroneously considers two
successive inlined vasprintf calls to have allocated the same pointer.

The root cause is that the result written back from vasprintf is a
conjured_svalue, and that we normally purge state when reusing a
conjured_svalue, but various places in the code were calling
region_model_manager::get_or_create_conjured_svalue but failing to
then call region_model::purge_state_involving on the result.

This patch fixes things by moving responsibility for calling
region_model::purge_state_involving into
region_model_manager::get_or_create_conjured_svalue, so that it is
always called when reusing a conjured_svalue, fixing the false positive.

gcc/analyzer/ChangeLog:
	PR analyzer/105087
	* analyzer.h (class conjured_purge): New forward decl.
	* region-model-asm.cc (region_model::on_asm_stmt): Add
	conjured_purge param to calls binding_cluster::on_asm and
	region_model_manager::get_or_create_conjured_svalue.
	* region-model-impl-calls.cc
	(call_details::get_or_create_conjured_svalue): Likewise for call
	to region_model_manager::get_or_create_conjured_svalue.
	(region_model::impl_call_fgets): Remove call to
	region_model::purge_state_involving, as this is now done
	implicitly by call_details::get_or_create_conjured_svalue.
	(region_model::impl_call_fread): Likewise.
	(region_model::impl_call_strchr): Pass conjured_purge param to
	call to region_model_manager::get_or_create_conjured_svalue.
	* region-model-manager.cc (conjured_purge::purge): New.
	(region_model_manager::get_or_create_conjured_svalue): Add
	param "p".  Use it to purge state when reusing an existing
	conjured_svalue.
	* region-model.cc (region_model::on_call_pre): Replace call to
	region_model::purge_state_involving with passing conjured_purge
	to region_model_manager::get_or_create_conjured_svalue.
	(region_model::handle_unrecognized_call): Pass conjured_purge to
	store::on_unknown_fncall.
	* region-model.h
	(region_model_manager::get_or_create_conjured_svalue): Add param
	"p".
	* store.cc (binding_cluster::on_unknown_fncall): Likewise.  Pass
	it on to region_model_manager::get_or_create_conjured_svalue.
	(binding_cluster::on_asm): Likewise.
	(store::on_unknown_fncall): Add param "p" and pass it on to
	binding_cluster::on_unknown_fncall.
	* store.h (binding_cluster::on_unknown_fncall): Add param p.
	(binding_cluster::on_asm): Likewise.
	(store::on_unknown_fncall): Likewise.
	* svalue.h (class conjured_purge): New.

gcc/testsuite/ChangeLog:
	* gcc.dg/analyzer/pr105087-1.c: New test.
	* gcc.dg/analyzer/pr105087-2.c: New test.
	* gcc.dg/analyzer/vasprintf-1.c: New test.

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

9 files changed, 85 insertions, 27 deletions

diff --git a/gcc/analyzer/analyzer.h b/gcc/analyzer/analyzer.h
index a8289d0a..39934a3 100644
--- a/gcc/analyzer/analyzer.h
+++ b/gcc/analyzer/analyzer.h
@@ -70,6 +70,7 @@ class region;
   class string_region;
   class bit_range_region;
 class region_model_manager;
+class conjured_purge;
 struct model_merger;
 class store_manager;
 class store;
diff --git a/gcc/analyzer/region-model-asm.cc b/gcc/analyzer/region-model-asm.cc
index c7389f0..bb73e6e 100644
--- a/gcc/analyzer/region-model-asm.cc
+++ b/gcc/analyzer/region-model-asm.cc
@@ -272,7 +272,8 @@ region_model::on_asm_stmt (const gasm *stmt, region_model_context *ctxt)
 	continue;
 
       binding_cluster *cluster = m_store.get_or_create_cluster (base_reg);
-      cluster->on_asm (stmt, m_mgr->get_store_manager ());
+      cluster->on_asm (stmt, m_mgr->get_store_manager (),
+		       conjured_purge (this, ctxt));
     }
 
   /* Update the outputs.  */
@@ -292,8 +293,9 @@ region_model::on_asm_stmt (const gasm *stmt, region_model_context *ctxt)
 	{
 	  sval = m_mgr->get_or_create_conjured_svalue (TREE_TYPE (dst_expr),
 						       stmt,
-						       dst_reg);
-	  purge_state_involving (sval, ctxt);
+						       dst_reg,
+						       conjured_purge (this,
+								       ctxt));
 	}
       set_value (dst_reg, sval, ctxt);
     }
diff --git a/gcc/analyzer/region-model-impl-calls.cc b/gcc/analyzer/region-model-impl-calls.cc
index 65daa34..621e700 100644
--- a/gcc/analyzer/region-model-impl-calls.cc
+++ b/gcc/analyzer/region-model-impl-calls.cc
@@ -212,13 +212,15 @@ call_details::dump (bool simple) const
   pp_flush (&pp);
 }
 
-/* Get a conjured_svalue for this call for REG.  */
+/* Get a conjured_svalue for this call for REG,
+   and purge any state already relating to that conjured_svalue.  */
 
 const svalue *
 call_details::get_or_create_conjured_svalue (const region *reg) const
 {
   region_model_manager *mgr = m_model->get_manager ();
-  return mgr->get_or_create_conjured_svalue (reg->get_type (), m_call, reg);
+  return mgr->get_or_create_conjured_svalue (reg->get_type (), m_call, reg,
+					     conjured_purge (m_model, m_ctxt));
 }
 
 /* Implementations of specific functions.  */
@@ -434,7 +436,6 @@ region_model::impl_call_fgets (const call_details &cd)
     {
       const region *base_reg = reg->get_base_region ();
       const svalue *new_sval = cd.get_or_create_conjured_svalue (base_reg);
-      purge_state_involving (new_sval, cd.get_ctxt ());
       set_value (base_reg, new_sval, cd.get_ctxt ());
     }
 }
@@ -449,7 +450,6 @@ region_model::impl_call_fread (const call_details &cd)
     {
       const region *base_reg = reg->get_base_region ();
       const svalue *new_sval = cd.get_or_create_conjured_svalue (base_reg);
-      purge_state_involving (new_sval, cd.get_ctxt ());
       set_value (base_reg, new_sval, cd.get_ctxt ());
     }
 }
@@ -830,7 +830,9 @@ region_model::impl_call_strchr (const call_details &cd)
 	      const svalue *offset
 		= mgr->get_or_create_conjured_svalue (size_type_node,
 						      cd.get_call_stmt (),
-						      str_reg);
+						      str_reg,
+						      conjured_purge (model,
+								      ctxt));
 	      result = mgr->get_or_create_binop (lhs_type, POINTER_PLUS_EXPR,
 						 str_sval, offset);
 	    }
diff --git a/gcc/analyzer/region-model-manager.cc b/gcc/analyzer/region-model-manager.cc
index 61c7fb32..5ca333a 100644
--- a/gcc/analyzer/region-model-manager.cc
+++ b/gcc/analyzer/region-model-manager.cc
@@ -1169,17 +1169,38 @@ region_model_manager::get_or_create_compound_svalue (tree type,
   return compound_sval;
 }
 
+/* class conjured_purge.  */
+
+/* Purge state relating to SVAL.  */
+
+void
+conjured_purge::purge (const conjured_svalue *sval) const
+{
+  m_model->purge_state_involving (sval, m_ctxt);
+}
+
 /* Return the svalue * of type TYPE for the value conjured for ID_REG
-   at STMT, creating it if necessary.  */
+   at STMT, creating it if necessary.
+   Use P to purge existing state from the svalue, for the case where a
+   conjured_svalue would be reused along an execution path.  */
 
 const svalue *
 region_model_manager::get_or_create_conjured_svalue (tree type,
 						     const gimple *stmt,
-						     const region *id_reg)
+						     const region *id_reg,
+						     const conjured_purge &p)
 {
   conjured_svalue::key_t key (type, stmt, id_reg);
   if (conjured_svalue **slot = m_conjured_values_map.get (key))
-    return *slot;
+    {
+      const conjured_svalue *sval = *slot;
+      /* We're reusing an existing conjured_svalue, perhaps from a different
+	 state within this analysis, or perhaps from an earlier state on this
+	 execution path.  For the latter, purge any state involving the "new"
+	 svalue from the current program_state.  */
+      p.purge (sval);
+      return sval;
+    }
   conjured_svalue *conjured_sval
     = new conjured_svalue (type, stmt, id_reg);
   RETURN_UNKNOWN_IF_TOO_COMPLEX (conjured_sval);
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 44bd802..816b410 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -1324,8 +1324,9 @@ region_model::on_call_pre (const gcall *call, region_model_context *ctxt,
 	     use a conjured value, and purge any prior state involving that
 	     value (in case this is in a loop).  */
 	  sval = m_mgr->get_or_create_conjured_svalue (TREE_TYPE (lhs), call,
-						       lhs_region);
-	  purge_state_involving (sval, ctxt);
+						       lhs_region,
+						       conjured_purge (this,
+								       ctxt));
 	}
       set_value (lhs_region, sval, ctxt);
     }
@@ -1802,7 +1803,8 @@ region_model::handle_unrecognized_call (const gcall *call,
 
   /* Update bindings for all clusters that have escaped, whether above,
      or previously.  */
-  m_store.on_unknown_fncall (call, m_mgr->get_store_manager ());
+  m_store.on_unknown_fncall (call, m_mgr->get_store_manager (),
+			     conjured_purge (this, ctxt));
 
   /* Purge dynamic extents from any regions that have escaped mutably:
      realloc could have been called on them.  */
diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h
index 13a2ea9..2384171 100644
--- a/gcc/analyzer/region-model.h
+++ b/gcc/analyzer/region-model.h
@@ -277,7 +277,8 @@ public:
   const svalue *get_or_create_compound_svalue (tree type,
 					       const binding_map &map);
   const svalue *get_or_create_conjured_svalue (tree type, const gimple *stmt,
-					       const region *id_reg);
+					       const region *id_reg,
+					       const conjured_purge &p);
   const svalue *
   get_or_create_asm_output_svalue (tree type,
 				   const gasm *asm_stmt,
diff --git a/gcc/analyzer/store.cc b/gcc/analyzer/store.cc
index 9aa7d69..0014633 100644
--- a/gcc/analyzer/store.cc
+++ b/gcc/analyzer/store.cc
@@ -1843,12 +1843,14 @@ binding_cluster::mark_as_escaped ()
 
 /* If this cluster has escaped (by this call, or by an earlier one, or
    by being an external param), then unbind all values and mark it
-   as "touched", so that it has an unknown value, rather than an
-   initial_svalue.  */
+   as "touched", so that it has a conjured value, rather than an
+   initial_svalue.
+   Use P to purge state involving conjured_svalues.  */
 
 void
 binding_cluster::on_unknown_fncall (const gcall *call,
-				    store_manager *mgr)
+				    store_manager *mgr,
+				    const conjured_purge &p)
 {
   if (m_escaped)
     {
@@ -1857,25 +1859,27 @@ binding_cluster::on_unknown_fncall (const gcall *call,
       /* Bind it to a new "conjured" value using CALL.  */
       const svalue *sval
 	= mgr->get_svalue_manager ()->get_or_create_conjured_svalue
-	    (m_base_region->get_type (), call, m_base_region);
+	    (m_base_region->get_type (), call, m_base_region, p);
       bind (mgr, m_base_region, sval);
 
       m_touched = true;
     }
 }
 
-/* Mark this cluster as having been clobbered by STMT.  */
+/* Mark this cluster as having been clobbered by STMT.
+   Use P to purge state involving conjured_svalues.  */
 
 void
 binding_cluster::on_asm (const gasm *stmt,
-			 store_manager *mgr)
+			 store_manager *mgr,
+			 const conjured_purge &p)
 {
   m_map.empty ();
 
   /* Bind it to a new "conjured" value using CALL.  */
   const svalue *sval
     = mgr->get_svalue_manager ()->get_or_create_conjured_svalue
-    (m_base_region->get_type (), stmt, m_base_region);
+    (m_base_region->get_type (), stmt, m_base_region, p);
   bind (mgr, m_base_region, sval);
 
   m_touched = true;
@@ -2766,13 +2770,14 @@ store::mark_as_escaped (const region *base_reg)
    (either in this fncall, or in a prior one).  */
 
 void
-store::on_unknown_fncall (const gcall *call, store_manager *mgr)
+store::on_unknown_fncall (const gcall *call, store_manager *mgr,
+			  const conjured_purge &p)
 {
   m_called_unknown_fn = true;
 
   for (cluster_map_t::iterator iter = m_cluster_map.begin ();
        iter != m_cluster_map.end (); ++iter)
-    (*iter).second->on_unknown_fncall (call, mgr);
+    (*iter).second->on_unknown_fncall (call, mgr, p);
 }
 
 /* Return true if a non-const pointer to BASE_REG (or something within it)
diff --git a/gcc/analyzer/store.h b/gcc/analyzer/store.h
index ee084dd..89bb352 100644
--- a/gcc/analyzer/store.h
+++ b/gcc/analyzer/store.h
@@ -609,8 +609,10 @@ public:
 				 store_manager *mgr);
 
   void mark_as_escaped ();
-  void on_unknown_fncall (const gcall *call, store_manager *mgr);
-  void on_asm (const gasm *stmt, store_manager *mgr);
+  void on_unknown_fncall (const gcall *call, store_manager *mgr,
+			  const conjured_purge &p);
+  void on_asm (const gasm *stmt, store_manager *mgr,
+	       const conjured_purge &p);
 
   bool escaped_p () const { return m_escaped; }
   bool touched_p () const { return m_touched; }
@@ -735,7 +737,8 @@ public:
 			   model_merger *merger);
 
   void mark_as_escaped (const region *base_reg);
-  void on_unknown_fncall (const gcall *call, store_manager *mgr);
+  void on_unknown_fncall (const gcall *call, store_manager *mgr,
+			  const conjured_purge &p);
   bool escaped_p (const region *reg) const;
 
   void get_representative_path_vars (const region_model *model,
diff --git a/gcc/analyzer/svalue.h b/gcc/analyzer/svalue.h
index 8040712..4bbe858 100644
--- a/gcc/analyzer/svalue.h
+++ b/gcc/analyzer/svalue.h
@@ -1306,6 +1306,27 @@ template <> struct default_hash_traits<compound_svalue::key_t>
 
 namespace ana {
 
+/* A bundle of state for purging information from a program_state about
+   a conjured_svalue.  We pass this whenever calling
+   get_or_create_conjured_svalue, so that if the program_state already
+   has information about this conjured_svalue on an execution path, we
+   can purge that information, to avoid the analyzer confusing the two
+   values as being the same.  */
+
+class conjured_purge
+{
+public:
+  conjured_purge (region_model *model, region_model_context *ctxt)
+  : m_model (model), m_ctxt (ctxt)
+  {
+  }
+  void purge (const conjured_svalue *sval) const;
+
+private:
+  region_model *m_model;
+  region_model_context *m_ctxt;
+};
+
 /* A defined value arising from a statement, where we want to identify a
    particular unknown value, rather than resorting to the unknown_value
    singleton, so that the value can have sm-state.