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 A402C3858D28 for ; Thu, 18 Aug 2022 09:45:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A402C3858D28 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=XgBuKYPgjtZx070D1VnyyfpD7KEWYrWVoyE3RPC3D84=; b=CuVdhgCfw5S5rTIs5XHo8FRpgv F/dITDzGvdXu+Dl+hm72b7UGk8nhW5OSvSVysxznDA2ry4bvGdSFZVm7Gvagln3HoCyngFlS2htVS TktjqA276yK77hVI/cb46TwpCvYeLArWuG+okdxj+ky/1XWLP6O+n32QGHeYatFAZupbfHXrCduIc Aymu6vxSwI92LIzYKdTmYD4JDOxRVkInYOgNO17kjU4wdFiagxUaiYjltcp9/Me23pYdp8Qaq70cT 5rZA7s3rDZoi1Z3Buv/HPno9tCrEf6EK1eYZ27SXFXdxpYpjDYgJryiQNneKYUEx8Cg0upCM9R5Z9 lJnRftXQ==; 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 1oOc5B-0001jC-UV; Thu, 18 Aug 2022 11:44:57 +0200 Received: from [2a02:908:1861:d6a0::6b5] (helo=fedora..) by sslproxy03.your-server.de with esmtpsa (TLSv1.3:TLS_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oOc5B-0005Vb-NL; Thu, 18 Aug 2022 11:44:57 +0200 From: Tim Lange To: gcc-patches@gcc.gnu.org Cc: dmalcolm@redhat.com, Tim Lange Subject: [PATCH v2] analyzer: warn on the use of floating-points operands in the size argument [PR106181] Date: Thu, 18 Aug 2022 11:44:14 +0200 Message-Id: <20220818094414.19488-1-mail@tim-lange.me> X-Mailer: git-send-email 2.37.1 In-Reply-To: <20220815123525.49172-1-mail@tim-lange.me> References: <20220815123525.49172-1-mail@tim-lange.me> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Authenticated-Sender: mail@tim-lange.me X-Virus-Scanned: Clear (ClamAV 0.103.6/26631/Thu Aug 18 09:52:06 2022) X-Spam-Status: No, score=-12.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-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 18 Aug 2022 09:45:04 -0000 Hi, this is the revised version of my patch. I had trouble to get your point regarding the float_visitor: > If the constant is seen first, then the non-constant won't be favored > (though perhaps binary ops get canonicalized so that constants are on > the RHS?). Only the assignment of m_result in visit_constant_svalue is guarded by !m_result, while the other two are not. So, there are two possibilities: 1. A constant is seen first and then assigned to m_result. 1.1. A non-constant float operand is seen later and overwrites m_result. 1.2. There's no non-constant float operand, thus the constant is the actual floating-point operand and is kept inside m_result. 2. A non-constant is seen first, then m_result might be overwritten with another non-constant later but never with a constant. Do I have a flaw in my thinking? (But they do seem to get canonicalized, so that shouldn't matter) > How about: > -Wanalyzer-imprecise-float-arithmetic > -Wanalyzer-imprecise-fp-arithmetic > instead? (ideas welcome) I've chosen the second. I mostly tried to avoid float because it is also a reserved keyword in many languages and I wanted to avoid confusion (might be overthinking that). - Tim This patch fixes the ICE reported in PR106181 and adds a new warning to the analyzer complaining about the use of floating-point operands. Regrtested on Linux x86_64. 2022-08-17 Tim Lange gcc/analyzer/ChangeLog: PR analyzer/106181 * analyzer.opt: Add Wanalyzer-imprecise-floating-point-arithmetic. * region-model.cc (is_any_cast_p): Formatting. (region_model::check_region_size): Ensure precondition. (class imprecise_floating_point_arithmetic): New abstract diagnostic class for all floating-point related warnings. (class float_as_size_arg): Concrete diagnostic class to complain about floating-point operands inside the size argument. (class contains_floating_point_visitor): New visitor to find floating-point operands inside svalues. (region_model::check_dynamic_size_for_floats): New function. (region_model::set_dynamic_extents): Call to check_dynamic_size_for_floats. * region-model.h (class region_model): Add region_model::check_dynamic_size_for_floats. gcc/ChangeLog: PR analyzer/106181 * doc/invoke.texi: Add Wanalyzer-imprecise-fp-arithmetic. gcc/testsuite/ChangeLog: PR analyzer/106181 * gcc.dg/analyzer/allocation-size-1.c: New test. * gcc.dg/analyzer/imprecise-floating-point-1.c: New test. * gcc.dg/analyzer/pr106181.c: New test. --- gcc/analyzer/analyzer.opt | 4 + gcc/analyzer/region-model.cc | 179 +++++++++++++++--- gcc/analyzer/region-model.h | 2 + gcc/doc/invoke.texi | 14 ++ .../gcc.dg/analyzer/allocation-size-1.c | 10 + .../analyzer/imprecise-floating-point-1.c | 74 ++++++++ gcc/testsuite/gcc.dg/analyzer/pr106181.c | 11 ++ 7 files changed, 271 insertions(+), 23 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/imprecise-floating-point-1.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr106181.c diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt index 61b58c575ff..437ea92e130 100644 --- a/gcc/analyzer/analyzer.opt +++ b/gcc/analyzer/analyzer.opt @@ -98,6 +98,10 @@ Wanalyzer-free-of-non-heap Common Var(warn_analyzer_free_of_non_heap) Init(1) Warning Warn about code paths in which a non-heap pointer is freed. +Wanalyzer-imprecise-fp-arithmetic +Common Var(warn_analyzer_imprecise_fp_arithmetic) Init(1) Warning +Warn about code paths in which floating-point arithmetic is used in locations where precise computation is needed. + Wanalyzer-jump-through-null Common Var(warn_analyzer_jump_through_null) Init(1) Warning Warn about code paths in which a NULL function pointer is called. diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index b5bc3efda32..ec29be259b5 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -3301,7 +3301,8 @@ public: 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"); + "allocated buffer size is not a multiple" + " of the pointee's size"); } label_text @@ -3396,21 +3397,20 @@ capacity_compatible_with_type (tree cst, tree pointee_size_tree) 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) + size_visitor (tree size_cst, const svalue *root_sval, constraint_manager *cm) + : m_size_cst (size_cst), m_root_sval (root_sval), m_cm (cm) { - sval->accept (this); + m_root_sval->accept (this); } bool get_result () { - return result_set.contains (m_sval); + return result_set.contains (m_root_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); + check_constant (sval->get_constant (), sval); } void visit_unknown_svalue (const unknown_svalue *sval ATTRIBUTE_UNUSED) @@ -3478,15 +3478,10 @@ public: 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); - } + if (tree cst = id.get_obj (*m_cm).get_any_constant ()) + check_constant (cst, sval); else - { - result_set.add (sval); - } + result_set.add (sval); } } @@ -3503,8 +3498,23 @@ public: } private: + void check_constant (tree cst, const svalue *sval) + { + switch (TREE_CODE (cst)) + { + default: + /* Assume all unhandled operands are compatible. */ + result_set.add (sval); + break; + case INTEGER_CST: + if (capacity_compatible_with_type (cst, m_size_cst)) + result_set.add (sval); + break; + } + } + tree m_size_cst; - const svalue *m_sval; + const svalue *m_root_sval; constraint_manager *m_cm; svalue_set result_set; /* Used as a mapping of svalue*->bool. */ }; @@ -3541,12 +3551,12 @@ struct_or_union_with_inheritance_p (tree struc) static bool is_any_cast_p (const gimple *stmt) { - if (const gassign *assign = dyn_cast(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)) + else if (const gcall *call = dyn_cast (stmt)) { tree lhs = gimple_call_lhs (call); return lhs != NULL_TREE && !pending_diagnostic::same_tree_p ( @@ -3606,10 +3616,11 @@ region_model::check_region_size (const region *lhs_reg, const svalue *rhs_sval, case svalue_kind::SK_CONSTANT: { const constant_svalue *cst_cap_sval - = as_a (capacity); + = as_a (capacity); tree cst_cap = cst_cap_sval->get_constant (); - if (!capacity_compatible_with_type (cst_cap, pointee_size_tree, - is_struct)) + if (TREE_CODE (cst_cap) == INTEGER_CST + && !capacity_compatible_with_type (cst_cap, pointee_size_tree, + is_struct)) ctxt->warn (new dubious_allocation_size (lhs_reg, rhs_reg, cst_cap)); } @@ -5055,6 +5066,125 @@ region_model::append_regions_cb (const region *base_reg, cb_data->out->safe_push (decl_reg); } + +/* Abstract class for diagnostics related to the use of + floating-point arithmetic where precision is needed. */ + +class imprecise_floating_point_arithmetic : public pending_diagnostic +{ +public: + int get_controlling_option () const final override + { + return OPT_Wanalyzer_imprecise_fp_arithmetic; + } +}; + +/* Concrete diagnostic to complain about uses of floating-point arithmetic + in the size argument of malloc etc. */ + +class float_as_size_arg : public imprecise_floating_point_arithmetic +{ +public: + float_as_size_arg (tree arg) : m_arg (arg) + {} + + const char *get_kind () const final override + { + return "float_as_size_arg_diagnostic"; + } + + bool subclass_equal_p (const pending_diagnostic &other) const + { + return same_tree_p (m_arg, ((const float_as_size_arg &) other).m_arg); + } + + bool emit (rich_location *rich_loc) final override + { + diagnostic_metadata m; + bool warned = warning_meta (rich_loc, m, get_controlling_option (), + "use of floating-point arithmetic here might" + " yield unexpected results"); + if (warned) + inform (rich_loc->get_loc (), "only use operands of an integer type" + " inside the size argument"); + return warned; + } + + label_text describe_final_event (const evdesc::final_event &ev) final + override + { + if (m_arg) + return ev.formatted_print ("operand %qE is of type %qT", + m_arg, TREE_TYPE (m_arg)); + return ev.formatted_print ("at least one operand of the size argument is" + " of a floating-point type"); + } + +private: + tree m_arg; +}; + +/* Visitor to find uses of floating-point variables/constants in an svalue. */ + +class contains_floating_point_visitor : public visitor +{ +public: + contains_floating_point_visitor (const svalue *root_sval) : m_result (NULL) + { + root_sval->accept (this); + } + + const svalue *get_svalue_to_report () + { + return m_result; + } + + void visit_constant_svalue (const constant_svalue *sval) final override + { + /* At the point the analyzer runs, constant integer operands in a floating + point expression are already implictly converted to floating-points. + Thus, we do prefer to report non-constants such that the diagnostic + always reports a floating-point operand. */ + tree type = sval->get_type (); + if (type && FLOAT_TYPE_P (type) && !m_result) + m_result = sval; + } + + void visit_conjured_svalue (const conjured_svalue *sval) final override + { + tree type = sval->get_type (); + if (type && FLOAT_TYPE_P (type)) + m_result = sval; + } + + void visit_initial_svalue (const initial_svalue *sval) final override + { + tree type = sval->get_type (); + if (type && FLOAT_TYPE_P (type)) + m_result = sval; + } + +private: + /* Non-null if at least one floating-point operand was found. */ + const svalue *m_result; +}; + +/* May complain about uses of floating-point operands in SIZE_IN_BYTES. */ + +void +region_model::check_dynamic_size_for_floats (const svalue *size_in_bytes, + region_model_context *ctxt) const +{ + gcc_assert (ctxt); + + contains_floating_point_visitor v (size_in_bytes); + if (const svalue *float_sval = v.get_svalue_to_report ()) + { + tree diag_arg = get_representative_tree (float_sval); + ctxt->warn (new float_as_size_arg (diag_arg)); + } +} + /* Return a new region describing a heap-allocated block of memory. Use CTXT to complain about tainted sizes. */ @@ -5092,8 +5222,11 @@ region_model::set_dynamic_extents (const region *reg, { assert_compat_types (size_in_bytes->get_type (), size_type_node); if (ctxt) - check_dynamic_size_for_taint (reg->get_memory_space (), size_in_bytes, - ctxt); + { + check_dynamic_size_for_taint (reg->get_memory_space (), size_in_bytes, + ctxt); + check_dynamic_size_for_floats (size_in_bytes, ctxt); + } m_dynamic_extents.put (reg, size_in_bytes); } diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h index cdf10872c0f..7ce832f6ce4 100644 --- a/gcc/analyzer/region-model.h +++ b/gcc/analyzer/region-model.h @@ -853,6 +853,8 @@ class region_model void check_dynamic_size_for_taint (enum memory_space mem_space, const svalue *size_in_bytes, region_model_context *ctxt) const; + void check_dynamic_size_for_floats (const svalue *size_in_bytes, + region_model_context *ctxt) const; void check_region_for_taint (const region *reg, enum access_direction dir, diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 1ac81ad0bb4..f65d351a5fc 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -453,6 +453,7 @@ Objective-C and Objective-C++ Dialects}. -Wno-analyzer-fd-use-without-check @gol -Wno-analyzer-file-leak @gol -Wno-analyzer-free-of-non-heap @gol +-Wno-analyzer-imprecise-fp-arithmetic @gol -Wno-analyzer-jump-through-null @gol -Wno-analyzer-malloc-leak @gol -Wno-analyzer-mismatching-deallocation @gol @@ -9758,6 +9759,7 @@ Enabling this option effectively enables the following warnings: -Wanalyzer-fd-use-without-check @gol -Wanalyzer-file-leak @gol -Wanalyzer-free-of-non-heap @gol +-Wanalyzer-imprecise-fp-arithmetic @gol -Wanalyzer-jump-through-null @gol -Wanalyzer-malloc-leak @gol -Wanalyzer-mismatching-deallocation @gol @@ -9946,6 +9948,18 @@ is called on a non-heap pointer (e.g. an on-stack buffer, or a global). See @uref{https://cwe.mitre.org/data/definitions/590.html, CWE-590: Free of Memory not on the Heap}. +@item -Wno-analyzer-imprecise-fp-arithmetic +@opindex Wanalyzer-imprecise-fp-arithmetic +@opindex Wno-analyzer-imprecise-fp-arithmetic +This warning requires @option{-fanalyzer}, which enables it; use +@option{-Wno-analyzer-imprecise-fp-arithmetic} +to disable it. + +This diagnostic warns for paths through the code in which floating-point +arithmetic is used in locations where precise computation is needed. This +diagnostic only warns on use of floating-point operands inside the +calculation of an allocation size at the moment. + @item -Wno-analyzer-jump-through-null @opindex Wanalyzer-jump-through-null @opindex Wno-analyzer-jump-through-null diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c b/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c index b5bed539250..003914ed96c 100644 --- a/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c @@ -115,3 +115,13 @@ void test_10 (int32_t n) char *ptr = malloc (7 * n); free (ptr); } + +void test_11 () +{ + /* 3.0 is folded to an int before the analyzer runs. */ + int32_t *ptr = malloc (3.0); /* { dg-line malloc11 } */ + free (ptr); + + /* { dg-warning "allocated buffer size is not a multiple of the pointee's size \\\[CWE-131\\\]" "warning" { target *-*-* } malloc11 } */ + /* { dg-message "'int32_t \\*' (\\\{aka '(long )?int \\*'\\\})? here; 'sizeof \\(int32_t (\\\{aka (long )?int\\\})?\\)' is '4'" "note" { target *-*-* } malloc11 } */ +} diff --git a/gcc/testsuite/gcc.dg/analyzer/imprecise-floating-point-1.c b/gcc/testsuite/gcc.dg/analyzer/imprecise-floating-point-1.c new file mode 100644 index 00000000000..d8a3f4884d6 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/imprecise-floating-point-1.c @@ -0,0 +1,74 @@ +#include + +/* Tests warn on use of floating-point operands inside the calculation + of an allocation size. + + The test cases here only test for warnings. The test cases inside + allocation-size-X.c should be plently enough to test for false positives. */ + +void test_1 (float f) +{ + int *ptr = malloc (sizeof (int) * f); /* { dg-line test_1 } */ + free (ptr); + + /* { dg-warning "use of floating-point arithmetic here might yield unexpected results" "warning" { target *-*-* } test_1 } */ + /* { dg-message "operand 'f' is of type 'float'" "note" { target *-*-* } test_1 } */ + /* { dg-message "only use operands of an integer type inside the size argument" "note" { target *-*-* } test_1 } */ +} + +void test_2 (int n) +{ + int *ptr = malloc (n * 3.1); /* { dg-line test_2 } */ + free (ptr); + + /* { dg-warning "use of floating-point arithmetic here might yield unexpected results" "warning" { target *-*-* } test_2 } */ + /* { dg-message "operand '\(\\d|e|f|\\.|\\+|\)+' is of type 'double'" "note" { target *-*-* } test_2 } */ + /* { dg-message "only use operands of an integer type inside the size argument" "note" { target *-*-* } test_2 } */ +} + +void *alloc_me (size_t size) +{ + return malloc (size); /* { dg-line test_3 } */ + + /* { dg-warning "use of floating-point arithmetic here might yield unexpected results" "warning" { target *-*-* } test_3 } */ + /* { dg-message "operand 'f' is of type 'float'" "note" { target *-*-* } test_3 } */ + /* { dg-message "only use operands of an integer type inside the size argument" "note" { target *-*-* } test_3 } */ +} + +void test_3 (float f) +{ + void *ptr = alloc_me (f); /* { dg-message "calling 'alloc_me' from 'test_3'" } */ + free (ptr); +} + +void test_4 (int n) +{ + int *ptr = calloc(1.7 * n, sizeof (int)); /* { dg-line test_4 } */ + free (ptr); + + /* { dg-warning "use of floating-point arithmetic here might yield unexpected results" "warning" { target *-*-* } test_4 } */ + /* { dg-message "operand '\(\\d|e|f|\\.|\\+|\)+' is of type 'double'" "note" { target *-*-* } test_4 } */ + /* { dg-message "only use operands of an integer type inside the size argument" "note" { target *-*-* } test_4 } */ +} + +int test_5 (float f) +{ + int *ptr = __builtin_alloca (sizeof (int) * f); /* { dg-line test_5 } */ + *ptr = 4; + return *ptr; + + /* { dg-warning "use of floating-point arithmetic here might yield unexpected results" "warning" { target *-*-* } test_5 } */ + /* { dg-message "operand 'f' is of type 'float'" "note" { target *-*-* } test_5 } */ + /* { dg-message "only use operands of an integer type inside the size argument" "note" { target *-*-* } test_5 } */ +} + +int test_6 (float f) +{ + int *ptr = __builtin_alloca (1.7f * f * 2.3f); /* { dg-line test_6 } */ + *ptr = 4; + return *ptr; + + /* { dg-warning "use of floating-point arithmetic here might yield unexpected results" "warning" { target *-*-* } test_6 } */ + /* { dg-message "operand 'f' is of type 'float'" "note" { target *-*-* } test_6 } */ + /* { dg-message "only use operands of an integer type inside the size argument" "note" { target *-*-* } test_6 } */ +} diff --git a/gcc/testsuite/gcc.dg/analyzer/pr106181.c b/gcc/testsuite/gcc.dg/analyzer/pr106181.c new file mode 100644 index 00000000000..6a78b78d352 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/pr106181.c @@ -0,0 +1,11 @@ +#include + +void * +foo (int x) +{ + return __builtin_calloc (x * 1.1, 1); /* { dg-line calloc } */ + + /* { dg-warning "use of floating-point arithmetic here might yield unexpected results" "warning" { target *-*-* } calloc } */ + /* { dg-message "operand '\(\\d|e|f|\\.|\\+|\)+' is of type 'double'" "note" { target *-*-* } calloc } */ + /* { dg-message "only use operands of an integer type inside the size argument" "note" { target *-*-* } calloc } */ +} -- 2.37.1