diff --git a/gcc/analyzer/diagnostic-manager.cc b/gcc/analyzer/diagnostic-manager.cc index 80bca6a0738..680016e94bc 100644 --- a/gcc/analyzer/diagnostic-manager.cc +++ b/gcc/analyzer/diagnostic-manager.cc @@ -1272,6 +1272,35 @@ diagnostic_manager::build_emission_path (const path_builder &pb, interesting_t interest; pb.get_pending_diagnostic ()->mark_interesting_stuff (&interest); + + /* Add region creation events for any globals of interest, at the + beginning of the path. */ + { + for (auto reg : interest.m_region_creation) + switch (reg->get_memory_space ()) + { + default: + continue; + case MEMSPACE_CODE: + case MEMSPACE_GLOBALS: + case MEMSPACE_READONLY_DATA: + { + const region *base_reg = reg->get_base_region (); + if (tree decl = base_reg->maybe_get_decl ()) + if (DECL_P (decl) + && DECL_SOURCE_LOCATION (decl) != UNKNOWN_LOCATION) + { + emission_path->add_region_creation_event + (reg, + DECL_SOURCE_LOCATION (decl), + NULL_TREE, + 0); + } + } + } + } + + /* Walk EPATH, adding events as appropriate. */ for (unsigned i = 0; i < epath.m_edges.length (); i++) { const exploded_edge *eedge = epath.m_edges[i]; @@ -1569,6 +1598,11 @@ struct null_assignment_sm_context : public sm_context return NULL_TREE; } + const program_state *get_old_program_state () const FINAL OVERRIDE + { + return m_old_state; + } + const program_state *m_old_state; const program_state *m_new_state; const gimple *m_stmt; @@ -1729,46 +1763,47 @@ diagnostic_manager::add_events_for_eedge (const path_builder &pb, break; } - /* Look for changes in dynamic extents, which will identify - the creation of heap-based regions and alloca regions. */ - if (interest) - { - const region_model *src_model = src_state.m_region_model; - const region_model *dst_model = dst_state.m_region_model; - if (src_model->get_dynamic_extents () - != dst_model->get_dynamic_extents ()) - { - unsigned i; - const region *reg; - FOR_EACH_VEC_ELT (interest->m_region_creation, i, reg) - { - const region *base_reg = reg->get_base_region (); - const svalue *old_extents - = src_model->get_dynamic_extents (base_reg); - const svalue *new_extents - = dst_model->get_dynamic_extents (base_reg); - if (old_extents == NULL && new_extents != NULL) - switch (base_reg->get_kind ()) - { - default: - break; - case RK_HEAP_ALLOCATED: - case RK_ALLOCA: - emission_path->add_region_creation_event - (reg, - src_point.get_location (), - src_point.get_fndecl (), - src_stack_depth); - break; - } - } - } - } } } break; } + /* Look for changes in dynamic extents, which will identify + the creation of heap-based regions and alloca regions. */ + if (interest) + { + const region_model *src_model = src_state.m_region_model; + const region_model *dst_model = dst_state.m_region_model; + if (src_model->get_dynamic_extents () + != dst_model->get_dynamic_extents ()) + { + unsigned i; + const region *reg; + FOR_EACH_VEC_ELT (interest->m_region_creation, i, reg) + { + const region *base_reg = reg->get_base_region (); + const svalue *old_extents + = src_model->get_dynamic_extents (base_reg); + const svalue *new_extents + = dst_model->get_dynamic_extents (base_reg); + if (old_extents == NULL && new_extents != NULL) + switch (base_reg->get_kind ()) + { + default: + break; + case RK_HEAP_ALLOCATED: + case RK_ALLOCA: + emission_path->add_region_creation_event + (reg, + src_point.get_location (), + src_point.get_fndecl (), + src_stack_depth); + break; + } + } + } + } + if (pb.get_feasibility_problem () && &pb.get_feasibility_problem ()->m_eedge == &eedge) { diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc index bff37798a39..f5b85ce102e 100644 --- a/gcc/analyzer/engine.cc +++ b/gcc/analyzer/engine.cc @@ -500,6 +500,11 @@ public: return m_unknown_side_effects; } + const program_state *get_old_program_state () const FINAL OVERRIDE + { + return m_old_state; + } + log_user m_logger; exploded_graph &m_eg; exploded_node *m_enode_for_diag; diff --git a/gcc/analyzer/sm-malloc.cc b/gcc/analyzer/sm-malloc.cc index 2f7a387ca8a..a5fa60d020b 100644 --- a/gcc/analyzer/sm-malloc.cc +++ b/gcc/analyzer/sm-malloc.cc @@ -356,10 +356,16 @@ public: if (const region_svalue *ptr = sval->dyn_cast_region_svalue ()) { const region *reg = ptr->get_pointee (); - const region *base_reg = reg->get_base_region (); - if (base_reg->get_kind () == RK_DECL - || base_reg->get_kind () == RK_STRING) - return m_non_heap; + switch (reg->get_memory_space ()) + { + default: + break; + case MEMSPACE_CODE: + case MEMSPACE_GLOBALS: + case MEMSPACE_STACK: + case MEMSPACE_READONLY_DATA: + return m_non_heap; + } } return m_start; } @@ -425,6 +431,11 @@ private: const gcall *call, const deallocator_set *deallocators, bool returns_nonnull = false) const; + void handle_free_of_non_heap (sm_context *sm_ctxt, + const supernode *node, + const gcall *call, + tree arg, + const deallocator *d) const; void on_deallocator_call (sm_context *sm_ctxt, const supernode *node, const gcall *call, @@ -1289,8 +1300,9 @@ class free_of_non_heap : public malloc_diagnostic { public: free_of_non_heap (const malloc_state_machine &sm, tree arg, + const region *freed_reg, const char *funcname) - : malloc_diagnostic (sm, arg), m_funcname (funcname), m_kind (KIND_UNKNOWN) + : malloc_diagnostic (sm, arg), m_freed_reg (freed_reg), m_funcname (funcname) { } @@ -1300,7 +1312,8 @@ public: FINAL OVERRIDE { const free_of_non_heap &other = (const free_of_non_heap &)base_other; - return (same_tree_p (m_arg, other.m_arg) && m_kind == other.m_kind); + return (same_tree_p (m_arg, other.m_arg) + && m_freed_reg == other.m_freed_reg); } bool emit (rich_location *rich_loc) FINAL OVERRIDE @@ -1308,44 +1321,32 @@ public: auto_diagnostic_group d; diagnostic_metadata m; m.add_cwe (590); /* CWE-590: Free of Memory not on the Heap. */ - switch (m_kind) + switch (get_memory_space ()) { default: + case MEMSPACE_HEAP: gcc_unreachable (); - case KIND_UNKNOWN: + case MEMSPACE_UNKNOWN: + case MEMSPACE_CODE: + case MEMSPACE_GLOBALS: + case MEMSPACE_READONLY_DATA: return warning_meta (rich_loc, m, OPT_Wanalyzer_free_of_non_heap, "%<%s%> of %qE which points to memory" " not on the heap", m_funcname, m_arg); break; - case KIND_ALLOCA: + case MEMSPACE_STACK: return warning_meta (rich_loc, m, OPT_Wanalyzer_free_of_non_heap, - "%<%s%> of memory allocated on the stack by" - " %qs (%qE) will corrupt the heap", - m_funcname, "alloca", m_arg); + "%<%s%> of %qE which points to memory" + " on the stack", + m_funcname, m_arg); break; } } - label_text describe_state_change (const evdesc::state_change &change) + label_text describe_state_change (const evdesc::state_change &) FINAL OVERRIDE { - /* Attempt to reconstruct what kind of pointer it is. - (It seems neater for this to be a part of the state, though). */ - if (change.m_expr && TREE_CODE (change.m_expr) == SSA_NAME) - { - gimple *def_stmt = SSA_NAME_DEF_STMT (change.m_expr); - if (gcall *call = dyn_cast (def_stmt)) - { - if (is_special_named_call_p (call, "alloca", 1) - || is_special_named_call_p (call, "__builtin_alloca", 1)) - { - m_kind = KIND_ALLOCA; - return label_text::borrow - ("memory is allocated on the stack here"); - } - } - } return label_text::borrow ("pointer is from here"); } @@ -1354,14 +1355,23 @@ public: return ev.formatted_print ("call to %qs here", m_funcname); } -private: - enum kind + void mark_interesting_stuff (interesting_t *interest) FINAL OVERRIDE { - KIND_UNKNOWN, - KIND_ALLOCA - }; + if (m_freed_reg) + interest->add_region_creation (m_freed_reg); + } + +private: + enum memory_space get_memory_space () const + { + if (m_freed_reg) + return m_freed_reg->get_memory_space (); + else + return MEMSPACE_UNKNOWN; + } + + const region *m_freed_reg; const char *m_funcname; - enum kind m_kind; }; /* struct allocation_state : public state_machine::state. */ @@ -1701,26 +1711,6 @@ malloc_state_machine::on_stmt (sm_context *sm_ctxt, if (any_pointer_p (lhs)) on_zero_assignment (sm_ctxt, stmt,lhs); - /* If we have "LHS = &EXPR;" and EXPR is something other than a MEM_REF, - transition LHS from start to non_heap. - Doing it for ADDR_EXPR(MEM_REF()) is likely wrong, and can lead to - unbounded chains of unmergeable sm-state on pointer arithmetic in loops - when optimization is enabled. */ - if (const gassign *assign_stmt = dyn_cast (stmt)) - { - enum tree_code op = gimple_assign_rhs_code (assign_stmt); - if (op == ADDR_EXPR) - { - tree lhs = gimple_assign_lhs (assign_stmt); - if (lhs) - { - tree addr_expr = gimple_assign_rhs1 (assign_stmt); - if (TREE_CODE (TREE_OPERAND (addr_expr, 0)) != MEM_REF) - sm_ctxt->on_transition (node, stmt, lhs, m_start, m_non_heap); - } - } - } - /* Handle dereferences. */ for (unsigned i = 0; i < gimple_num_ops (stmt); i++) { @@ -1789,6 +1779,30 @@ malloc_state_machine::on_allocator_call (sm_context *sm_ctxt, } } +/* Handle deallocations of non-heap pointers. + non-heap -> stop, with warning. */ + +void +malloc_state_machine::handle_free_of_non_heap (sm_context *sm_ctxt, + const supernode *node, + const gcall *call, + tree arg, + const deallocator *d) const +{ + tree diag_arg = sm_ctxt->get_diagnostic_tree (arg); + const region *freed_reg = NULL; + if (const program_state *old_state = sm_ctxt->get_old_program_state ()) + { + const region_model *old_model = old_state->m_region_model; + const svalue *ptr_sval = old_model->get_rvalue (arg, NULL); + freed_reg = old_model->deref_rvalue (ptr_sval, arg, NULL); + } + sm_ctxt->warn (node, call, arg, + new free_of_non_heap (*this, diag_arg, freed_reg, + d->m_name)); + sm_ctxt->set_next_state (call, arg, m_stop); +} + void malloc_state_machine::on_deallocator_call (sm_context *sm_ctxt, const supernode *node, @@ -1835,11 +1849,7 @@ malloc_state_machine::on_deallocator_call (sm_context *sm_ctxt, else if (state == m_non_heap) { /* non-heap -> stop, with warning. */ - tree diag_arg = sm_ctxt->get_diagnostic_tree (arg); - sm_ctxt->warn (node, call, arg, - new free_of_non_heap (*this, diag_arg, - d->m_name)); - sm_ctxt->set_next_state (call, arg, m_stop); + handle_free_of_non_heap (sm_ctxt, node, call, arg, d); } } @@ -1894,11 +1904,7 @@ malloc_state_machine::on_realloc_call (sm_context *sm_ctxt, else if (state == m_non_heap) { /* non-heap -> stop, with warning. */ - tree diag_arg = sm_ctxt->get_diagnostic_tree (arg); - sm_ctxt->warn (node, call, arg, - new free_of_non_heap (*this, diag_arg, - d->m_name)); - sm_ctxt->set_next_state (call, arg, m_stop); + handle_free_of_non_heap (sm_ctxt, node, call, arg, d); if (path_context *path_ctxt = sm_ctxt->get_path_context ()) path_ctxt->terminate_path (); } diff --git a/gcc/analyzer/sm.h b/gcc/analyzer/sm.h index fccfc882bf1..7ce1c73e217 100644 --- a/gcc/analyzer/sm.h +++ b/gcc/analyzer/sm.h @@ -274,6 +274,10 @@ public: /* Are we handling an external function with unknown side effects? */ virtual bool unknown_side_effects_p () const { return false; } + virtual const program_state *get_old_program_state () const = 0; + + const svalue *get_old_svalue (tree expr) const; + protected: sm_context (int sm_idx, const state_machine &sm) : m_sm_idx (sm_idx), m_sm (sm) {} diff --git a/gcc/testsuite/g++.dg/analyzer/placement-new.C b/gcc/testsuite/g++.dg/analyzer/placement-new.C index b648a428247..89055491a48 100644 --- a/gcc/testsuite/g++.dg/analyzer/placement-new.C +++ b/gcc/testsuite/g++.dg/analyzer/placement-new.C @@ -20,9 +20,9 @@ void test_2 (void) void test_3 (void) { - char buf[sizeof(int)]; + char buf[sizeof(int)]; // { dg-message "region created on stack here" } int *p = new(buf) int (42); - delete p; // { dg-warning "memory not on the heap" } + delete p; // { dg-warning "memory on the stack" } } // { dg-prune-output "-Wfree-nonheap-object" } diff --git a/gcc/testsuite/g++.dg/analyzer/pr100244.C b/gcc/testsuite/g++.dg/analyzer/pr100244.C index 261b3cfff57..1d5e13dd865 100644 --- a/gcc/testsuite/g++.dg/analyzer/pr100244.C +++ b/gcc/testsuite/g++.dg/analyzer/pr100244.C @@ -11,7 +11,7 @@ struct _Hashtable_alloc { int _M_single_bucket; int *_M_buckets; _Hashtable_alloc () { _M_buckets = &_M_single_bucket; } - ~_Hashtable_alloc () { delete _M_buckets; } // { dg-warning "not on the heap" } + ~_Hashtable_alloc () { delete _M_buckets; } // { dg-warning "on the stack" } }; void diff --git a/gcc/testsuite/gcc.dg/analyzer/attr-malloc-1.c b/gcc/testsuite/gcc.dg/analyzer/attr-malloc-1.c index a9a2a3dee85..e956cf5fda6 100644 --- a/gcc/testsuite/gcc.dg/analyzer/attr-malloc-1.c +++ b/gcc/testsuite/gcc.dg/analyzer/attr-malloc-1.c @@ -70,7 +70,7 @@ void test_6 (struct foo *p) void test_7 () { struct foo f; - foo_release (&f); /* { dg-warning "not on the heap" "analyzer" } */ + foo_release (&f); /* { dg-warning "on the stack" "analyzer" } */ /* { dg-warning "'foo_release' called on unallocated object 'f'" "non-analyzer" { target *-*-* } .-1 } */ } diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-1.c b/gcc/testsuite/gcc.dg/analyzer/malloc-1.c index 3219f85f721..d69a60530eb 100644 --- a/gcc/testsuite/gcc.dg/analyzer/malloc-1.c +++ b/gcc/testsuite/gcc.dg/analyzer/malloc-1.c @@ -271,23 +271,23 @@ int *test_23a (int n) int test_24 (void) { - void *ptr = __builtin_alloca (sizeof (int)); /* { dg-message "memory is allocated on the stack here" } */ - free (ptr); /* { dg-warning "'free' of memory allocated on the stack by 'alloca' \\('ptr'\\) will corrupt the heap \\\[CWE-590\\\]" } */ + void *ptr = __builtin_alloca (sizeof (int)); /* { dg-message "region created on stack here" } */ + free (ptr); /* { dg-warning "'free' of 'ptr' which points to memory on the stack \\\[CWE-590\\\]" } */ } int test_25 (void) { - char tmp[100]; - void *p = tmp; /* { dg-message "pointer is from here" } */ - free (p); /* { dg-warning "'free' of 'p' which points to memory not on the heap \\\[CWE-590\\\]" } */ + char tmp[100]; /* { dg-message "region created on stack here" } */ + void *p = tmp; + free (p); /* { dg-warning "'free' of 'p' which points to memory on the stack \\\[CWE-590\\\]" } */ /* TODO: more precise messages here. */ } -char global_buffer[100]; +char global_buffer[100]; /* { dg-message "region created here" } */ int test_26 (void) { - void *p = global_buffer; /* { dg-message "pointer is from here" } */ + void *p = global_buffer; free (p); /* { dg-warning "'free' of 'p' which points to memory not on the heap \\\[CWE-590\\\]" } */ /* TODO: more precise messages here. */ } @@ -608,4 +608,22 @@ int test_49 (int i) return x; } +/* Free of function, and of label within function. */ + +void test_50a (void) +{ +} + +void test_50b (void) +{ + free (test_50a); /* { dg-warning "'free' of '&test_50a' which points to memory not on the heap \\\[CWE-590\\\]" } */ +} + +void test_50c (void) +{ + my_label: + free (&&my_label); /* { dg-warning "'free' of '&my_label' which points to memory not on the heap \\\[CWE-590\\\]" } */ +} + + /* { dg-prune-output "\\\[-Wfree-nonheap-object" } */ diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-callbacks.c b/gcc/testsuite/gcc.dg/analyzer/malloc-callbacks.c index 8820dddf923..c79f80d376d 100644 --- a/gcc/testsuite/gcc.dg/analyzer/malloc-callbacks.c +++ b/gcc/testsuite/gcc.dg/analyzer/malloc-callbacks.c @@ -61,9 +61,8 @@ int *test_5 (void) { allocator_t alloc_fn = get_alloca (); deallocator_t dealloc_fn = get_free (); - int *ptr = alloc_fn (sizeof (int)); /* { dg-message "pointer is from here" } */ - /* TODO: message should read "memory is allocated on the stack here". */ - dealloc_fn (ptr); /* { dg-warning "'free' of 'ptr' which points to memory not on the heap" } */ + int *ptr = alloc_fn (sizeof (int)); /* dg-message "region created on stack here" } */ + dealloc_fn (ptr); /* { dg-warning "'free' of 'ptr' which points to memory on the stack" } */ } static void __attribute__((noinline)) diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-paths-8.c b/gcc/testsuite/gcc.dg/analyzer/malloc-paths-8.c index 9a7c414920c..4988f53b94c 100644 --- a/gcc/testsuite/gcc.dg/analyzer/malloc-paths-8.c +++ b/gcc/testsuite/gcc.dg/analyzer/malloc-paths-8.c @@ -40,7 +40,7 @@ void test_3 (size_t sz) { void *ptr; if (sz <= LIMIT) - ptr = __builtin_alloca (sz); /* { dg-message "memory is allocated on the stack here" } */ + ptr = __builtin_alloca (sz); /* { dg-message "region created on stack here" } */ else ptr = malloc (sz); @@ -49,7 +49,7 @@ void test_3 (size_t sz) /* Bug: the "sz <= LIMIT" above should have been "sz < LIMIT", so there's a free-of-alloca when sz == LIMIT. */ if (sz >= LIMIT) - free (ptr); /* { dg-warning "'free' of memory allocated on the stack by 'alloca'" } */ + free (ptr); /* { dg-warning "'free' of 'ptr' which points to memory on the stack" } */ } /* { dg-bogus "leak of 'ptr'" } */ /* This can't happen, as "sz > 1024" && "sz <= 1023" is impossible. */ diff --git a/gcc/testsuite/gcc.dg/analyzer/pr104560-1.c b/gcc/testsuite/gcc.dg/analyzer/pr104560-1.c new file mode 100644 index 00000000000..aeab4b9f06c --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/pr104560-1.c @@ -0,0 +1,43 @@ +/* Reduced/adapted from false positive from -Wanalyzer-free-of-non-heap + seen on rdma-core. */ + +#include + +#define check_types_match(expr1, expr2) \ + ((typeof(expr1) *)0 != (typeof(expr2) *)0) + +#define container_of(member_ptr, containing_type, member) \ + ((containing_type *) \ + ((char *)(member_ptr) \ + - container_off(containing_type, member)) \ + + check_types_match(*(member_ptr), ((containing_type *)0)->member)) + +#define container_off(containing_type, member) \ + offsetof(containing_type, member) + +struct ibv_device { + /* [...snip...] */ +}; + +struct verbs_device { + struct ibv_device device; /* Must be first */ + /* [...snip...] */ + int placeholder; +}; + +struct mlx5_device { + struct verbs_device verbs_dev; + int placeholder; +}; + +static inline struct mlx5_device *to_mdev(struct ibv_device *ibdev) +{ + return container_of(ibdev, struct mlx5_device, verbs_dev.device); +} + +static void mlx5_uninit_device(struct verbs_device *verbs_device) +{ + struct mlx5_device *dev = to_mdev(&verbs_device->device); + + __builtin_free(dev); /* { dg-bogus "not on the heap" } */ +} diff --git a/gcc/testsuite/gcc.dg/analyzer/pr104560-2.c b/gcc/testsuite/gcc.dg/analyzer/pr104560-2.c new file mode 100644 index 00000000000..f968a582caa --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/pr104560-2.c @@ -0,0 +1,26 @@ +struct ibv_device { + /* [...snip...] */ + int placeholder; +}; + +struct verbs_device { + struct ibv_device device; /* Must be first */ + /* [...snip...] */ + int placeholder; +}; + +struct mlx5_device { + struct verbs_device verbs_dev; + int placeholder; +}; + +static inline struct mlx5_device *to_mdev(struct ibv_device *ibdev) +{ + return (struct mlx5_device *)ibdev; +} + +static void mlx5_uninit_device(struct verbs_device *verbs_device) +{ + struct mlx5_device *dev = to_mdev(&verbs_device->device); + __builtin_free(dev); /* { dg-bogus "not on the heap" } */ +} diff --git a/gcc/testsuite/gcc.dg/analyzer/realloc-1.c b/gcc/testsuite/gcc.dg/analyzer/realloc-1.c index ef117ad9d7b..9951e118efe 100644 --- a/gcc/testsuite/gcc.dg/analyzer/realloc-1.c +++ b/gcc/testsuite/gcc.dg/analyzer/realloc-1.c @@ -59,8 +59,8 @@ void test_6 (size_t sz) void *test_7 (size_t sz) { - char buf[100]; - void *p = realloc (&buf, sz); /* { dg-warning "'realloc' of '&buf' which points to memory not on the heap" } */ + char buf[100]; /* { dg-message "region created on stack here" } */ + void *p = realloc (&buf, sz); /* { dg-warning "'realloc' of '&buf' which points to memory on the stack" } */ return p; } diff --git a/gcc/testsuite/gcc.dg/analyzer/vla-1.c b/gcc/testsuite/gcc.dg/analyzer/vla-1.c index e5971c835fa..9561d742c3a 100644 --- a/gcc/testsuite/gcc.dg/analyzer/vla-1.c +++ b/gcc/testsuite/gcc.dg/analyzer/vla-1.c @@ -12,3 +12,12 @@ void test_1 (int n) s.b = 17; __analyzer_eval (s.b == 17); /* { dg-warning "TRUE" } */ } + +void test_2 (int n) +{ + int arr[n]; /* { dg-message "region created on stack here" } */ + __builtin_free (arr); /* { dg-warning "'free' of '' which points to memory on the stack" } */ + // TODO: fix the "unknown" here +} + +/* { dg-prune-output "\\\[-Wfree-nonheap-object" } */