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 AB1303850203 for ; Wed, 29 Jun 2022 15:40:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org AB1303850203 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:Content-Type: MIME-Version:References:In-Reply-To:Message-Id:Date:Subject:Cc:To:From:Sender :Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID; bh=BPrvOnzME+fZMprXi3/h6a7JZ3i964m80nS5timtTTA=; b=VSfzcvGdSkQZnacJG3BhZ1L6bA ICG0moaHtDRhk9kG6eFisQfFLqePnK9x1eoUrXwNYmWrhlSE0S4aqW+128XuDxK2BYVILHw3FqXsP 9Cwslc16QooSJH5wnldbvJVl5xdTD5SYdtrEoSiivvN/UFQxioglRcBKcJ7QBCxxo6SRafj8HD+hI Cczgpj2P6v2FOlAoK4oQeJkIiJ4lzk4akNntQuRWUAHj+fb01r6fw+doxZkEKE0PyNqXiGzYsuwOu WoM/hHa90+3G15SXXzb60TT/daDkJ7c84XnS+Ng5oVAQ6e7dHi/2tHibcbzPifd+xpwqht/HoNUJM NfqSQHcA==; Received: from sslproxy05.your-server.de ([78.46.172.2]) by www523.your-server.de with esmtpsa (TLSv1.3:TLS_AES_256_GCM_SHA384:256) (Exim 4.92.3) (envelope-from ) id 1o6ZnU-000Atx-UH; Wed, 29 Jun 2022 17:40:09 +0200 Received: from [2a02:908:1861:d6a0::b718] (helo=fedora..) by sslproxy05.your-server.de with esmtpsa (TLSv1.3:TLS_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1o6ZnU-000MgW-N7; Wed, 29 Jun 2022 17:40:08 +0200 From: Tim Lange To: dmalcolm@redhat.com Cc: gcc@gcc.gnu.org, Tim Lange Subject: [PATCH v2] analyzer: add allocation size checker Date: Wed, 29 Jun 2022 17:39:49 +0200 Message-Id: <20220629153949.74450-1-mail@tim-lange.me> X-Mailer: git-send-email 2.36.1 In-Reply-To: References: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Authenticated-Sender: mail@tim-lange.me X-Virus-Scanned: Clear (ClamAV 0.103.6/26588/Wed Jun 29 10:21:17 2022) X-Spam-Status: No, score=-11.2 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: Wed, 29 Jun 2022 15:40:16 -0000 Hi, I've addressed most of the points from the review. * The allocation size warning warns whenever region_model::get_capacity returns something useful, i.e. also on statically-allocated regions. * I added a new virtual function to the pending-diagnostic class, so that it is possible to emit a custom region creation description. * The test cases should have a better coverage now. * Conservative struct handling The warning now looks like this: /path/to/main.c:9:8: warning: allocated buffer size is not a multiple of the pointee's size [CWE-131] [-Wanalyzer-allocation-size] 9 | int *iptr = ptr; | ^~~~ ‘main’: events 1-2 | | 8 | void *ptr = malloc((long unsigned int)n * sizeof(short)); | | ^~~~~~~~~~~~~~~~~~~~~~~~~ | | | | | (1) allocated ‘(long unsigned int)n * 2’ bytes here | 9 | int *iptr = ptr; | | ~~~~ | | | | | (2) assigned to ‘int *’ here; ‘sizeof(int)’ is ‘4’ | /path/to/main.c:15:15: warning: allocated buffer size is not a multiple of the pointee's size [CWE-131] [-Wanalyzer-allocation-size] 15 | int *ptrw = malloc (sizeof (short)); | ^~~~~~~~~~~~~~~~~~~~~~~ ‘main’: events 1-2 | | 15 | int *ptrw = malloc (sizeof (short)); | | ^~~~~~~~~~~~~~~~~~~~~~~ | | | | | (1) allocated ‘2’ bytes here | | (2) assigned to ‘int *’ here; ‘sizeof (int)’ is ‘4’ | The only thing I couldn't address was moving the second event toward the lhs or assign token here. I tracked it down till get_stmt_location where it seems that the rhs is actually the location of the statement. Is there any way to get (2) to be focused on the lhs? Otherwise, the patch compiled coreutils, openssh, curl and httpd without any false-positive (but none of them contained a bug found by the checker either). `make check-gcc RUNTESTFLAGS="analyzer.exp"` tests pass and as I just worked on the event splitting, the regression tests are yet to run. - 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. gcc/analyzer/ChangeLog: * 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: * doc/invoke.texi: Added Wanalyzer-allocation-size. gcc/testsuite/ChangeLog: * 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 | 344 ++++++++++++++++++ gcc/analyzer/region-model.h | 2 + gcc/analyzer/svalue.cc | 26 +- gcc/doc/invoke.texi | 13 + .../gcc.dg/analyzer/allocation-size-1.c | 102 ++++++ .../gcc.dg/analyzer/allocation-size-2.c | 125 +++++++ .../gcc.dg/analyzer/allocation-size-3.c | 48 +++ .../gcc.dg/analyzer/allocation-size-4.c | 58 +++ .../gcc.dg/analyzer/allocation-size-5.c | 40 ++ gcc/testsuite/gcc.dg/analyzer/pr96639.c | 2 +- 16 files changed, 846 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..912def2faf2 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 buffer is assigned to a 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..7805aa26752 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,347 @@ 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; + } + + 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) + 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)); + else + if (m_expr) + 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)); + else + 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) +{ + unsigned HOST_WIDE_INT pointee_size = TREE_INT_CST_LOW (pointee_size_tree); + if (pointee_size == 0) + return 0; + 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; +} + +/* 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 + { + unsigned HOST_WIDE_INT sval_int + = TREE_INT_CST_LOW (sval->get_constant ()); + unsigned HOST_WIDE_INT size_cst_int = TREE_INT_CST_LOW (m_size_cst); + if (size_cst_int == 0 || sval_int % size_cst_int == 0) + 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 + { + if (m_cm->get_equiv_class_by_svalue (sval, NULL)) + 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) + || (gimple_num_ops (assign) == 2 + && !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 + || TREE_INT_CST_LOW (pointee_size_tree) == 1) + 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 +3152,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..cf77e9b3a43 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,18 @@ 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 buffer is casted +to a type where the buffer size is not a multiple of the pointee size. + +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..02634ae883b --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c @@ -0,0 +1,102 @@ +#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 "" "" { target *-*-* } malloc2 } */ + /* { dg-message "" "" { 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 } */ + int *iptr = (int *)ptr; /* { dg-line assign4 } */ + free (iptr); + + /* { dg-warning "" "" { target *-*-* } assign4 } */ + /* { dg-message "" "" { 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 } */ + int *iptr = (int *)ptr; /* { dg-line assign6 } */ + free (iptr); + + /* { dg-warning "" "" { target *-*-* } assign6 } */ + /* { dg-message "" "" { 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 + int *buf = create_buffer(42); /* { dg-warning "" "" { xfail *-*-* } } */ + free (buf); +} 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..cb35a9d717b --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c @@ -0,0 +1,125 @@ +#include +#include + +/* Tests with symbolic buffer sizes. */ + +void test_1 (void) +{ + int n; + scanf("%i", &n); + short *ptr = malloc (n * sizeof (short)); + free (ptr); +} + +void test_2 (void) +{ + int n; + scanf("%i", &n); + int *ptr = malloc (n * sizeof (short)); /* { dg-line malloc2 } */ + free (ptr); + + /* { dg-warning "" "" { target *-*-* } malloc2 } */ + /* { dg-message "" "" { target *-*-* } malloc2 } */ +} + +void test_3 (void) +{ + int n; + scanf("%i", &n); + void *ptr = malloc (n * sizeof (short)); + short *sptr = (short *)ptr; + free (sptr); +} + +void test_4 (void) +{ + int n; + scanf("%i", &n); + void *ptr = malloc (n * sizeof (short)); /* { dg-message } */ + int *iptr = (int *)ptr; /* { dg-line assign4 } */ + free (iptr); + + /* { dg-warning "" "" { target *-*-* } assign4 } */ + /* { dg-message "" "" { 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(void) +{ + int n; + scanf("%i", &n); + int *buf = create_buffer(n * sizeof (int)); + free (buf); +} + +void test_8(void) +{ + int n; + scanf("%i", &n); + // FIXME + 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 } */ + int *iptr = (int *)ptr; /* { dg-line assign9 } */ + free (iptr); + + /* { dg-warning "" "" { target *-*-* } assign9 } */ + /* { dg-message "" "" { target *-*-* } assign9 } */ +} + +void test_11 (void) +{ + int n; + scanf("%i", &n); + void *ptr = malloc (n); + if (n == 4) + { + /* n is a conjured_svalue but guarded. */ + 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..dafc0e73c63 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-3.c @@ -0,0 +1,48 @@ +#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 "" "" { target *-*-* } malloc1 } */ + /* { dg-message "" "" { target *-*-* } malloc1 } */ +} + +void test_2(void) +{ + int *ptr = malloc (10 + sizeof(int)); /* { dg-line malloc2 } */ + free (ptr); + + /* { dg-warning "" "" { target *-*-* } malloc2 } */ + /* { dg-message "" "" { target *-*-* } malloc2 } */ +} + +void test_3(void) +{ + int n; + scanf("%i", &n); + int *ptr = malloc (n + sizeof (int)); /* { dg-line malloc3 } */ + free (ptr); + + /* { dg-warning "" "" { target *-*-* } malloc3 } */ + /* { dg-message "" "" { target *-*-* } malloc3 } */ +} + +void test_4(void) +{ + int n; + scanf("%i", &n); + int m; + scanf("%i", &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..7f992eb8c3e --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-4.c @@ -0,0 +1,58 @@ +#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 "" "" { target *-*-* } malloc2 } */ + /* { dg-message "" "" { 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 "" "" { target *-*-* } malloc5 } */ + /* { dg-message "" "" { 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..afb1782e0cd --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-5.c @@ -0,0 +1,40 @@ +#include +#include + +/* Tests related to structs. */ + +typedef struct a { + short s; +} a; + +int *test_1 (void) +{ + a A; + A.s = 1; + int *ptr = (int *) &A; /* { dg-line assign1 } */ + return ptr; + + /* { dg-warning "" "" { target *-*-* } assign1 } */ + /* { dg-message "" "" { target *-*-* } assign1 } */ +} + +int *test2 (void) +{ + char arr[4]; + int *ptr = (int *)arr; + return ptr; +} + +int *test3 (void) +{ + char arr[2]; + int *ptr = (int *)arr; /* { dg-line assign3 } */ + return ptr; + + /* { dg-warning "" "" { target *-*-* } assign3 } */ + /* { dg-message "" "" { target *-*-* } assign3 } */ +} + +int main() { + return 0; +} 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