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.

gcc/analyzer/ChangeLog

 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.
 	* diagnostic-manager.cc (saved_diagnostic::saved_diagnostic):
 	Initialize new fields m_status, m_epath_length, and m_problem.

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"

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

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

gcc/testsuite/ChangeLog

 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
 	dot-output.c.supergraph-eg.dot is valid.
 

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.  */

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.  */
+

gcc/testsuite/gcc.dg/analyzer/malloc-5.c

+#include <stdlib.h>
+
+void test (void)
+{
+  void *p = malloc (sizeof (int));
+  if (!p)
+    return;
+  int *q = p;
+  if (!q)
+    return;
+  free (q);
+}