analyzer: ensure that we purge state when reusing a conjured_svalue [PR105087]
authorDavid Malcolm <dmalcolm@redhat.com>
Tue, 29 Mar 2022 00:41:23 +0000 (20:41 -0400)
committerDavid Malcolm <dmalcolm@redhat.com>
Tue, 29 Mar 2022 00:41:23 +0000 (20:41 -0400)
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>
12 files changed:
gcc/analyzer/analyzer.h
gcc/analyzer/region-model-asm.cc
gcc/analyzer/region-model-impl-calls.cc
gcc/analyzer/region-model-manager.cc
gcc/analyzer/region-model.cc
gcc/analyzer/region-model.h
gcc/analyzer/store.cc
gcc/analyzer/store.h
gcc/analyzer/svalue.h
gcc/testsuite/gcc.dg/analyzer/pr105087-1.c [new file with mode: 0644]
gcc/testsuite/gcc.dg/analyzer/pr105087-2.c [new file with mode: 0644]
gcc/testsuite/gcc.dg/analyzer/vasprintf-1.c [new file with mode: 0644]

index a8289d0..39934a3 100644 (file)
@@ -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;
index c7389f0..bb73e6e 100644 (file)
@@ -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);
     }
index 65daa34..621e700 100644 (file)
@@ -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);
            }
index 61c7fb3..5ca333a 100644 (file)
@@ -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);
index 44bd802..816b410 100644 (file)
@@ -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.  */
index 13a2ea9..2384171 100644 (file)
@@ -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,
index 9aa7d69..0014633 100644 (file)
@@ -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)
index ee084dd..89bb352 100644 (file)
@@ -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,
index 8040712..4bbe858 100644 (file)
@@ -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.
diff --git a/gcc/testsuite/gcc.dg/analyzer/pr105087-1.c b/gcc/testsuite/gcc.dg/analyzer/pr105087-1.c
new file mode 100644 (file)
index 0000000..c4acf42
--- /dev/null
@@ -0,0 +1,18 @@
+#include "analyzer-decls.h"
+
+extern void *inner_alloc (void);
+
+void * __attribute__((noinline))
+outer_alloc (void)
+{
+  return inner_alloc ();
+}
+
+void test_1 (void)
+{
+  void *p, *q;
+
+  p = outer_alloc ();
+  q = outer_alloc ();
+  __analyzer_eval (p == q); /* { dg-warning "UNKNOWN" } */
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/pr105087-2.c b/gcc/testsuite/gcc.dg/analyzer/pr105087-2.c
new file mode 100644 (file)
index 0000000..7cd6591
--- /dev/null
@@ -0,0 +1,20 @@
+#include "analyzer-decls.h"
+
+extern void inner_alloc (void **);
+
+void * __attribute__((noinline))
+outer_alloc (void)
+{
+  void *result;
+  inner_alloc (&result);
+  return result;
+}
+
+void test_1 (void)
+{
+  void *p, *q;
+
+  p = outer_alloc ();
+  q = outer_alloc ();
+  __analyzer_eval (p == q);  /* { dg-warning "UNKNOWN" } */
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/vasprintf-1.c b/gcc/testsuite/gcc.dg/analyzer/vasprintf-1.c
new file mode 100644 (file)
index 0000000..061cd00
--- /dev/null
@@ -0,0 +1,57 @@
+/* { dg-additional-options "-Wno-analyzer-too-complex" } */
+
+#define NULL ((void *)0)
+
+extern int printf (const char *__restrict __format, ...);
+extern int vasprintf (char **__restrict __ptr, const char *__restrict __f,
+                     __builtin_va_list __arg)
+  __attribute__ ((__nothrow__, __format__ (__printf__, 2, 0))) ;
+extern void free (void *__ptr) __attribute__ ((__nothrow__ , __leaf__));
+
+static char * __attribute__ ((__format__ (__printf__, 1, 2)))
+zasprintf (const char *format, ...)
+{
+  char *resultp;
+  __builtin_va_list args;
+  __builtin_va_start (args, format);
+  int r = vasprintf (&resultp, format, args);
+  __builtin_va_end (args);
+  return r < 0 ? NULL : resultp;
+}
+
+int run_test() {
+    char *buf = NULL;
+    char *bar = NULL;
+    char *baz = NULL;
+    int i = 1232;
+
+    printf("static function check\n");
+
+    buf = zasprintf("i = %d", i);
+    if (buf) {
+        printf("buf = %s\nbuf = %p\n", buf, buf);
+    }
+
+    bar = zasprintf("i = %d - %d", i, i - 13);
+    if (bar) {
+        printf("bar = %s\nbar = %p\n", bar, bar);
+        printf("buf = %s\nbuf = %p\n", buf, buf);
+    }
+
+    baz = zasprintf("No i's here");
+    if (baz) {
+        printf("baz = %s\nbaz = %p\n", baz, baz);
+        printf("bar = %s\nbar = %p\n", bar, bar);
+        printf("buf = %s\nbuf = %p\n", buf, buf);
+    }
+
+    free(buf);
+    free(bar);
+    free(baz);
+
+    return 1;
+}
+
+int main(int argc, char **argv) {
+    return run_test();
+}