From 6969ac301f2229366a812942a906257e5c060762 Mon Sep 17 00:00:00 2001 From: David Malcolm Date: Thu, 26 Mar 2020 09:42:25 -0400 Subject: [PATCH] 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 | 15 +++++ gcc/analyzer/program-state.cc | 2 +- gcc/analyzer/region-model.cc | 61 +++++++++++++++++++ gcc/analyzer/region-model.h | 10 ++- gcc/testsuite/ChangeLog | 7 +++ gcc/testsuite/gcc.dg/analyzer/data-model-5b.c | 6 +- gcc/testsuite/gcc.dg/analyzer/data-model-5c.c | 7 ++- gcc/testsuite/gcc.dg/analyzer/malloc-5.c | 12 ++++ 8 files changed, 115 insertions(+), 5 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/malloc-5.c diff --git a/gcc/analyzer/ChangeLog b/gcc/analyzer/ChangeLog index ddb02afe192..98093cd4f35 100644 --- a/gcc/analyzer/ChangeLog +++ b/gcc/analyzer/ChangeLog @@ -1,3 +1,18 @@ +2020-03-27 David Malcolm + + * 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 * analyzer.h (class feasibility_problem): New forward decl. diff --git a/gcc/analyzer/program-state.cc b/gcc/analyzer/program-state.cc index 24b6783d92a..a6049604ca8 100644 --- 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 a71d3de7b12..68152526486 100644 --- 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 65e66005c93..235db72141e 100644 --- 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 bc88021ca2d..2baa6e800cb 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,10 @@ +2020-03-27 David Malcolm + + * 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 * 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 6866f5bf469..11b56719a66 100644 --- 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 4dc559c1fcd..3aba7bdc2aa 100644 --- 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 index 00000000000..b75135f26a3 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/malloc-5.c @@ -0,0 +1,12 @@ +#include + +void test (void) +{ + void *p = malloc (sizeof (int)); + if (!p) + return; + int *q = p; + if (!q) + return; + free (q); +} -- 2.30.2