From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from www523.your-server.de (www523.your-server.de [159.69.224.22]) by sourceware.org (Postfix) with ESMTPS id 9BCAA3858D39 for ; Thu, 30 Jun 2022 22:12:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9BCAA3858D39 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=tim-lange.me Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tim-lange.me DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tim-lange.me; s=default2108; h=Content-Transfer-Encoding:MIME-Version: References:In-Reply-To:Message-Id:Date:Subject:Cc:To:From:Sender:Reply-To: Content-Type:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID; bh=kXOcJhi8TtcXQNbgF4OF9FI6HwMZPC4tCyzzWCcN2Z0=; b=cUq4x6f8gTLTmlTCTXS+MsqrCg wXognh6l+lr/Md0BXSCmpP/kmOgbu9u29WKhhnuNRfM1X3XPcEb8RdvEm9MZsyWXYQELPzEv88XEU VxY74Bfs7q2cK+Otv7NB8NP7BUK60OBGvYWCQ8iFKZOC/01K0S/BSfT6/zlGXZKu1yFM2lZxX6ANz uvlFnrpZtOLoIJPlzS8JT2kbYgoWOOV527Y6+rq/A1apKYG1i2r5YuQQX66krLRWXm2/PYEkJOQ62 vEH2oB/FPjJWzKi3CwDHkDAlYlOqiYvgyjSZuoB9uYNUTIOVIx0rXaH2yl50mZrYsG0gzsr9efSD6 m6dnLkpQ==; Received: from sslproxy03.your-server.de ([88.198.220.132]) by www523.your-server.de with esmtpsa (TLSv1.3:TLS_AES_256_GCM_SHA384:256) (Exim 4.92.3) (envelope-from ) id 1o72OF-0001Pd-2d; Fri, 01 Jul 2022 00:11:59 +0200 Received: from [2a02:908:1861:d6a0::b718] (helo=fedora..) by sslproxy03.your-server.de with esmtpsa (TLSv1.3:TLS_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1o72OE-000BYY-SH; Fri, 01 Jul 2022 00:11:58 +0200 From: Tim Lange To: dmalcolm@redhat.com Cc: gcc@gcc.gnu.org, Tim Lange Subject: [PATCH v3] analyzer: add allocation size checker [PR105900] Date: Fri, 1 Jul 2022 00:11:53 +0200 Message-Id: <20220630221153.49510-1-mail@tim-lange.me> X-Mailer: git-send-email 2.36.1 In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Authenticated-Sender: mail@tim-lange.me X-Virus-Scanned: Clear (ClamAV 0.103.6/26589/Thu Jun 30 10:08:14 2022) X-Spam-Status: No, score=-11.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_INFOUSMEBIZ, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 30 Jun 2022 22:12:05 -0000 Hi, here's the updated patch that should address all the comments from the v2. - Tim This patch adds an checker that warns about code paths in which a buffer is assigned to a incompatible type, i.e. when the allocated buffer size is not a multiple of the pointee's size. 2022-07-30 Tim Lange gcc/analyzer/ChangeLog: PR analyzer/105900 * analyzer.opt: Added Wanalyzer-allocation-size. * checker-path.cc (region_creation_event::get_desc): Added call to new virtual function pending_diagnostic::describe_region_creation_event. * checker-path.h: Added region_creation_event::get_desc. * diagnostic-manager.cc (diagnostic_manager::add_event_on_final_node): New function. * diagnostic-manager.h: Added diagnostic_manager::add_event_on_final_node. * pending-diagnostic.h (struct region_creation): New event_desc struct. (pending_diagnostic::describe_region_creation_event): Added virtual function to overwrite description of a region creation. * region-model.cc (class dubious_allocation_size): New class. (capacity_compatible_with_type): New helper function. (class size_visitor): New class. (struct_or_union_with_inheritance_p): New helper function. (is_any_cast_p): New helper function. (region_model::check_region_size): New function. (region_model::set_value): Added call to region_model::check_region_size. * region-model.h (class region_model): New function check_region_size. * svalue.cc (region_svalue::accept): Changed to post-order traversal. (initial_svalue::accept): Likewise. (unaryop_svalue::accept): Likewise. (binop_svalue::accept): Likewise. (sub_svalue::accept): Likewise. (repeated_svalue::accept): Likewise. (bits_within_svalue::accept): Likewise. (widening_svalue::accept): Likewise. (unmergeable_svalue::accept): Likewise. (compound_svalue::accept): Likewise. (conjured_svalue::accept): Likewise. (asm_output_svalue::accept): Likewise. (const_fn_result_svalue::accept): Likewise. gcc/ChangeLog: PR analyzer/105900 * doc/invoke.texi: Added Wanalyzer-allocation-size. gcc/testsuite/ChangeLog: PR analyzer/105900 * gcc.dg/analyzer/pr96639.c: Changed buffer size to omit warning. * gcc.dg/analyzer/allocation-size-1.c: New test. * gcc.dg/analyzer/allocation-size-2.c: New test. * gcc.dg/analyzer/allocation-size-3.c: New test. * gcc.dg/analyzer/allocation-size-4.c: New test. * gcc.dg/analyzer/allocation-size-5.c: New test. Signed-off-by: Tim Lange --- gcc/analyzer/analyzer.opt | 4 + gcc/analyzer/checker-path.cc | 11 +- gcc/analyzer/checker-path.h | 2 +- gcc/analyzer/diagnostic-manager.cc | 61 +++ gcc/analyzer/diagnostic-manager.h | 4 + gcc/analyzer/pending-diagnostic.h | 20 + gcc/analyzer/region-model.cc | 370 ++++++++++++++++++ gcc/analyzer/region-model.h | 2 + gcc/analyzer/svalue.cc | 26 +- gcc/doc/invoke.texi | 14 + .../gcc.dg/analyzer/allocation-size-1.c | 116 ++++++ .../gcc.dg/analyzer/allocation-size-2.c | 155 ++++++++ .../gcc.dg/analyzer/allocation-size-3.c | 45 +++ .../gcc.dg/analyzer/allocation-size-4.c | 60 +++ .../gcc.dg/analyzer/allocation-size-5.c | 36 ++ gcc/testsuite/gcc.dg/analyzer/pr96639.c | 2 +- 16 files changed, 912 insertions(+), 16 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/allocation-size-3.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/allocation-size-4.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/allocation-size-5.c diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt index 4aea52d3a87..1d612246a30 100644 --- a/gcc/analyzer/analyzer.opt +++ b/gcc/analyzer/analyzer.opt @@ -54,6 +54,10 @@ The minimum number of supernodes within a function for the analyzer to consider Common Joined UInteger Var(param_analyzer_max_enodes_for_full_dump) Init(200) Param The maximum depth of exploded nodes that should appear in a dot dump before switching to a less verbose format. +Wanalyzer-allocation-size +Common Var(warn_analyzer_allocation_size) Init(1) Warning +Warn about code paths in which a pointer to a buffer is assigned to an incompatible type. + Wanalyzer-double-fclose Common Var(warn_analyzer_double_fclose) Init(1) Warning Warn about code paths in which a stdio FILE can be closed more than once. diff --git a/gcc/analyzer/checker-path.cc b/gcc/analyzer/checker-path.cc index 0133dc94137..953e192cd55 100644 --- a/gcc/analyzer/checker-path.cc +++ b/gcc/analyzer/checker-path.cc @@ -302,8 +302,17 @@ region_creation_event::region_creation_event (const region *reg, region_creation_event. */ label_text -region_creation_event::get_desc (bool) const +region_creation_event::get_desc (bool can_colorize) const { + if (m_pending_diagnostic) + { + label_text custom_desc + = m_pending_diagnostic->describe_region_creation_event + (evdesc::region_creation (can_colorize, m_reg)); + if (custom_desc.m_buffer) + return custom_desc; + } + switch (m_reg->get_memory_space ()) { default: diff --git a/gcc/analyzer/checker-path.h b/gcc/analyzer/checker-path.h index 24decf5ce3d..8e48d8a07ab 100644 --- a/gcc/analyzer/checker-path.h +++ b/gcc/analyzer/checker-path.h @@ -219,7 +219,7 @@ public: region_creation_event (const region *reg, location_t loc, tree fndecl, int depth); - label_text get_desc (bool) const final override; + label_text get_desc (bool can_colorize) const final override; private: const region *m_reg; diff --git a/gcc/analyzer/diagnostic-manager.cc b/gcc/analyzer/diagnostic-manager.cc index 8ea1f61776e..4adfda1af65 100644 --- a/gcc/analyzer/diagnostic-manager.cc +++ b/gcc/analyzer/diagnostic-manager.cc @@ -1476,6 +1476,67 @@ diagnostic_manager::build_emission_path (const path_builder &pb, const exploded_edge *eedge = epath.m_edges[i]; add_events_for_eedge (pb, *eedge, emission_path, &interest); } + add_event_on_final_node (epath.get_final_enode (), emission_path, &interest); +} + +/* Emit a region_creation_event when requested on the last statement in + the path. + + If a region_creation_event should be emitted on the last statement of the + path, we need to peek to the successors to get whether the final enode + created a region. +*/ + +void +diagnostic_manager::add_event_on_final_node (const exploded_node *final_enode, + checker_path *emission_path, + interesting_t *interest) const +{ + const program_point &src_point = final_enode->get_point (); + const int src_stack_depth = src_point.get_stack_depth (); + const program_state &src_state = final_enode->get_state (); + const region_model *src_model = src_state.m_region_model; + + unsigned j; + exploded_edge *e; + FOR_EACH_VEC_ELT (final_enode->m_succs, j, e) + { + exploded_node *dst = e->m_dest; + const program_state &dst_state = dst->get_state (); + 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; + bool emitted = false; + 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); + emitted = true; + break; + } + } + if (emitted) + break; + } + } } /* Subclass of state_change_visitor that creates state_change_event diff --git a/gcc/analyzer/diagnostic-manager.h b/gcc/analyzer/diagnostic-manager.h index b9bb7c8c254..266eed8f9cb 100644 --- a/gcc/analyzer/diagnostic-manager.h +++ b/gcc/analyzer/diagnostic-manager.h @@ -149,6 +149,10 @@ private: const exploded_path &epath, checker_path *emission_path) const; + void add_event_on_final_node (const exploded_node *final_enode, + checker_path *emission_path, + interesting_t *interest) const; + void add_events_for_eedge (const path_builder &pb, const exploded_edge &eedge, checker_path *emission_path, diff --git a/gcc/analyzer/pending-diagnostic.h b/gcc/analyzer/pending-diagnostic.h index 9e1c656bf0a..4ea469e1879 100644 --- a/gcc/analyzer/pending-diagnostic.h +++ b/gcc/analyzer/pending-diagnostic.h @@ -58,6 +58,17 @@ struct event_desc bool m_colorize; }; +/* For use by pending_diagnostic::describe_region_creation. */ + +struct region_creation : public event_desc +{ + region_creation (bool colorize, const region *reg) + : event_desc (colorize), m_reg (reg) + {} + + const region *m_reg; +}; + /* For use by pending_diagnostic::describe_state_change. */ struct state_change : public event_desc @@ -215,6 +226,15 @@ class pending_diagnostic description; NULL otherwise (falling back on a more generic description). */ + /* Precision-of-wording vfunc for describing a region creation event + triggered by the mark_interesting_stuff vfunc. */ + virtual label_text + describe_region_creation_event (const evdesc::region_creation &) + { + /* Default no-op implementation. */ + return label_text (); + } + /* Precision-of-wording vfunc for describing a critical state change within the diagnostic_path. diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index 6b49719d521..893566a811b 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -73,6 +73,7 @@ along with GCC; see the file COPYING3. If not see #include "tree-ssa-operands.h" #include "ssa-iterators.h" #include "calls.h" +#include "is-a.h" #if ENABLE_ANALYZER @@ -2799,6 +2800,373 @@ region_model::check_region_for_read (const region *src_reg, check_region_access (src_reg, DIR_READ, ctxt); } +/* Concrete subclass for casts of pointers that lead to trailing bytes. */ + +class dubious_allocation_size +: public pending_diagnostic_subclass +{ +public: + dubious_allocation_size (const region *lhs, const region *rhs) + : m_lhs (lhs), m_rhs (rhs), m_expr (NULL_TREE) + {} + + dubious_allocation_size (const region *lhs, const region *rhs, + tree expr) + : m_lhs (lhs), m_rhs (rhs), m_expr (expr) + {} + + const char *get_kind () const final override + { + return "dubious_allocation_size"; + } + + bool operator== (const dubious_allocation_size &other) const + { + return m_lhs == other.m_lhs && m_rhs == other.m_rhs + && pending_diagnostic::same_tree_p (m_expr, other.m_expr); + } + + int get_controlling_option () const final override + { + return OPT_Wanalyzer_allocation_size; + } + + bool emit (rich_location *rich_loc) final override + { + diagnostic_metadata m; + m.add_cwe (131); + + return warning_meta (rich_loc, m, get_controlling_option (), + "allocated buffer size is not a multiple of the pointee's size"); + } + + label_text + describe_region_creation_event (const evdesc::region_creation &ev) final + override + { + m_allocation_event = &ev; + if (m_expr) + { + if (TREE_CODE (m_expr) == INTEGER_CST) + return ev.formatted_print ("allocated %E bytes here", m_expr); + else + return ev.formatted_print ("allocated %qE bytes here", m_expr); + } + + return ev.formatted_print ("allocated here"); + } + + label_text describe_final_event (const evdesc::final_event &ev) final + override + { + tree pointee_type = TREE_TYPE (m_lhs->get_type ()); + if (m_allocation_event) + /* Fallback: Typically, we should always + see an m_allocation_event before. */ + return ev.formatted_print ("assigned to %qT here;" + " % is %qE", + m_lhs->get_type (), pointee_type, + size_in_bytes (pointee_type)); + + if (m_expr) + { + if (TREE_CODE (m_expr) == INTEGER_CST) + return ev.formatted_print ("allocated %E bytes and assigned to" + " %qT here; % is %qE", + m_expr, m_lhs->get_type (), pointee_type, + size_in_bytes (pointee_type)); + else + return ev.formatted_print ("allocated %qE bytes and assigned to" + " %qT here; % is %qE", + m_expr, m_lhs->get_type (), pointee_type, + size_in_bytes (pointee_type)); + } + + return ev.formatted_print ("allocated and assigned to %qT here;" + " % is %qE", + m_lhs->get_type (), pointee_type, + size_in_bytes (pointee_type)); + } + + void mark_interesting_stuff (interesting_t *interest) final override + { + interest->add_region_creation (m_rhs); + } + +private: + const region *m_lhs; + const region *m_rhs; + const tree m_expr; + const evdesc::region_creation *m_allocation_event; +}; + +/* Return true on dubious allocation sizes for constant sizes. */ + +static bool +capacity_compatible_with_type (tree cst, tree pointee_size_tree, + bool is_struct) +{ + gcc_assert (TREE_CODE (cst) == INTEGER_CST); + gcc_assert (TREE_CODE (pointee_size_tree) == INTEGER_CST); + + unsigned HOST_WIDE_INT pointee_size = TREE_INT_CST_LOW (pointee_size_tree); + unsigned HOST_WIDE_INT alloc_size = TREE_INT_CST_LOW (cst); + + if (is_struct) + return alloc_size >= pointee_size; + return alloc_size % pointee_size == 0; +} + +static bool +capacity_compatible_with_type (tree cst, tree pointee_size_tree) +{ + return capacity_compatible_with_type (cst, pointee_size_tree, false); +} + +/* Checks whether SVAL could be a multiple of SIZE_CST. + + It works by visiting all svalues inside SVAL until it reaches + atomic nodes. From those, it goes back up again and adds each + node that might be a multiple of SIZE_CST to the RESULT_SET. */ + +class size_visitor : public visitor +{ +public: + size_visitor (tree size_cst, const svalue *sval, constraint_manager *cm) + : m_size_cst (size_cst), m_sval (sval), m_cm (cm) + { + sval->accept (this); + } + + bool get_result () + { + return result_set.contains (m_sval); + } + + void visit_constant_svalue (const constant_svalue *sval) final override + { + if (capacity_compatible_with_type (sval->get_constant (), m_size_cst)) + result_set.add (sval); + } + + void visit_unknown_svalue (const unknown_svalue *sval ATTRIBUTE_UNUSED) + final override + { + result_set.add (sval); + } + + void visit_poisoned_svalue (const poisoned_svalue *sval ATTRIBUTE_UNUSED) + final override + { + result_set.add (sval); + } + + void visit_unaryop_svalue (const unaryop_svalue *sval) + { + const svalue *arg = sval->get_arg (); + if (result_set.contains (arg)) + result_set.add (sval); + } + + void visit_binop_svalue (const binop_svalue *sval) final override + { + const svalue *arg0 = sval->get_arg0 (); + const svalue *arg1 = sval->get_arg1 (); + + if (sval->get_op () == MULT_EXPR) + { + if (result_set.contains (arg0) || result_set.contains (arg1)) + result_set.add (sval); + } + else + { + if (result_set.contains (arg0) && result_set.contains (arg1)) + result_set.add (sval); + } + } + + void visit_repeated_svalue (const repeated_svalue *sval) + { + sval->get_inner_svalue ()->accept (this); + if (result_set.contains (sval->get_inner_svalue ())) + result_set.add (sval); + } + + void visit_unmergeable_svalue (const unmergeable_svalue *sval) final override + { + sval->get_arg ()->accept (this); + if (result_set.contains (sval->get_arg ())) + result_set.add (sval); + } + + void visit_widening_svalue (const widening_svalue *sval) final override + { + const svalue *base = sval->get_base_svalue (); + const svalue *iter = sval->get_iter_svalue (); + + if (result_set.contains (base) && result_set.contains (iter)) + result_set.add (sval); + } + + void visit_conjured_svalue (const conjured_svalue *sval ATTRIBUTE_UNUSED) + final override + { + equiv_class_id id (-1); + if (m_cm->get_equiv_class_by_svalue (sval, &id)) + { + if (tree cst_val = id.get_obj (*m_cm).get_any_constant ()) + { + if (capacity_compatible_with_type (cst_val, m_size_cst)) + result_set.add (sval); + } + else + { + result_set.add (sval); + } + } + } + + void visit_asm_output_svalue (const asm_output_svalue *sval ATTRIBUTE_UNUSED) + final override + { + result_set.add (sval); + } + + void visit_const_fn_result_svalue (const const_fn_result_svalue + *sval ATTRIBUTE_UNUSED) final override + { + result_set.add (sval); + } + +private: + tree m_size_cst; + const svalue *m_sval; + constraint_manager *m_cm; + svalue_set result_set; /* Used as a mapping of svalue*->bool. */ +}; + +/* Return true if a struct or union either uses the inheritance pattern, + where the first field is a base struct, or the flexible array member + pattern, where the last field is an array without a specified size. */ + +static bool +struct_or_union_with_inheritance_p (tree struc) +{ + tree iter = TYPE_FIELDS (struc); + if (iter == NULL_TREE) + return false; + if (RECORD_OR_UNION_TYPE_P (TREE_TYPE (iter))) + return true; + + tree last_field; + while (iter != NULL_TREE) + { + last_field = iter; + iter = DECL_CHAIN (iter); + } + + if (last_field != NULL_TREE + && TREE_CODE (TREE_TYPE (last_field)) == ARRAY_TYPE) + return true; + + return false; +} + +/* Return true if the lhs and rhs of an assignment have different types. */ + +static bool +is_any_cast_p (const gimple *stmt) +{ + if (const gassign *assign = dyn_cast(stmt)) + return gimple_assign_cast_p (assign) + || !pending_diagnostic::same_tree_p ( + TREE_TYPE (gimple_assign_lhs (assign)), + TREE_TYPE (gimple_assign_rhs1 (assign))); + else if (const gcall *call = dyn_cast(stmt)) + { + tree lhs = gimple_call_lhs (call); + return lhs != NULL_TREE && !pending_diagnostic::same_tree_p ( + TREE_TYPE (gimple_call_lhs (call)), + gimple_call_return_type (call)); + } + + return false; +} + +/* On pointer assignments, check whether the buffer size of + RHS_SVAL is compatible with the type of the LHS_REG. + Use a non-null CTXT to report allocation size warnings. */ + +void +region_model::check_region_size (const region *lhs_reg, const svalue *rhs_sval, + region_model_context *ctxt) const +{ + if (!ctxt || ctxt->get_stmt () == NULL) + return; + /* Only report warnings on assignments that actually change the type. */ + if (!is_any_cast_p (ctxt->get_stmt ())) + return; + + const region_svalue *reg_sval = dyn_cast (rhs_sval); + if (!reg_sval) + return; + + tree pointer_type = lhs_reg->get_type (); + if (pointer_type == NULL_TREE || !POINTER_TYPE_P (pointer_type)) + return; + + tree pointee_type = TREE_TYPE (pointer_type); + /* Make sure that the type on the left-hand size actually has a size. */ + if (pointee_type == NULL_TREE || VOID_TYPE_P (pointee_type) + || TYPE_SIZE_UNIT (pointee_type) == NULL_TREE) + return; + + /* Bail out early on pointers to structs where we can + not deduce whether the buffer size is compatible. */ + bool is_struct = RECORD_OR_UNION_TYPE_P (pointee_type); + if (is_struct && struct_or_union_with_inheritance_p (pointee_type)) + return; + + tree pointee_size_tree = size_in_bytes (pointee_type); + /* We give up if the type size is not known at compile-time or the + type size is always compatible regardless of the buffer size. */ + if (TREE_CODE (pointee_size_tree) != INTEGER_CST + || integer_zerop (pointee_size_tree) + || integer_onep (pointee_size_tree)) + return; + + const region *rhs_reg = reg_sval->get_pointee (); + const svalue *capacity = get_capacity (rhs_reg); + switch (capacity->get_kind ()) + { + case svalue_kind::SK_CONSTANT: + { + const constant_svalue *cst_cap_sval + = as_a (capacity); + tree cst_cap = cst_cap_sval->get_constant (); + if (!capacity_compatible_with_type (cst_cap, pointee_size_tree, + is_struct)) + ctxt->warn (new dubious_allocation_size (lhs_reg, rhs_reg, + cst_cap)); + } + break; + default: + { + if (!is_struct) + { + size_visitor v (pointee_size_tree, capacity, m_constraints); + if (!v.get_result ()) + { + tree expr = get_representative_tree (capacity); + ctxt->warn (new dubious_allocation_size (lhs_reg, rhs_reg, + expr)); + } + } + break; + } + } +} + /* Set the value of the region given by LHS_REG to the value given by RHS_SVAL. Use CTXT to report any warnings associated with writing to LHS_REG. */ @@ -2810,6 +3178,8 @@ region_model::set_value (const region *lhs_reg, const svalue *rhs_sval, gcc_assert (lhs_reg); gcc_assert (rhs_sval); + check_region_size (lhs_reg, rhs_sval, ctxt); + check_region_for_write (lhs_reg, ctxt); m_store.set_value (m_mgr->get_store_manager(), lhs_reg, rhs_sval, diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h index 1bfa56a8cd2..91b7b370b81 100644 --- a/gcc/analyzer/region-model.h +++ b/gcc/analyzer/region-model.h @@ -857,6 +857,8 @@ class region_model region_model_context *ctxt) const; void check_region_for_read (const region *src_reg, region_model_context *ctxt) const; + void check_region_size (const region *lhs_reg, const svalue *rhs_sval, + region_model_context *ctxt) const; void check_call_args (const call_details &cd) const; void check_external_function_for_access_attr (const gcall *call, diff --git a/gcc/analyzer/svalue.cc b/gcc/analyzer/svalue.cc index 2f9149412b9..7bad3cea31b 100644 --- a/gcc/analyzer/svalue.cc +++ b/gcc/analyzer/svalue.cc @@ -732,8 +732,8 @@ region_svalue::dump_to_pp (pretty_printer *pp, bool simple) const void region_svalue::accept (visitor *v) const { - v->visit_region_svalue (this); m_reg->accept (v); + v->visit_region_svalue (this); } /* Implementation of svalue::implicitly_live_p vfunc for region_svalue. */ @@ -1031,8 +1031,8 @@ initial_svalue::dump_to_pp (pretty_printer *pp, bool simple) const void initial_svalue::accept (visitor *v) const { - v->visit_initial_svalue (this); m_reg->accept (v); + v->visit_initial_svalue (this); } /* Implementation of svalue::implicitly_live_p vfunc for initial_svalue. */ @@ -1123,8 +1123,8 @@ unaryop_svalue::dump_to_pp (pretty_printer *pp, bool simple) const void unaryop_svalue::accept (visitor *v) const { - v->visit_unaryop_svalue (this); m_arg->accept (v); + v->visit_unaryop_svalue (this); } /* Implementation of svalue::implicitly_live_p vfunc for unaryop_svalue. */ @@ -1225,9 +1225,9 @@ binop_svalue::dump_to_pp (pretty_printer *pp, bool simple) const void binop_svalue::accept (visitor *v) const { - v->visit_binop_svalue (this); m_arg0->accept (v); m_arg1->accept (v); + v->visit_binop_svalue (this); } /* Implementation of svalue::implicitly_live_p vfunc for binop_svalue. */ @@ -1283,9 +1283,9 @@ sub_svalue::dump_to_pp (pretty_printer *pp, bool simple) const void sub_svalue::accept (visitor *v) const { - v->visit_sub_svalue (this); m_parent_svalue->accept (v); m_subregion->accept (v); + v->visit_sub_svalue (this); } /* Implementation of svalue::implicitly_live_p vfunc for sub_svalue. */ @@ -1352,8 +1352,8 @@ repeated_svalue::dump_to_pp (pretty_printer *pp, bool simple) const void repeated_svalue::accept (visitor *v) const { - v->visit_repeated_svalue (this); m_inner_svalue->accept (v); + v->visit_repeated_svalue (this); } /* Implementation of svalue::all_zeroes_p for repeated_svalue. */ @@ -1494,8 +1494,8 @@ bits_within_svalue::maybe_fold_bits_within (tree type, void bits_within_svalue::accept (visitor *v) const { - v->visit_bits_within_svalue (this); m_inner_svalue->accept (v); + v->visit_bits_within_svalue (this); } /* Implementation of svalue::implicitly_live_p vfunc for bits_within_svalue. */ @@ -1544,9 +1544,9 @@ widening_svalue::dump_to_pp (pretty_printer *pp, bool simple) const void widening_svalue::accept (visitor *v) const { - v->visit_widening_svalue (this); m_base_sval->accept (v); m_iter_sval->accept (v); + v->visit_widening_svalue (this); } /* Attempt to determine in which direction this value is changing @@ -1711,8 +1711,8 @@ unmergeable_svalue::dump_to_pp (pretty_printer *pp, bool simple) const void unmergeable_svalue::accept (visitor *v) const { - v->visit_unmergeable_svalue (this); m_arg->accept (v); + v->visit_unmergeable_svalue (this); } /* Implementation of svalue::implicitly_live_p vfunc for unmergeable_svalue. */ @@ -1776,13 +1776,13 @@ compound_svalue::dump_to_pp (pretty_printer *pp, bool simple) const void compound_svalue::accept (visitor *v) const { - v->visit_compound_svalue (this); for (binding_map::iterator_t iter = m_map.begin (); iter != m_map.end (); ++iter) { //(*iter).first.accept (v); (*iter).second->accept (v); } + v->visit_compound_svalue (this); } /* Calculate what the complexity of a compound_svalue instance for MAP @@ -1903,8 +1903,8 @@ conjured_svalue::dump_to_pp (pretty_printer *pp, bool simple) const void conjured_svalue::accept (visitor *v) const { - v->visit_conjured_svalue (this); m_id_reg->accept (v); + v->visit_conjured_svalue (this); } /* class asm_output_svalue : public svalue. */ @@ -1968,9 +1968,9 @@ asm_output_svalue::input_idx_to_asm_idx (unsigned input_idx) const void asm_output_svalue::accept (visitor *v) const { - v->visit_asm_output_svalue (this); for (unsigned i = 0; i < m_num_inputs; i++) m_input_arr[i]->accept (v); + v->visit_asm_output_svalue (this); } /* class const_fn_result_svalue : public svalue. */ @@ -2021,9 +2021,9 @@ const_fn_result_svalue::dump_input (pretty_printer *pp, void const_fn_result_svalue::accept (visitor *v) const { - v->visit_const_fn_result_svalue (this); for (unsigned i = 0; i < m_num_inputs; i++) m_input_arr[i]->accept (v); + v->visit_const_fn_result_svalue (this); } } // namespace ana diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 60b7b5a26bb..ddf5125a2b1 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -9711,6 +9711,7 @@ This analysis is much more expensive than other GCC warnings. Enabling this option effectively enables the following warnings: @gccoptlist{ @gol +-Wanalyzer-allocation-size @gol -Wanalyzer-double-fclose @gol -Wanalyzer-double-free @gol -Wanalyzer-exposure-through-output-file @gol @@ -9758,6 +9759,19 @@ By default, the analysis silently stops if the code is too complicated for the analyzer to fully explore and it reaches an internal limit. The @option{-Wanalyzer-too-complex} option warns if this occurs. +@item -Wno-analyzer-allocation-size +@opindex Wanalyzer-allocation-size +@opindex Wno-analyzer-allocation-size +This warning requires @option{-fanalyzer}, which enables it; use +@option{-Wno-analyzer-allocation-size} +to disable it. + +This diagnostic warns for paths through the code in which a pointer to +a buffer is assigned to point at a buffer with a size that is not a +multiple of @code{sizeof (*pointer)}. + +See @url{https://cwe.mitre.org/data/definitions/131.html, CWE-131: Incorrect Calculation of Buffer Size}. + @item -Wno-analyzer-double-fclose @opindex Wanalyzer-double-fclose @opindex Wno-analyzer-double-fclose diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c b/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c new file mode 100644 index 00000000000..4fc2bf75d6c --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c @@ -0,0 +1,116 @@ +#include +#include + +/* Tests with constant buffer sizes. */ + +void test_1 (void) +{ + short *ptr = malloc (21 * sizeof (short)); + free (ptr); +} + +void test_2 (void) +{ + int *ptr = malloc (21 * sizeof (short)); /* { dg-line malloc2 } */ + free (ptr); + + /* { dg-warning "allocated buffer size is not a multiple of the pointee's size \\\[CWE-131\\\]" "warning" { target *-*-* } malloc2 } */ + /* { dg-message "\\d+ bytes" "note" { target *-*-* } malloc2 } */ + /* { dg-message "'int \\*' here; 'sizeof \\(int\\)' is '\\d+'" "note" { target *-*-* } malloc2 } */ +} + +void test_3 (void) +{ + void *ptr = malloc (21 * sizeof (short)); + short *sptr = (short *)ptr; + free (sptr); +} + +void test_4 (void) +{ + void *ptr = malloc (21 * sizeof (short)); /* { dg-message "\\d+ bytes" } */ + int *iptr = (int *)ptr; /* { dg-line assign4 } */ + free (iptr); + + /* { dg-warning "allocated buffer size is not a multiple of the pointee's size \\\[CWE-131\\\]" "warning" { target *-*-* } assign4 } */ + /* { dg-message "'int \\*' here; 'sizeof \\(int\\)' is '\\d+'" "note" { target *-*-* } assign4 } */ +} + +void test_5 (void) +{ + int user_input; + scanf("%i", &user_input); + int n; + if (user_input == 0) + n = 21 * sizeof (short); + else + n = 42 * sizeof (short); + void *ptr = malloc (n); + short *sptr = (short *)ptr; + free (sptr); +} + +void test_6 (void) +{ + int user_input; + scanf("%i", &user_input); + int n; + if (user_input == 0) + n = 21 * sizeof (short); + else + n = 42 * sizeof (short); + void *ptr = malloc (n); /* { dg-message "" "note" } */ + /* ^^^ on widening_svalues no expr is returned + by get_representative_tree at the moment. */ + int *iptr = (int *)ptr; /* { dg-line assign6 } */ + free (iptr); + + /* { dg-warning "allocated buffer size is not a multiple of the pointee's size \\\[CWE-131\\\]" "warning" { target *-*-* } assign6 } */ + /* { dg-message "'int \\*' here; 'sizeof \\(int\\)' is '\\d+'" "note" { target *-*-* } assign6 } */ +} + +void test_7 (void) +{ + int user_input; + scanf("%i", &user_input); + int n; + if (user_input == 0) + n = 1; + else if (user_input == 2) + n = 5; + else + n = 7; + /* n is an unknown_svalue at this point. */ + void *ptr = malloc (n); + int *iptr = (int *)ptr; + free (iptr); +} + +void *create_buffer (int n) +{ + return malloc(n); +} + +void test_8 (void) +{ + int *buf = create_buffer(4 * sizeof (int)); + free (buf); +} + +void test_9 (void) +{ + /* FIXME: At the moment, region_model::set_value (lhs, ) + is called at the src_node of the return edge. This edge has no stmts + associated with it, leading to a rejection of the warning inside + impl_region_model_context::warn. To ensure that the indentation + in the diagnostic is right, the warning has to be emitted on an EN + that is after the return edge. */ + int *buf = create_buffer(42); /* { dg-warning "" "" { xfail *-*-* } } */ + free (buf); +} + +void test_10 (int n) +{ + char *ptr = malloc (7 * n); + free (ptr); +} \ No newline at end of file diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c b/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c new file mode 100644 index 00000000000..37bbbac87c5 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c @@ -0,0 +1,155 @@ +#include +#include + +/* Tests with symbolic buffer sizes. */ + +void test_1 (int n) +{ + short *ptr = malloc (n * sizeof (short)); + free (ptr); +} + +void test_2 (int n) +{ + int *ptr = malloc (n * sizeof (short)); /* { dg-line malloc2 } */ + free (ptr); + + /* { dg-warning "allocated buffer size is not a multiple of the pointee's size \\\[CWE-131\\\]" "warning" { target *-*-* } malloc2 } */ + /* { dg-message "'\[a-z0-9\\*\\(\\)\\s\]*' bytes" "note" { target *-*-* } malloc2 } */ + /* { dg-message "'int \\*' here; 'sizeof \\(int\\)' is '\\d+'" "note" { target *-*-* } malloc2 } */ +} + +void test_3 (int n) +{ + void *ptr = malloc (n * sizeof (short)); + short *sptr = (short *)ptr; + free (sptr); +} + +void test_4 (int n) +{ + void *ptr = malloc (n * sizeof (short)); /* { dg-message "'\[a-z0-9\\*\\(\\)\\s\]*'" "note" } */ + int *iptr = (int *)ptr; /* { dg-line assign4 } */ + free (iptr); + + /* { dg-warning "allocated buffer size is not a multiple of the pointee's size \\\[CWE-131\\\]" "warning" { target *-*-* } assign4 } */ + /* { dg-message "'int \\*' here; 'sizeof \\(int\\)' is '\\d+'" "note" { target *-*-* } assign4 } */ +} + +void test_5 (void) +{ + int user_input; + scanf("%i", &user_input); + int n; + if (user_input == 0) + n = 3 * user_input * sizeof (short); + else + n = 5 * user_input * sizeof (short); + void *ptr = malloc (n); + short *sptr = (short *)ptr; + free (sptr); +} + +void test_6 (void) +{ + int user_input; + scanf("%i", &user_input); + int n; + if (user_input == 0) + n = user_input; + else if (user_input == 2) + n = user_input * 3; + else + n = user_input * 5; + /* n is an unknown_svalue at this point. */ + void *ptr = malloc (n); + int *iptr = (int *)ptr; + free (iptr); +} + +void *create_buffer(int n) +{ + return malloc(n); +} + +void test_7(int n) +{ + int *buf = create_buffer(n * sizeof (int)); + free (buf); +} + +void test_8(int n) +{ + /* FIXME: At the moment, region_model::set_value (lhs, ) + is called at the src_node of the return edge. This edge has no stmts + associated with it, leading to a rejection of the warning inside + impl_region_model_context::warn. To ensure that the indentation + in the diagnostic is right, the warning has to be emitted on an EN + that is after the return edge. */ + int *buf = create_buffer(n * sizeof(short)); /* { dg-warning "" "" { xfail *-*-* } } */ + free (buf); +} + +void test_9 (void) +{ + int n; + scanf("%i", &n); + /* n is a conjured_svalue. */ + void *ptr = malloc (n); /* { dg-message "'n' bytes" "note" } */ + int *iptr = (int *)ptr; /* { dg-line assign9 } */ + free (iptr); + + /* { dg-warning "allocated buffer size is not a multiple of the pointee's size \\\[CWE-131\\\]" "warning" { target *-*-* } assign9 } */ + /* { dg-message "'int \\*' here; 'sizeof \\(int\\)' is '\\d+'" "note" { target *-*-* } assign9 } */ +} + +void test_11 (void) +{ + int n; + scanf("%i", &n); + void *ptr = malloc (n); + if (n == sizeof (int)) + { + /* n is a conjured_svalue but guarded such that we + know the value is a multiple of sizeof (*iptr). */ + int *iptr = (int *)ptr; + free (iptr); + } + else + free (ptr); +} + +void test_12 (void) +{ + int n; + scanf("%i", &n); + void *ptr = malloc (n); /* { dg-message "'n' bytes" } */ + if (n == 5) + { + /* n is a conjured_svalue but guarded such that we + know the value isn't a multiple of sizeof (*iptr). */ + int *iptr = (int *)ptr; /* { dg-line assign12 } */ + free (iptr); + } + else + free (ptr); + /* { dg-warning "allocated buffer size is not a multiple of the pointee's size \\\[CWE-131\\\]" "warning" { target *-*-* } assign12 } */ + /* { dg-message "'int \\*' here; 'sizeof \\(int\\)' is '\\d+'" "note" { target *-*-* } assign12 } */ +} + +void test_13 (void) +{ + int n; + scanf("%i", &n); + void *ptr = malloc (n); + if (n == n * n) + { + /* n is a conjured_svalue but guarded such that we don't have an + equivalence class for it. In such cases, we assume that the + condition ensures that the value is okay. */ + int *iptr = (int *)ptr; + free (iptr); + } + else + free (ptr); +} diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-3.c b/gcc/testsuite/gcc.dg/analyzer/allocation-size-3.c new file mode 100644 index 00000000000..fdc1c56b7ee --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-3.c @@ -0,0 +1,45 @@ +#include +#include + +/* CWE-131 example 5 */ +void test_1 (void) +{ + int *id_sequence = (int *) malloc (3); /* { dg-line malloc1 } */ + if (id_sequence == NULL) exit (1); + + id_sequence[0] = 13579; + id_sequence[1] = 24680; + id_sequence[2] = 97531; + + free (id_sequence); + + /* { dg-warning "allocated buffer size is not a multiple of the pointee's size \\\[CWE-131\\\]" "warning" { target *-*-* } malloc1 } */ + /* { dg-message "\\d+ bytes" "note" { target *-*-* } malloc1 } */ + /* { dg-message "'int \\*' here; 'sizeof \\(int\\)' is '\\d+'" "note" { target *-*-* } malloc1 } */ +} + +void test_2 (void) +{ + int *ptr = malloc (10 + sizeof(int)); /* { dg-line malloc2 } */ + free (ptr); + + /* { dg-warning "allocated buffer size is not a multiple of the pointee's size \\\[CWE-131\\\]" "warning" { target *-*-* } malloc2 } */ + /* { dg-message "\\d+ bytes" "note" { target *-*-* } malloc2 } */ + /* { dg-message "'int \\*' here; 'sizeof \\(int\\)' is '\\d+'" "note" { target *-*-* } malloc2 } */ +} + +void test_3 (int n) +{ + int *ptr = malloc (n + sizeof (int)); /* { dg-line malloc3 } */ + free (ptr); + + /* { dg-warning "allocated buffer size is not a multiple of the pointee's size \\\[CWE-131\\\]" "warning" { target *-*-* } malloc3 } */ + /* { dg-message "'\[a-z0-9\\+\\(\\)\\s\]*' bytes" "note" { target *-*-* } malloc3 } */ + /* { dg-message "'int \\*' here; 'sizeof \\(int\\)' is '\\d+'" "note" { target *-*-* } malloc3 } */ +} + +void test_4 (int n, int m) +{ + int *ptr = malloc ((n + m) * sizeof (int)); + free (ptr); +} diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-4.c b/gcc/testsuite/gcc.dg/analyzer/allocation-size-4.c new file mode 100644 index 00000000000..e475c1586a3 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-4.c @@ -0,0 +1,60 @@ +#include + +/* Tests related to structs. */ + +struct base { + int i; +}; + +struct sub { + struct base b; + int j; +}; + +struct var_len { + int i; + char arr[]; +}; + + +void test_1 (void) +{ + struct base *ptr = malloc (5 * sizeof (struct base)); + free (ptr); +} + +void test_2 (void) +{ + long *ptr = malloc (5 * sizeof (struct base)); /* { dg-line malloc2 } */ + free (ptr); + + /* { dg-warning "allocated buffer size is not a multiple of the pointee's size \\\[CWE-131\\\]" "warning" { target *-*-* } malloc2 } */ + /* { dg-message "\\d+ bytes" "note" { target *-*-* } malloc2 } */ + /* { dg-message "'long (int)? \\*' here; 'sizeof \\(long (int)?\\)' is '\\d+'" "note" { target *-*-* } malloc2 } */ +} + +void test_3 (void) +{ + /* Even though 10 bytes is not a multiple of 4, we do not warn to prevent + a false positive in case s is the base struct of a struct inheritance. */ + struct base *ptr = malloc (10); + free (ptr); +} + +void test_4 (void) +{ + struct var_len *ptr = malloc (10); + free (ptr); +} + +void test_5 (void) +{ + /* For constant sizes, we warn if the buffer + is too small to hold a single struct. */ + struct base *ptr = malloc (2); /* { dg-line malloc5 } */ + free (ptr); + + /* { dg-warning "allocated buffer size is not a multiple of the pointee's size \\\[CWE-131\\\]" "warning" { target *-*-* } malloc5 } */ + /* { dg-message "\\d+ bytes" "note" { target *-*-* } malloc5 } */ + /* { dg-message "'struct base \\*' here; 'sizeof \\(struct base\\)' is '\\d+'" "note" { target *-*-* } malloc5 } */ +} diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-5.c b/gcc/testsuite/gcc.dg/analyzer/allocation-size-5.c new file mode 100644 index 00000000000..ae7e1074ebb --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-5.c @@ -0,0 +1,36 @@ +#include +#include + +/* Tests related to statically allocated buffers. */ + +typedef struct a { + short s; +} a; + +int *test_1 (void) +{ + a A; /* { dg-message "\\d+ bytes" "note" } */ + A.s = 1; + int *ptr = (int *) &A; /* { dg-line assign1 } */ + return ptr; + + /* { dg-warning "allocated buffer size is not a multiple of the pointee's size \\\[CWE-131\\\]" "warning" { target *-*-* } assign1 } */ + /* { dg-message "assigned to 'int \\*' here; 'sizeof \\(int\\)' is '\\d+'" "note" { target *-*-* } assign1 } */ +} + +int *test2 (void) +{ + char arr[sizeof (int)]; + int *ptr = (int *)arr; + return ptr; +} + +int *test3 (void) +{ + char arr[sizeof (short)]; /* { dg-message "\\d+ bytes" "note" } */ + int *ptr = (int *)arr; /* { dg-line assign3 } */ + return ptr; + + /* { dg-warning "allocated buffer size is not a multiple of the pointee's size \\\[CWE-131\\\]" "warning" { target *-*-* } assign3 } */ + /* { dg-message "assigned to 'int \\*' here; 'sizeof \\(int\\)' is '\\d+'" "note" { target *-*-* } assign3 } */ +} diff --git a/gcc/testsuite/gcc.dg/analyzer/pr96639.c b/gcc/testsuite/gcc.dg/analyzer/pr96639.c index 02ca3f084a2..aedf0464dc9 100644 --- a/gcc/testsuite/gcc.dg/analyzer/pr96639.c +++ b/gcc/testsuite/gcc.dg/analyzer/pr96639.c @@ -3,7 +3,7 @@ void *calloc (__SIZE_TYPE__, __SIZE_TYPE__); int x7 (void) { - int **md = calloc (1, 1); + int **md = calloc (1, sizeof (void *)); return md[0][0]; /* { dg-warning "possibly-NULL" "unchecked deref" } */ /* { dg-warning "leak of 'md'" "leak" { target *-*-* } .-1 } */ -- 2.36.1