From: David Malcolm Date: Fri, 28 Aug 2020 17:43:56 +0000 (-0400) Subject: analyzer: generalize sm-malloc to new/delete [PR94355] X-Git-Tag: upstream/12.2.0~13864 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=1690a839cff2e0276017a013419d81d675bbf69d;p=platform%2Fupstream%2Fgcc.git analyzer: generalize sm-malloc to new/delete [PR94355] This patch generalizes the state machine in sm-malloc.c to support multiple allocator APIs, and adds just enough support for C++ new and delete to demonstrate the feature, allowing for detection of code paths where the result of new in C++ can leak - for some crude examples, at least (bearing in mind that the analyzer doesn't yet know about e.g. vfuncs, exceptions, inheritance, RTTI, etc) It also implements a new warning: -Wanalyzer-mismatching-deallocation. For example: demo.cc: In function 'void test()': demo.cc:8:8: warning: 'f' should have been deallocated with 'delete' but was deallocated with 'free' [CWE-762] [-Wanalyzer-mismatching-deallocation] 8 | free (f); | ~~~~~^~~ 'void test()': events 1-2 | | 7 | foo *f = new foo; | | ^~~ | | | | | (1) allocated here (expects deallocation with 'delete') | 8 | free (f); | | ~~~~~~~~ | | | | | (2) deallocated with 'free' here; allocation at (1) expects deallocation with 'delete' | The patch also adds just enough knowledge of exception-handling to suppress a false positive from -Wanalyzer-malloc-leak on g++.dg/analyzer/pr96723.C on the exception-handling CFG edge after operator new. It does this by adding a constraint that the result is NULL if an exception was thrown from operator new, since the result from operator new is lost when following that exception-handling CFG edge. gcc/analyzer/ChangeLog: PR analyzer/94355 * analyzer.opt (Wanalyzer-mismatching-deallocation): New warning. * region-model-impl-calls.cc (region_model::impl_call_operator_new): New. (region_model::impl_call_operator_delete): New. * region-model.cc (region_model::on_call_pre): Detect operator new and operator delete. (region_model::on_call_post): Likewise. (region_model::maybe_update_for_edge): Detect EH edges and call... (region_model::apply_constraints_for_exception): New function. * region-model.h (region_model::impl_call_operator_new): New decl. (region_model::impl_call_operator_delete): New decl. (region_model::apply_constraints_for_exception): New decl. * sm-malloc.cc (enum resource_state): New. (struct allocation_state): New state subclass. (enum wording): New. (struct api): New. (malloc_state_machine::custom_data_t): New typedef. (malloc_state_machine::add_state): New decl. (malloc_state_machine::m_unchecked) (malloc_state_machine::m_nonnull) (malloc_state_machine::m_freed): Delete these states in favor of... (malloc_state_machine::m_malloc) (malloc_state_machine::m_scalar_new) (malloc_state_machine::m_vector_new): ...this new api instances, which own their own versions of these states. (malloc_state_machine::on_allocator_call): New decl. (malloc_state_machine::on_deallocator_call): New decl. (api::api): New ctor. (dyn_cast_allocation_state): New. (as_a_allocation_state): New. (get_rs): New. (unchecked_p): New. (nonnull_p): New. (freed_p): New. (malloc_diagnostic::describe_state_change): Use unchecked_p and nonnull_p. (class mismatching_deallocation): New. (double_free::double_free): Add funcname param for initializing m_funcname. (double_free::emit): Use m_funcname in warning message rather than hardcoding "free". (double_free::describe_state_change): Likewise. Use freed_p. (double_free::describe_call_with_state): Use freed_p. (double_free::describe_final_event): Use m_funcname in message rather than hardcoding "free". (double_free::m_funcname): New field. (possible_null::describe_state_change): Use unchecked_p. (possible_null::describe_return_of_state): Likewise. (use_after_free::use_after_free): Add param for initializing m_api. (use_after_free::emit): Use m_api->m_dealloc_funcname in message rather than hardcoding "free". (use_after_free::describe_state_change): Use freed_p. Change the wording of the message based on the API. (use_after_free::describe_final_event): Use m_api->m_dealloc_funcname in message rather than hardcoding "free". Change the wording of the message based on the API. (use_after_free::m_api): New field. (malloc_leak::describe_state_change): Use unchecked_p. Update for renaming of m_malloc_event to m_alloc_event. (malloc_leak::describe_final_event): Update for renaming of m_malloc_event to m_alloc_event. (malloc_leak::m_malloc_event): Rename... (malloc_leak::m_alloc_event): ...to this. (free_of_non_heap::free_of_non_heap): Add param for initializing m_funcname. (free_of_non_heap::emit): Use m_funcname in message rather than hardcoding "free". (free_of_non_heap::describe_final_event): Likewise. (free_of_non_heap::m_funcname): New field. (allocation_state::dump_to_pp): New. (allocation_state::get_nonnull): New. (malloc_state_machine::malloc_state_machine): Update for changes to state fields and new api fields. (malloc_state_machine::add_state): New. (malloc_state_machine::on_stmt): Move malloc/calloc handling to on_allocator_call and call it, passing in the API pointer. Likewise for free, moving it to on_deallocator_call. Handle calls to operator new and delete in an analogous way. Use unchecked_p when testing for possibly-null-arg and possibly-null-deref, and transition to the non-null for the correct API. Remove redundant node param from call to on_zero_assignment. Use freed_p for use-after-free check, and pass in API. (malloc_state_machine::on_allocator_call): New, based on code in on_stmt. (malloc_state_machine::on_deallocator_call): Likewise. (malloc_state_machine::on_phi): Mark node param with ATTRIBUTE_UNUSED; don't pass it to on_zero_assignment. (malloc_state_machine::on_condition): Mark node param with ATTRIBUTE_UNUSED. Replace on_transition calls with get_state and set_next_state pairs, transitioning to the non-null state for the appropriate API. (malloc_state_machine::can_purge_p): Port to new state approach. (malloc_state_machine::on_zero_assignment): Replace on_transition calls with get_state and set_next_state pairs. Drop redundant node param. * sm.h (state_machine::add_custom_state): New. gcc/ChangeLog: PR analyzer/94355 * doc/invoke.texi: Document -Wanalyzer-mismatching-deallocation. gcc/testsuite/ChangeLog: PR analyzer/94355 * g++.dg/analyzer/new-1.C: New test. * g++.dg/analyzer/new-vs-malloc.C: New test. --- diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt index b0c797f..07bff33 100644 --- a/gcc/analyzer/analyzer.opt +++ b/gcc/analyzer/analyzer.opt @@ -70,6 +70,10 @@ Wanalyzer-malloc-leak Common Var(warn_analyzer_malloc_leak) Init(1) Warning Warn about code paths in which a heap-allocated pointer leaks. +Wanalyzer-mismatching-deallocation +Common Var(warn_analyzer_mismatching_deallocation) Init(1) Warning +Warn about code paths in which the wrong deallocation function is called. + Wanalyzer-possible-null-argument Common Var(warn_analyzer_possible_null_argument) Init(1) Warning Warn about code paths in which a possibly-NULL value is passed to a must-not-be-NULL function argument. diff --git a/gcc/analyzer/region-model-impl-calls.cc b/gcc/analyzer/region-model-impl-calls.cc index 27c8ae5..fa85035 100644 --- a/gcc/analyzer/region-model-impl-calls.cc +++ b/gcc/analyzer/region-model-impl-calls.cc @@ -318,6 +318,41 @@ region_model::impl_call_memset (const call_details &cd) return false; } +/* Handle the on_call_pre part of "operator new". */ + +bool +region_model::impl_call_operator_new (const call_details &cd) +{ + const svalue *size_sval = cd.get_arg_svalue (0); + const region *new_reg = create_region_for_heap_alloc (size_sval); + if (cd.get_lhs_type ()) + { + const svalue *ptr_sval + = m_mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg); + cd.maybe_set_lhs (ptr_sval); + } + return false; +} + +/* Handle the on_call_pre part of "operator delete", which comes in + both sized and unsized variants (2 arguments and 1 argument + respectively). */ + +bool +region_model::impl_call_operator_delete (const call_details &cd) +{ + const svalue *ptr_sval = cd.get_arg_svalue (0); + if (const region_svalue *ptr_to_region_sval + = ptr_sval->dyn_cast_region_svalue ()) + { + /* If the ptr points to an underlying heap region, delete it, + poisoning pointers. */ + const region *freed_reg = ptr_to_region_sval->get_pointee (); + unbind_region_and_descendents (freed_reg, POISON_KIND_FREED); + } + return false; +} + /* Handle the on_call_pre part of "strlen". Return true if the LHS is updated. */ diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index e6a9d3c..03c7daf 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -706,6 +706,16 @@ region_model::on_call_pre (const gcall *call, region_model_context *ctxt) if (impl_call_strlen (cd)) return false; } + else if (is_named_call_p (callee_fndecl, "operator new", call, 1)) + return impl_call_operator_new (cd); + else if (is_named_call_p (callee_fndecl, "operator new []", call, 1)) + return impl_call_operator_new (cd); + else if (is_named_call_p (callee_fndecl, "operator delete", call, 1) + || is_named_call_p (callee_fndecl, "operator delete", call, 2) + || is_named_call_p (callee_fndecl, "operator delete []", call, 1)) + { + /* Handle in "on_call_post". */ + } else if (!fndecl_has_gimple_body_p (callee_fndecl) && !DECL_PURE_P (callee_fndecl) && !fndecl_built_in_p (callee_fndecl)) @@ -746,12 +756,22 @@ region_model::on_call_post (const gcall *call, region_model_context *ctxt) { if (tree callee_fndecl = get_fndecl_for_call (call, ctxt)) - if (is_named_call_p (callee_fndecl, "free", call, 1)) - { - call_details cd (call, this, ctxt); - impl_call_free (cd); - return; - } + { + if (is_named_call_p (callee_fndecl, "free", call, 1)) + { + call_details cd (call, this, ctxt); + impl_call_free (cd); + return; + } + if (is_named_call_p (callee_fndecl, "operator delete", call, 1) + || is_named_call_p (callee_fndecl, "operator delete", call, 2) + || is_named_call_p (callee_fndecl, "operator delete []", call, 1)) + { + call_details cd (call, this, ctxt); + impl_call_operator_delete (cd); + return; + } + } if (unknown_side_effects) handle_unrecognized_call (call, ctxt); @@ -2191,6 +2211,11 @@ region_model::maybe_update_for_edge (const superedge &edge, return apply_constraints_for_gswitch (*switch_sedge, switch_stmt, ctxt); } + /* Apply any constraints due to an exception being thrown. */ + if (const cfg_superedge *cfg_sedge = dyn_cast (&edge)) + if (cfg_sedge->get_flags () & EDGE_EH) + return apply_constraints_for_exception (last_stmt, ctxt); + return true; } @@ -2349,6 +2374,34 @@ region_model::apply_constraints_for_gswitch (const switch_cfg_superedge &edge, } } +/* Apply any constraints due to an exception being thrown at LAST_STMT. + + If they are feasible, add the constraints and return true. + + Return false if the constraints contradict existing knowledge + (and so the edge should not be taken). */ + +bool +region_model::apply_constraints_for_exception (const gimple *last_stmt, + region_model_context *ctxt) +{ + gcc_assert (last_stmt); + if (const gcall *call = dyn_cast (last_stmt)) + if (tree callee_fndecl = get_fndecl_for_call (call, ctxt)) + if (is_named_call_p (callee_fndecl, "operator new", call, 1) + || is_named_call_p (callee_fndecl, "operator new []", call, 1)) + { + /* We have an exception thrown from operator new. + Add a constraint that the result was NULL, to avoid a false + leak report due to the result being lost when following + the EH edge. */ + if (tree lhs = gimple_call_lhs (call)) + return add_constraint (lhs, EQ_EXPR, null_pointer_node, ctxt); + return true; + } + return true; +} + /* For use with push_frame when handling a top-level call within the analysis. PARAM has a defined but unknown initial value. Anything it points to has escaped, since the calling context "knows" diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h index f325a8b..4707293 100644 --- a/gcc/analyzer/region-model.h +++ b/gcc/analyzer/region-model.h @@ -2556,6 +2556,8 @@ class region_model bool impl_call_malloc (const call_details &cd); bool impl_call_memset (const call_details &cd); bool impl_call_strlen (const call_details &cd); + bool impl_call_operator_new (const call_details &cd); + bool impl_call_operator_delete (const call_details &cd); void handle_unrecognized_call (const gcall *call, region_model_context *ctxt); @@ -2694,6 +2696,8 @@ class region_model bool apply_constraints_for_gswitch (const switch_cfg_superedge &edge, const gswitch *switch_stmt, region_model_context *ctxt); + bool apply_constraints_for_exception (const gimple *last_stmt, + region_model_context *ctxt); int poison_any_pointers_to_descendents (const region *reg, enum poison_kind pkind); diff --git a/gcc/analyzer/sm-malloc.cc b/gcc/analyzer/sm-malloc.cc index 2f7db92..2b5a870 100644 --- a/gcc/analyzer/sm-malloc.cc +++ b/gcc/analyzer/sm-malloc.cc @@ -48,6 +48,114 @@ namespace ana { namespace { +class api; +class malloc_state_machine; + +/* An enum for discriminating between different kinds of allocation_state. */ + +enum resource_state +{ + /* States that are independent of api. */ + + /* The start state. */ + RS_START, + + /* State for a pointer that's known to be NULL. */ + RS_NULL, + + /* State for a pointer that's known to not be on the heap (e.g. to a local + or global). */ + RS_NON_HEAP, + + /* Stop state, for pointers we don't want to track any more. */ + RS_STOP, + + /* States that relate to a specific api. */ + + /* State for a pointer returned from the api's allocator that hasn't + been checked for NULL. + It could be a pointer to heap-allocated memory, or could be NULL. */ + RS_UNCHECKED, + + /* State for a pointer returned from the api's allocator, + known to be non-NULL. */ + RS_NONNULL, + + /* State for a pointer passed to the api's deallocator. */ + RS_FREED +}; + +/* Custom state subclass, which can optionally refer to an an api. */ + +struct allocation_state : public state_machine::state +{ + allocation_state (const char *name, unsigned id, + enum resource_state rs, const api *a) + : state (name, id), m_rs (rs), m_api (a) + {} + + void dump_to_pp (pretty_printer *pp) const FINAL OVERRIDE; + + const allocation_state *get_nonnull () const; + + enum resource_state m_rs; + const api *m_api; +}; + +/* An enum for choosing which wording to use in various diagnostics + when describing deallocations. */ + +enum wording +{ + WORDING_FREED, + WORDING_DELETED +}; + +/* Represents a particular family of API calls for allocating/deallocating + heap memory that should be matched e.g. + - malloc/free + - scalar new/delete + - vector new[]/delete[] + etc. + + We track the expected deallocation function, but not the allocation + function - there could be more than one allocator per deallocator. For + example, there could be dozens of allocators for "free" beyond just + malloc e.g. calloc, xstrdup, etc. We don't want to explode the number + of states by tracking individual allocators in the exploded graph; + we merely want to track "this value expects to have 'free' called on it". + Perhaps we can reconstruct which allocator was used later, when emitting + the path, if it's necessary for precision of wording of diagnostics. */ + +struct api +{ + api (malloc_state_machine *sm, + const char *name, + const char *dealloc_funcname, + enum wording wording); + + /* An internal name for identifying this API in dumps. */ + const char *m_name; + + /* The name of the deallocation function, for use in diagnostics. */ + const char *m_dealloc_funcname; + + /* Which wording to use in diagnostics. */ + enum wording m_wording; + + /* Pointers to api-specific states. + These states are owned by the state_machine base class. */ + + /* State for an unchecked result from this api's allocator. */ + state_machine::state_t m_unchecked; + + /* State for a known non-NULL result from this apis's allocator. */ + state_machine::state_t m_nonnull; + + /* State for a value passed to this api's deallocator. */ + state_machine::state_t m_freed; +}; + /* A state machine for detecting misuses of the malloc/free API. See sm-malloc.dot for an overview (keep this in-sync with that file). */ @@ -55,8 +163,13 @@ namespace { class malloc_state_machine : public state_machine { public: + typedef allocation_state custom_data_t; + malloc_state_machine (logger *logger); + state_t + add_state (const char *name, enum resource_state rs, const api *a); + bool inherited_state_p () const FINAL OVERRIDE { return false; } state_machine::state_t @@ -98,20 +211,15 @@ public: bool reset_when_passed_to_unknown_fn_p (state_t s, bool is_mutable) const FINAL OVERRIDE; - /* State for a pointer returned from malloc that hasn't been checked for - NULL. - It could be a pointer to heap-allocated memory, or could be NULL. */ - state_t m_unchecked; + api m_malloc; + api m_scalar_new; + api m_vector_new; + + /* States that are independent of api. */ /* State for a pointer that's known to be NULL. */ state_t m_null; - /* State for a pointer to heap-allocated memory, known to be non-NULL. */ - state_t m_nonnull; - - /* State for a pointer to freed memory. */ - state_t m_freed; - /* State for a pointer that's known to not be on the heap (e.g. to a local or global). */ state_t m_non_heap; // TODO: or should this be a different state machine? @@ -121,12 +229,89 @@ public: state_t m_stop; private: + void on_allocator_call (sm_context *sm_ctxt, + const gcall *call, + const api &ap) const; + void on_deallocator_call (sm_context *sm_ctxt, + const supernode *node, + const gcall *call, + const api &ap) const; void on_zero_assignment (sm_context *sm_ctxt, - const supernode *node, const gimple *stmt, tree lhs) const; }; +/* struct api. */ + +api::api (malloc_state_machine *sm, + const char *name, + const char *dealloc_funcname, + enum wording wording) +: m_name (name), + m_dealloc_funcname (dealloc_funcname), + m_wording (wording), + m_unchecked (sm->add_state ("unchecked", RS_UNCHECKED, this)), + m_nonnull (sm->add_state ("nonnull", RS_NONNULL, this)), + m_freed (sm->add_state ("freed", RS_FREED, this)) +{ +} + +/* Return STATE cast to the custom state subclass, or NULL for the start state. + Everything should be an allocation_state apart from the start state. */ + +static const allocation_state * +dyn_cast_allocation_state (state_machine::state_t state) +{ + if (state->get_id () == 0) + return NULL; + return static_cast (state); +} + +/* Return STATE cast to the custom state subclass, for a state that is + already known to not be the start state . */ + +static const allocation_state * +as_a_allocation_state (state_machine::state_t state) +{ + gcc_assert (state->get_id () != 0); + return static_cast (state); +} + +/* Get the resource_state for STATE. */ + +static enum resource_state +get_rs (state_machine::state_t state) +{ + if (const allocation_state *astate = dyn_cast_allocation_state (state)) + return astate->m_rs; + else + return RS_START; +} + +/* Return true if STATE is an unchecked result from an allocator. */ + +static bool +unchecked_p (state_machine::state_t state) +{ + return get_rs (state) == RS_UNCHECKED; +} + +/* Return true if STATE is a non-null result from an allocator. */ + +static bool +nonnull_p (state_machine::state_t state) +{ + return get_rs (state) == RS_NONNULL; +} + +/* Return true if STATE is a value that has been passed to a deallocator. */ + +static bool +freed_p (state_machine::state_t state) +{ + return get_rs (state) == RS_FREED; +} + /* Class for diagnostics relating to malloc_state_machine. */ class malloc_diagnostic : public pending_diagnostic @@ -145,11 +330,11 @@ public: OVERRIDE { if (change.m_old_state == m_sm.get_start_state () - && change.m_new_state == m_sm.m_unchecked) + && unchecked_p (change.m_new_state)) // TODO: verify that it's the allocation stmt, not a copy return label_text::borrow ("allocated here"); - if (change.m_old_state == m_sm.m_unchecked - && change.m_new_state == m_sm.m_nonnull) + if (unchecked_p (change.m_old_state) + && nonnull_p (change.m_new_state)) { if (change.m_expr) return change.formatted_print ("assuming %qE is non-NULL", @@ -160,7 +345,7 @@ public: } if (change.m_new_state == m_sm.m_null) { - if (change.m_old_state == m_sm.m_unchecked) + if (unchecked_p (change.m_old_state)) { if (change.m_expr) return change.formatted_print ("assuming %qE is NULL", @@ -188,13 +373,75 @@ protected: tree m_arg; }; +/* Concrete subclass for reporting mismatching allocator/deallocator + diagnostics. */ + +class mismatching_deallocation : public malloc_diagnostic +{ +public: + mismatching_deallocation (const malloc_state_machine &sm, tree arg, + const api *expected_dealloc, + const api *actual_dealloc) + : malloc_diagnostic (sm, arg), + m_expected_dealloc (expected_dealloc), + m_actual_dealloc (actual_dealloc) + {} + + const char *get_kind () const FINAL OVERRIDE + { + return "mismatching_deallocation"; + } + + bool emit (rich_location *rich_loc) FINAL OVERRIDE + { + auto_diagnostic_group d; + diagnostic_metadata m; + m.add_cwe (762); /* CWE-762: Mismatched Memory Management Routines. */ + return warning_meta (rich_loc, m, OPT_Wanalyzer_mismatching_deallocation, + "%qE should have been deallocated with %qs" + " but was deallocated with %qs", + m_arg, m_expected_dealloc->m_dealloc_funcname, + m_actual_dealloc->m_dealloc_funcname); + } + + label_text describe_state_change (const evdesc::state_change &change) + FINAL OVERRIDE + { + if (unchecked_p (change.m_new_state)) + { + m_alloc_event = change.m_event_id; + return change.formatted_print ("allocated here" + " (expects deallocation with %qs)", + m_expected_dealloc->m_dealloc_funcname); + } + return malloc_diagnostic::describe_state_change (change); + } + + label_text describe_final_event (const evdesc::final_event &ev) FINAL OVERRIDE + { + if (m_alloc_event.known_p ()) + return ev.formatted_print + ("deallocated with %qs here;" + " allocation at %@ expects deallocation with %qs", + m_actual_dealloc->m_dealloc_funcname, &m_alloc_event, + m_expected_dealloc->m_dealloc_funcname); + return ev.formatted_print ("deallocated with %qs here", + m_actual_dealloc->m_dealloc_funcname); + } + +private: + diagnostic_event_id_t m_alloc_event; + const api *m_expected_dealloc; + const api *m_actual_dealloc; +}; + /* Concrete subclass for reporting double-free diagnostics. */ class double_free : public malloc_diagnostic { public: - double_free (const malloc_state_machine &sm, tree arg) - : malloc_diagnostic (sm, arg) + double_free (const malloc_state_machine &sm, tree arg, const char *funcname) + : malloc_diagnostic (sm, arg), m_funcname (funcname) {} const char *get_kind () const FINAL OVERRIDE { return "double_free"; } @@ -205,16 +452,16 @@ public: diagnostic_metadata m; m.add_cwe (415); /* CWE-415: Double Free. */ return warning_meta (rich_loc, m, OPT_Wanalyzer_double_free, - "double-% of %qE", m_arg); + "double-%<%s%> of %qE", m_funcname, m_arg); } label_text describe_state_change (const evdesc::state_change &change) FINAL OVERRIDE { - if (change.m_new_state == m_sm.m_freed) + if (freed_p (change.m_new_state)) { m_first_free_event = change.m_event_id; - return change.formatted_print ("first %qs here", "free"); + return change.formatted_print ("first %qs here", m_funcname); } return malloc_diagnostic::describe_state_change (change); } @@ -222,7 +469,7 @@ public: label_text describe_call_with_state (const evdesc::call_with_state &info) FINAL OVERRIDE { - if (info.m_state == m_sm.m_freed) + if (freed_p (info.m_state)) return info.formatted_print ("passing freed pointer %qE in call to %qE from %qE", info.m_expr, info.m_callee_fndecl, info.m_caller_fndecl); @@ -233,13 +480,14 @@ public: { if (m_first_free_event.known_p ()) return ev.formatted_print ("second %qs here; first %qs was at %@", - "free", "free", + m_funcname, m_funcname, &m_first_free_event); - return ev.formatted_print ("second %qs here", "free"); + return ev.formatted_print ("second %qs here", m_funcname); } private: diagnostic_event_id_t m_first_free_event; + const char *m_funcname; }; /* Abstract subclass for describing possible bad uses of NULL. @@ -256,7 +504,7 @@ public: FINAL OVERRIDE { if (change.m_old_state == m_sm.get_start_state () - && change.m_new_state == m_sm.m_unchecked) + && unchecked_p (change.m_new_state)) { m_origin_of_unchecked_event = change.m_event_id; return label_text::borrow ("this call could return NULL"); @@ -267,7 +515,7 @@ public: label_text describe_return_of_state (const evdesc::return_of_state &info) FINAL OVERRIDE { - if (info.m_state == m_sm.m_unchecked) + if (unchecked_p (info.m_state)) return info.formatted_print ("possible return of NULL to %qE from %qE", info.m_caller_fndecl, info.m_callee_fndecl); return label_text (); @@ -480,8 +728,9 @@ private: class use_after_free : public malloc_diagnostic { public: - use_after_free (const malloc_state_machine &sm, tree arg) - : malloc_diagnostic (sm, arg) + use_after_free (const malloc_state_machine &sm, tree arg, + const api *a) + : malloc_diagnostic (sm, arg), m_api (a) {} const char *get_kind () const FINAL OVERRIDE { return "use_after_free"; } @@ -492,31 +741,52 @@ public: diagnostic_metadata m; m.add_cwe (416); return warning_meta (rich_loc, m, OPT_Wanalyzer_use_after_free, - "use after % of %qE", m_arg); + "use after %<%s%> of %qE", + m_api->m_dealloc_funcname, m_arg); } label_text describe_state_change (const evdesc::state_change &change) FINAL OVERRIDE { - if (change.m_new_state == m_sm.m_freed) + if (freed_p (change.m_new_state)) { m_free_event = change.m_event_id; - return label_text::borrow ("freed here"); + switch (m_api->m_wording) + { + default: + gcc_unreachable (); + case WORDING_FREED: + return label_text::borrow ("freed here"); + case WORDING_DELETED: + return label_text::borrow ("deleted here"); + } } return malloc_diagnostic::describe_state_change (change); } label_text describe_final_event (const evdesc::final_event &ev) FINAL OVERRIDE { + const char *funcname = m_api->m_dealloc_funcname; if (m_free_event.known_p ()) - return ev.formatted_print ("use after % of %qE; freed at %@", - ev.m_expr, &m_free_event); + switch (m_api->m_wording) + { + default: + gcc_unreachable (); + case WORDING_FREED: + return ev.formatted_print ("use after %<%s%> of %qE; freed at %@", + funcname, ev.m_expr, &m_free_event); + case WORDING_DELETED: + return ev.formatted_print ("use after %<%s%> of %qE; deleted at %@", + funcname, ev.m_expr, &m_free_event); + } else - return ev.formatted_print ("use after % of %qE", ev.m_expr); + return ev.formatted_print ("use after %<%s%> of %qE", + funcname, ev.m_expr); } private: diagnostic_event_id_t m_free_event; + const api *m_api; }; class malloc_leak : public malloc_diagnostic @@ -542,9 +812,9 @@ public: label_text describe_state_change (const evdesc::state_change &change) FINAL OVERRIDE { - if (change.m_new_state == m_sm.m_unchecked) + if (unchecked_p (change.m_new_state)) { - m_malloc_event = change.m_event_id; + m_alloc_event = change.m_event_id; return label_text::borrow ("allocated here"); } return malloc_diagnostic::describe_state_change (change); @@ -554,31 +824,32 @@ public: { if (ev.m_expr) { - if (m_malloc_event.known_p ()) + if (m_alloc_event.known_p ()) return ev.formatted_print ("%qE leaks here; was allocated at %@", - ev.m_expr, &m_malloc_event); + ev.m_expr, &m_alloc_event); else return ev.formatted_print ("%qE leaks here", ev.m_expr); } else { - if (m_malloc_event.known_p ()) + if (m_alloc_event.known_p ()) return ev.formatted_print ("%qs leaks here; was allocated at %@", - "", &m_malloc_event); + "", &m_alloc_event); else return ev.formatted_print ("%qs leaks here", ""); } } private: - diagnostic_event_id_t m_malloc_event; + diagnostic_event_id_t m_alloc_event; }; class free_of_non_heap : public malloc_diagnostic { public: - free_of_non_heap (const malloc_state_machine &sm, tree arg) - : malloc_diagnostic (sm, arg), m_kind (KIND_UNKNOWN) + free_of_non_heap (const malloc_state_machine &sm, tree arg, + const char *funcname) + : malloc_diagnostic (sm, arg), m_funcname (funcname), m_kind (KIND_UNKNOWN) { } @@ -602,15 +873,15 @@ public: gcc_unreachable (); case KIND_UNKNOWN: return warning_meta (rich_loc, m, OPT_Wanalyzer_free_of_non_heap, - "% of %qE which points to memory" + "%<%s%> of %qE which points to memory" " not on the heap", - m_arg); + m_funcname, m_arg); break; case KIND_ALLOCA: return warning_meta (rich_loc, m, OPT_Wanalyzer_free_of_non_heap, - "% of memory allocated on the stack by" + "%<%s%> of memory allocated on the stack by" " %qs (%qE) will corrupt the heap", - "alloca", m_arg); + m_funcname, "alloca", m_arg); break; } } @@ -639,7 +910,7 @@ public: label_text describe_final_event (const evdesc::final_event &ev) FINAL OVERRIDE { - return ev.formatted_print ("call to %qs here", "free"); + return ev.formatted_print ("call to %qs here", m_funcname); } private: @@ -648,20 +919,54 @@ private: KIND_UNKNOWN, KIND_ALLOCA }; + const char *m_funcname; enum kind m_kind; }; +/* struct allocation_state : public state_machine::state. */ + +/* Implementation of state_machine::state::dump_to_pp vfunc + for allocation_state: append the API that this allocation is + associated with. */ + +void +allocation_state::dump_to_pp (pretty_printer *pp) const +{ + state_machine::state::dump_to_pp (pp); + if (m_api) + pp_printf (pp, " (%s)", m_api->m_name); +} + +/* Given a allocation_state for an api, get the "nonnull" state + for the corresponding allocator. */ + +const allocation_state * +allocation_state::get_nonnull () const +{ + gcc_assert (m_api); + return as_a_allocation_state (m_api->m_nonnull); +} + /* malloc_state_machine's ctor. */ malloc_state_machine::malloc_state_machine (logger *logger) -: state_machine ("malloc", logger) -{ - m_unchecked = add_state ("unchecked"); - m_null = add_state ("null"); - m_nonnull = add_state ("nonnull"); - m_freed = add_state ("freed"); - m_non_heap = add_state ("non-heap"); - m_stop = add_state ("stop"); +: state_machine ("malloc", logger), + m_malloc (this, "malloc", "free", WORDING_FREED), + m_scalar_new (this, "new", "delete", WORDING_DELETED), + m_vector_new (this, "new[]", "delete[]", WORDING_DELETED) +{ + gcc_assert (m_start->get_id () == 0); + m_null = add_state ("null", RS_FREED, NULL); + m_non_heap = add_state ("non-heap", RS_NON_HEAP, NULL); + m_stop = add_state ("stop", RS_STOP, NULL); +} + +state_machine::state_t +malloc_state_machine::add_state (const char *name, enum resource_state rs, + const api *a) +{ + return add_custom_state (new allocation_state (name, alloc_state_id (), + rs, a)); } /* Implementation of state_machine::on_stmt vfunc for malloc_state_machine. */ @@ -681,13 +986,23 @@ malloc_state_machine::on_stmt (sm_context *sm_ctxt, || is_named_call_p (callee_fndecl, "__builtin_malloc", call, 1) || is_named_call_p (callee_fndecl, "__builtin_calloc", call, 2)) { - tree lhs = gimple_call_lhs (call); - if (lhs) - sm_ctxt->on_transition (node, stmt, lhs, m_start, m_unchecked); - else - { - /* TODO: report leak. */ - } + on_allocator_call (sm_ctxt, call, m_malloc); + return true; + } + + if (is_named_call_p (callee_fndecl, "operator new", call, 1)) + on_allocator_call (sm_ctxt, call, m_scalar_new); + else if (is_named_call_p (callee_fndecl, "operator new []", call, 1)) + on_allocator_call (sm_ctxt, call, m_vector_new); + else if (is_named_call_p (callee_fndecl, "operator delete", call, 1) + || is_named_call_p (callee_fndecl, "operator delete", call, 2)) + { + on_deallocator_call (sm_ctxt, node, call, m_scalar_new); + return true; + } + else if (is_named_call_p (callee_fndecl, "operator delete []", call, 1)) + { + on_deallocator_call (sm_ctxt, node, call, m_vector_new); return true; } @@ -704,32 +1019,7 @@ malloc_state_machine::on_stmt (sm_context *sm_ctxt, || is_std_named_call_p (callee_fndecl, "free", call, 1) || is_named_call_p (callee_fndecl, "__builtin_free", call, 1)) { - tree arg = gimple_call_arg (call, 0); - tree diag_arg = sm_ctxt->get_diagnostic_tree (arg); - - /* start/unchecked/nonnull -> freed. */ - sm_ctxt->on_transition (node, stmt, arg, m_start, m_freed); - sm_ctxt->on_transition (node, stmt, arg, m_unchecked, m_freed); - sm_ctxt->on_transition (node, stmt, arg, m_nonnull, m_freed); - - /* Keep state "null" as-is, rather than transitioning to "free"; - we don't want to complain about double-free of NULL. */ - - /* freed -> stop, with warning. */ - if (sm_ctxt->get_state (stmt, arg) == m_freed) - { - sm_ctxt->warn (node, stmt, arg, - new double_free (*this, diag_arg)); - sm_ctxt->set_next_state (stmt, arg, m_stop); - } - - /* non-heap -> stop, with warning. */ - if (sm_ctxt->get_state (stmt, arg) == m_non_heap) - { - sm_ctxt->warn (node, stmt, arg, - new free_of_non_heap (*this, diag_arg)); - sm_ctxt->set_next_state (stmt, arg, m_stop); - } + on_deallocator_call (sm_ctxt, node, call, m_malloc); return true; } @@ -752,13 +1042,16 @@ malloc_state_machine::on_stmt (sm_context *sm_ctxt, tree diag_arg = sm_ctxt->get_diagnostic_tree (arg); state_t state = sm_ctxt->get_state (stmt, arg); /* Can't use a switch as the states are non-const. */ - if (state == m_unchecked) + if (unchecked_p (state)) { sm_ctxt->warn (node, stmt, arg, new possible_null_arg (*this, diag_arg, callee_fndecl, i)); - sm_ctxt->set_next_state (stmt, arg, m_nonnull); + const allocation_state *astate + = as_a_allocation_state (state); + sm_ctxt->set_next_state (stmt, arg, + astate->get_nonnull ()); } else if (state == m_null) { @@ -776,7 +1069,7 @@ malloc_state_machine::on_stmt (sm_context *sm_ctxt, if (tree lhs = sm_ctxt->is_zero_assignment (stmt)) if (any_pointer_p (lhs)) - on_zero_assignment (sm_ctxt, node, stmt,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. @@ -813,12 +1106,12 @@ malloc_state_machine::on_stmt (sm_context *sm_ctxt, tree diag_arg = sm_ctxt->get_diagnostic_tree (arg); state_t state = sm_ctxt->get_state (stmt, arg); - /* Can't use a switch as the states are non-const. */ - if (state == m_unchecked) + if (unchecked_p (state)) { sm_ctxt->warn (node, stmt, arg, new possible_null_deref (*this, diag_arg)); - sm_ctxt->set_next_state (stmt, arg, m_nonnull); + const allocation_state *astate = as_a_allocation_state (state); + sm_ctxt->set_next_state (stmt, arg, astate->get_nonnull ()); } else if (state == m_null) { @@ -826,10 +1119,12 @@ malloc_state_machine::on_stmt (sm_context *sm_ctxt, new null_deref (*this, diag_arg)); sm_ctxt->set_next_state (stmt, arg, m_stop); } - else if (state == m_freed) + else if (freed_p (state)) { + const allocation_state *astate = as_a_allocation_state (state); sm_ctxt->warn (node, stmt, arg, - new use_after_free (*this, diag_arg)); + new use_after_free (*this, diag_arg, + astate->m_api)); sm_ctxt->set_next_state (stmt, arg, m_stop); } } @@ -837,18 +1132,87 @@ malloc_state_machine::on_stmt (sm_context *sm_ctxt, return false; } +/* Handle a call to an allocator. */ + +void +malloc_state_machine::on_allocator_call (sm_context *sm_ctxt, + const gcall *call, + const api &ap) const +{ + tree lhs = gimple_call_lhs (call); + if (lhs) + { + if (sm_ctxt->get_state (call, lhs) == m_start) + sm_ctxt->set_next_state (call, lhs, ap.m_unchecked); + } + else + { + /* TODO: report leak. */ + } +} + +void +malloc_state_machine::on_deallocator_call (sm_context *sm_ctxt, + const supernode *node, + const gcall *call, + const api &ap) const +{ + tree arg = gimple_call_arg (call, 0); + tree diag_arg = sm_ctxt->get_diagnostic_tree (arg); + + state_t state = sm_ctxt->get_state (call, arg); + + /* start/unchecked/nonnull -> freed. */ + if (state == m_start) + sm_ctxt->set_next_state (call, arg, ap.m_freed); + else if (unchecked_p (state) || nonnull_p (state)) + { + const allocation_state *astate = as_a_allocation_state (state); + + if (astate->m_api != &ap) + { + /* Wrong allocator. */ + pending_diagnostic *d + = new mismatching_deallocation (*this, diag_arg, + astate->m_api, &ap); + sm_ctxt->warn (node, call, arg, d); + } + sm_ctxt->set_next_state (call, arg, ap.m_freed); + } + + /* Keep state "null" as-is, rather than transitioning to "freed"; + we don't want to complain about double-free of NULL. */ + + else if (state == ap.m_freed) + { + /* freed -> stop, with warning. */ + sm_ctxt->warn (node, call, arg, + new double_free (*this, diag_arg, + ap.m_dealloc_funcname)); + sm_ctxt->set_next_state (call, arg, m_stop); + } + else if (state == m_non_heap) + { + /* non-heap -> stop, with warning. */ + sm_ctxt->warn (node, call, arg, + new free_of_non_heap (*this, diag_arg, + ap.m_dealloc_funcname)); + sm_ctxt->set_next_state (call, arg, m_stop); + } +} + /* Implementation of state_machine::on_phi vfunc for malloc_state_machine. */ void malloc_state_machine::on_phi (sm_context *sm_ctxt, - const supernode *node, + const supernode *node ATTRIBUTE_UNUSED, const gphi *phi, tree rhs) const { if (zerop (rhs)) { tree lhs = gimple_phi_result (phi); - on_zero_assignment (sm_ctxt, node, phi, lhs); + on_zero_assignment (sm_ctxt, phi, lhs); } } @@ -857,7 +1221,7 @@ malloc_state_machine::on_phi (sm_context *sm_ctxt, void malloc_state_machine::on_condition (sm_context *sm_ctxt, - const supernode *node, + const supernode *node ATTRIBUTE_UNUSED, const gimple *stmt, tree lhs, enum tree_code op, @@ -874,14 +1238,19 @@ malloc_state_machine::on_condition (sm_context *sm_ctxt, if (op == NE_EXPR) { log ("got 'ARG != 0' match"); - sm_ctxt->on_transition (node, stmt, - lhs, m_unchecked, m_nonnull); + state_t s = sm_ctxt->get_state (stmt, lhs); + if (unchecked_p (s)) + { + const allocation_state *astate = as_a_allocation_state (s); + sm_ctxt->set_next_state (stmt, lhs, astate->get_nonnull ()); + } } else if (op == EQ_EXPR) { log ("got 'ARG == 0' match"); - sm_ctxt->on_transition (node, stmt, - lhs, m_unchecked, m_null); + state_t s = sm_ctxt->get_state (stmt, lhs); + if (unchecked_p (s)) + sm_ctxt->set_next_state (stmt, lhs, m_null); } } @@ -892,7 +1261,8 @@ malloc_state_machine::on_condition (sm_context *sm_ctxt, bool malloc_state_machine::can_purge_p (state_t s) const { - return s != m_unchecked && s != m_nonnull; + enum resource_state rs = get_rs (s); + return rs != RS_UNCHECKED && rs != RS_NONNULL; } /* Implementation of state_machine::on_leak vfunc for malloc_state_machine @@ -926,14 +1296,16 @@ malloc_state_machine::reset_when_passed_to_unknown_fn_p (state_t s, void malloc_state_machine::on_zero_assignment (sm_context *sm_ctxt, - const supernode *node, const gimple *stmt, tree lhs) const { - sm_ctxt->on_transition (node, stmt, lhs, m_start, m_null); - sm_ctxt->on_transition (node, stmt, lhs, m_unchecked, m_null); - sm_ctxt->on_transition (node, stmt, lhs, m_nonnull, m_null); - sm_ctxt->on_transition (node, stmt, lhs, m_freed, m_null); + state_t s = sm_ctxt->get_state (stmt, lhs); + enum resource_state rs = get_rs (s); + if (rs == RS_START + || rs == RS_UNCHECKED + || rs == RS_NONNULL + || rs == RS_FREED) + sm_ctxt->set_next_state (stmt, lhs, m_null); } } // anonymous namespace diff --git a/gcc/analyzer/sm.h b/gcc/analyzer/sm.h index 740cbec..f44ad92 100644 --- a/gcc/analyzer/sm.h +++ b/gcc/analyzer/sm.h @@ -125,6 +125,12 @@ public: protected: state_t add_state (const char *name); + state_t add_custom_state (state *s) + { + m_states.safe_push (s); + return s; + } + unsigned alloc_state_id () { return m_next_state_id++; } private: diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index bca8c85..070445a 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -414,6 +414,7 @@ Objective-C and Objective-C++ Dialects}. -Wno-analyzer-file-leak @gol -Wno-analyzer-free-of-non-heap @gol -Wno-analyzer-malloc-leak @gol +-Wno-analyzer-mismatching-deallocation @gol -Wno-analyzer-null-argument @gol -Wno-analyzer-null-dereference @gol -Wno-analyzer-possible-null-argument @gol @@ -8645,6 +8646,7 @@ Enabling this option effectively enables the following warnings: -Wanalyzer-file-leak @gol -Wanalyzer-free-of-non-heap @gol -Wanalyzer-malloc-leak @gol +-Wanalyzer-mismatching-deallocation @gol -Wanalyzer-possible-null-argument @gol -Wanalyzer-possible-null-dereference @gol -Wanalyzer-null-argument @gol @@ -8729,6 +8731,17 @@ to disable it. This diagnostic warns for paths through the code in which a pointer allocated via @code{malloc} is leaked. +@item -Wno-analyzer-mismatching-deallocation +@opindex Wanalyzer-mismatching-deallocation +@opindex Wno-analyzer-mismatching-deallocation +This warning requires @option{-fanalyzer}, which enables it; use +@option{-Wno-analyzer-mismatching-deallocation} +to disable it. + +This diagnostic warns for paths through the code in which the +wrong deallocation function is called on a pointer value, based on +which function was used to allocate the pointer value. + @item -Wno-analyzer-possible-null-argument @opindex Wanalyzer-possible-null-argument @opindex Wno-analyzer-possible-null-argument diff --git a/gcc/testsuite/g++.dg/analyzer/new-1.C b/gcc/testsuite/g++.dg/analyzer/new-1.C new file mode 100644 index 0000000..fadb1d2 --- /dev/null +++ b/gcc/testsuite/g++.dg/analyzer/new-1.C @@ -0,0 +1,52 @@ +void test_1 () +{ + char* p = new char; // { dg-message "allocated here" } +} // { dg-warning "leak of 'p'" } + +void test_2 () +{ + char* p = new char; + delete p; +} + +/* double-delete shows up as use-after-delete + due to a clobber before the delete. */ + +void test_3 () +{ + char* p = new char; + delete p; // { dg-message "deleted here" } + delete p; +} // { dg-warning "use after 'delete'" } +// FIXME: should be on the 2nd delete, not here + +void test_4 () +{ + char *p = new char[16]; // { dg-message "allocated here" } + delete[] p; // { dg-message "first 'delete\\\[\\\]' here" } + delete[] p; // { dg-warning "double-'delete\\\[\\\]' of 'p'" } +} + +void test_5 () +{ + char *p = new char[16]; + delete p; // { dg-warning "'p' should have been deallocated with 'delete\\\[\\\]' but was deallocated with 'delete'" } +} + +void test_6 () +{ + char *p = new char; + delete[] p; // { dg-warning "'p' should have been deallocated with 'delete' but was deallocated with 'delete\\\[\\\]'" } +} + +char test_7 (char *p) +{ + delete p; // { dg-message "deleted here" } + return *p; // { dg-warning "use after 'delete' of 'p'" } +} + +char test_8 (char *p) +{ + delete[] p; // { dg-message "deleted here" } + return *p; // { dg-warning "use after 'delete\\\[\\\]' of 'p'" } +} diff --git a/gcc/testsuite/g++.dg/analyzer/new-vs-malloc.C b/gcc/testsuite/g++.dg/analyzer/new-vs-malloc.C new file mode 100644 index 0000000..cb0b324 --- /dev/null +++ b/gcc/testsuite/g++.dg/analyzer/new-vs-malloc.C @@ -0,0 +1,21 @@ +#include + +struct s {}; + +void test_1 () +{ + s *p = new s; // { dg-message "allocated here \\(expects deallocation with 'delete'\\)" + free (p); // { dg-warning "'p' should have been deallocated with 'delete' but was deallocated with 'free'" } +} + +void test_2 () +{ + char *p = new char[16]; // { dg-message "allocated here \\(expects deallocation with 'delete\\\[\\\]'\\)" + free (p); // { dg-warning "'p' should have been deallocated with 'delete\\\[\\\]' but was deallocated with 'free'" } +} + +void test_3 () +{ + char *p = (char *)malloc (16); // { dg-message "allocated here \\(expects deallocation with 'free'\\)" + delete[] p; // { dg-warning "'p' should have been deallocated with 'free' but was deallocated with 'delete\\\[\\\]'" } +}