analyzer: fix malloc pointer NULL-ness

Fixes to exploded_path::feasible_p exposed a pre-existing bug
with pointer NULL-ness for pointers to symbolic_region.

symbolic_region has an "m_possibly_null" flag which if set means
that a region_svalue pointing to that region is treated as possibly
NULL.  Adding a constraint of "!= NULL" on an edge records that
the pointer is non-NULL, but doesn't affect other pointers (e.g.
if the first if a void *, but the other pointers are cast to other
pointer types).  This showed up in the tests
gcc.dg/analyzer/data-model-5b.c and -5c.c, which malloc a buffer
and test for NULL, but then cast that to a struct * and later test
that struct *: a path for the first test being non-NULL and the
second being NULL was erroneously found to be feasible.

This patch clears the m_possibly_null flag when a "!= NULL" constraint
is added, fixing that erroneous path (but not yet fixing the false
positive in the above tests, which seems to go on to hit a different
issue).  It also adds the field to dumps.

gcc/analyzer/ChangeLog:
	* program-state.cc (selftest::test_program_state_dumping): Update
	expected dump to include symbolic_region's possibly_null field.
	* region-model.cc (symbolic_region::print_fields): New vfunc
	implementation.
	(region_model::add_constraint): Clear m_possibly_null from
	symbolic_regions now known to be non-NULL.
	(selftest::test_malloc_constraints): New selftest.
	(selftest::analyzer_region_model_cc_tests): Call it.
	* region-model.h (region::dyn_cast_symbolic_region): Add non-const
	overload.
	(symbolic_region::dyn_cast_symbolic_region): Implement it.
	(symbolic_region::print_fields): New vfunc override decl.

gcc/testsuite/ChangeLog:
	* gcc.dg/analyzer/data-model-5b.c: Add xfail for new false
	positive leak.
	* gcc.dg/analyzer/data-model-5c.c: Likewise.
	* gcc.dg/analyzer/malloc-5.c: New test.

diff --git a/gcc/analyzer/ChangeLog b/gcc/analyzer/ChangeLog
index ddb02afe192668eb137c0f7d98810987a3e06445..98093cd4f35913122aa95762b2cdacda16efc76c 100644 (file)
--- a/gcc/analyzer/ChangeLog
+++ b/gcc/analyzer/ChangeLog
@@ -1,3 +1,18 @@
+2020-03-27  David Malcolm  <dmalcolm@redhat.com>
+
+       * program-state.cc (selftest::test_program_state_dumping): Update
+       expected dump to include symbolic_region's possibly_null field.
+       * region-model.cc (symbolic_region::print_fields): New vfunc
+       implementation.
+       (region_model::add_constraint): Clear m_possibly_null from
+       symbolic_regions now known to be non-NULL.
+       (selftest::test_malloc_constraints): New selftest.
+       (selftest::analyzer_region_model_cc_tests): Call it.
+       * region-model.h (region::dyn_cast_symbolic_region): Add non-const
+       overload.
+       (symbolic_region::dyn_cast_symbolic_region): Implement it.
+       (symbolic_region::print_fields): New vfunc override decl.
+
 2020-03-27  David Malcolm  <dmalcolm@redhat.com>
 
        * analyzer.h (class feasibility_problem): New forward decl.

diff --git a/gcc/analyzer/program-state.cc b/gcc/analyzer/program-state.cc
index 24b6783d92a8713e1792a7a9c0d6e0c0c974b206..a6049604ca847810413ded65fe4bc993cdd903d9 100644 (file)
--- a/gcc/analyzer/program-state.cc
+++ b/gcc/analyzer/program-state.cc
@@ -1450,7 +1450,7 @@ test_program_state_dumping ()
      "rmodel: r0: {kind: `root', parent: null, sval: null}\n"
      "|-heap: r1: {kind: `heap', parent: r0, sval: sv0}\n"
      "|  |: sval: sv0: {poisoned: uninit}\n"
-     "|  `-r2: {kind: `symbolic', parent: r1, sval: null}\n"
+     "|  `-r2: {kind: `symbolic', parent: r1, sval: null, possibly_null: true}\n"
      "`-globals: r3: {kind: `globals', parent: r0, sval: null, map: {`p': r4}}\n"
      "  `-`p': r4: {kind: `primitive', parent: r3, sval: sv1, type: `void *'}\n"
      "    |: sval: sv1: {type: `void *', &r2}\n"

diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index a71d3de7b12289c6a61cb62adc3678ac6d97ba77..68152526486ab03a4cf72de13fabba2247ec046b 100644 (file)
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -3305,6 +3305,17 @@ symbolic_region::walk_for_canonicalization (canonicalization *) const
   /* Empty.  */
 }
 
+/* Implementation of region::print_fields vfunc for symbolic_region.  */
+
+void
+symbolic_region::print_fields (const region_model &model,
+                              region_id this_rid,
+                              pretty_printer *pp) const
+{
+  region::print_fields (model, this_rid, pp);
+  pp_printf (pp, ", possibly_null: %s", m_possibly_null ? "true" : "false");
+}
+
 /* class region_model.  */
 
 /* region_model's default ctor.  */
@@ -5520,6 +5531,16 @@ region_model::add_constraint (tree lhs, enum tree_code op, tree rhs,
 
   add_any_constraints_from_ssa_def_stmt (lhs, op, rhs, ctxt);
 
+  /* If we now know a symbolic_region is non-NULL, clear its
+     m_possibly_null.  */
+  if (zerop (rhs) && op == NE_EXPR)
+    if (region_svalue *ptr = get_svalue (lhs_sid)->dyn_cast_region_svalue ())
+      {
+       region *pointee = get_region (ptr->get_pointee ());
+       if (symbolic_region *sym_reg = pointee->dyn_cast_symbolic_region ())
+         sym_reg->m_possibly_null = false;
+      }
+
   /* Notify the context, if any.  This exists so that the state machines
      in a program_state can be notified about the condition, and so can
      set sm-state for e.g. unchecked->checked, both for cfg-edges, and
@@ -8595,6 +8616,45 @@ test_constraint_merging ()
             tristate (tristate::TS_UNKNOWN));
 }
 
+/* Verify that if we mark a pointer to a malloc-ed region as non-NULL,
+   all cast pointers to that region are also known to be non-NULL.  */
+
+static void
+test_malloc_constraints ()
+{
+  region_model model;
+  tree p = build_global_decl ("p", ptr_type_node);
+  tree char_star = build_pointer_type (char_type_node);
+  tree q = build_global_decl ("q", char_star);
+  tree null_ptr = build_int_cst (ptr_type_node, 0);
+
+  region_id rid = model.add_new_malloc_region ();
+  svalue_id sid = model.get_or_create_ptr_svalue (ptr_type_node, rid);
+  model.set_value (model.get_lvalue (p, NULL), sid, NULL);
+  model.set_value (q, p, NULL);
+
+  /* We should have a symbolic_region with m_possibly_null: true.  */
+  region *pointee = model.get_region (rid);
+  symbolic_region *sym_reg = pointee->dyn_cast_symbolic_region ();
+  ASSERT_NE (sym_reg, NULL);
+  ASSERT_TRUE (sym_reg->m_possibly_null);
+
+  ASSERT_CONDITION_UNKNOWN (model, p, NE_EXPR, null_ptr);
+  ASSERT_CONDITION_UNKNOWN (model, p, EQ_EXPR, null_ptr);
+  ASSERT_CONDITION_UNKNOWN (model, q, NE_EXPR, null_ptr);
+  ASSERT_CONDITION_UNKNOWN (model, q, EQ_EXPR, null_ptr);
+
+  model.add_constraint (p, NE_EXPR, null_ptr, NULL);
+
+  /* Adding the constraint should have cleared m_possibly_null.  */
+  ASSERT_FALSE (sym_reg->m_possibly_null);
+
+  ASSERT_CONDITION_TRUE (model, p, NE_EXPR, null_ptr);
+  ASSERT_CONDITION_FALSE (model, p, EQ_EXPR, null_ptr);
+  ASSERT_CONDITION_TRUE (model, q, NE_EXPR, null_ptr);
+  ASSERT_CONDITION_FALSE (model, q, EQ_EXPR, null_ptr);
+}
+
 /* Run all of the selftests within this file.  */
 
 void
@@ -8619,6 +8679,7 @@ analyzer_region_model_cc_tests ()
   test_canonicalization_4 ();
   test_state_merging ();
   test_constraint_merging ();
+  test_malloc_constraints ();
 }
 
 } // namespace selftest

diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h
index 65e66005c93ba8edb774591a98a3f3612433b632..235db72141eb81d7e47639146877b229c9be3017 100644 (file)
--- a/gcc/analyzer/region-model.h
+++ b/gcc/analyzer/region-model.h
@@ -845,8 +845,8 @@ public:
   virtual enum region_kind get_kind () const = 0;
   virtual map_region *dyn_cast_map_region () { return NULL; }
   virtual array_region *dyn_cast_array_region () { return NULL; }
-  virtual const symbolic_region *dyn_cast_symbolic_region () const
-  { return NULL; }
+  virtual symbolic_region *dyn_cast_symbolic_region () { return NULL; }
+  virtual const symbolic_region *dyn_cast_symbolic_region () const { return NULL; }
 
   region_id get_parent () const { return m_parent_rid; }
   region *get_parent_region (const region_model &model) const;
@@ -1625,6 +1625,8 @@ public:
 
   const symbolic_region *dyn_cast_symbolic_region () const FINAL OVERRIDE
   { return this; }
+  symbolic_region *dyn_cast_symbolic_region () FINAL OVERRIDE
+  { return this; }
 
   bool compare_fields (const symbolic_region &other) const;
 
@@ -1634,6 +1636,10 @@ public:
 
   void walk_for_canonicalization (canonicalization *c) const FINAL OVERRIDE;
 
+  void print_fields (const region_model &model,
+                    region_id this_rid,
+                    pretty_printer *pp) const FINAL OVERRIDE;
+
   bool m_possibly_null;
 };
 

diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index bc88021ca2d0ca916ead552d3280372021ebb31c..2baa6e800cbafe4f146409854bbad3e436e35915 100644 (file)
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,10 @@
+2020-03-27  David Malcolm  <dmalcolm@redhat.com>
+
+       * gcc.dg/analyzer/data-model-5b.c: Add xfail for new false
+       positive leak.
+       * gcc.dg/analyzer/data-model-5c.c: Likewise.
+       * gcc.dg/analyzer/malloc-5.c: New test.
+
 2020-03-27  David Malcolm  <dmalcolm@redhat.com>
 
        * gcc.dg/analyzer/dot-output.c: Check that

diff --git a/gcc/testsuite/gcc.dg/analyzer/data-model-5b.c b/gcc/testsuite/gcc.dg/analyzer/data-model-5b.c
index 6866f5bf4696dc2f163e5aa0006d39f6467b9cbe..11b56719a66b4cba1a96e34ccde900cdbbe7dc6b 100644 (file)
--- a/gcc/testsuite/gcc.dg/analyzer/data-model-5b.c
+++ b/gcc/testsuite/gcc.dg/analyzer/data-model-5b.c
@@ -87,4 +87,8 @@ void test_1 (const char *str)
   //__analyzer_dump();
   if (obj)
     unref (obj);
-}
+} /* { dg-bogus "leak of 'obj'" "" { xfail *-*-* } } */
+/* TODO(xfail): the false leak report involves the base_obj.ob_refcnt
+   being 1, but the string_obj.str_base.ob_refcnt being unknown (when
+   they ought to be the same region), thus allowing for a path in which
+   the object is allocated but not freed.  */

diff --git a/gcc/testsuite/gcc.dg/analyzer/data-model-5c.c b/gcc/testsuite/gcc.dg/analyzer/data-model-5c.c
index 4dc559c1fcd6d7b628bb1237067a06ae9f697aca..3aba7bdc2aa1642410f9b585aa6304f45df03abf 100644 (file)
--- a/gcc/testsuite/gcc.dg/analyzer/data-model-5c.c
+++ b/gcc/testsuite/gcc.dg/analyzer/data-model-5c.c
@@ -75,4 +75,9 @@ void test_1 (const char *str)
   string_obj *obj = new_string_obj (str);
   if (obj)
     unref (obj);
-}
+} /* { dg-bogus "leak of 'obj'" "" { xfail *-*-* } } */
+/* TODO(xfail): the false leak report involves the base_obj.ob_refcnt
+   being 1, but the string_obj.str_base.ob_refcnt being unknown (when
+   they ought to be the same region), thus allowing for a path in which
+   the object is allocated but not freed.  */
+

diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-5.c b/gcc/testsuite/gcc.dg/analyzer/malloc-5.c
new file mode 100644 (file)
index 0000000..b75135f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-5.c
@@ -0,0 +1,12 @@
+#include <stdlib.h>
+
+void test (void)
+{
+  void *p = malloc (sizeof (int));
+  if (!p)
+    return;
+  int *q = p;
+  if (!q)
+    return;
+  free (q);
+}