From patchwork Fri Jun 9 18:28:12 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tim Lange X-Patchwork-Id: 70840 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 9862238558A2 for ; Fri, 9 Jun 2023 18:29:57 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from www523.your-server.de (www523.your-server.de [159.69.224.22]) by sourceware.org (Postfix) with ESMTPS id 2A31B3856614 for ; Fri, 9 Jun 2023 18:29:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 2A31B3856614 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: 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:In-Reply-To:References; bh=3u+e+/ufLyOKg02Kp2hsDgm7i0eo+MPBEtijdtJEgOE=; b=rXQfG90nHP/B2/sXsv1EprroqJ PGfV/m+4CHLCA5a8yX2pe2u+tmZQ4B5Uh1Zm35xE17MKWxePnPuWJcZkqvIZp7Fucf7Rbp268OwU9 dRFWxao4OHlZpz/1bQqq6qs+nVDQVtPy+YFmXrAeUoNN0pHiwSx0p9pqhnkhohS8WJfJMrxmk0y+n jXWnvjzrQZc8cNBjrkDLDTy7+/3ELc86KwEvK6FAvx3A50IPdpIiRwU/ls3fA8uqYH5iXCkFaufMS NoevPMBOpQ1QRfKvxbX0T9NosyZX5CiKMQ420jiLOEUNdMk5C0Dhmt08FOKxIo/jfxd/xcas8xGU4 Ig/mkEow==; Received: from sslproxy03.your-server.de ([88.198.220.132]) by www523.your-server.de with esmtpsa (TLS1.3) tls TLS_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1q7grg-000Nz7-3Y; Fri, 09 Jun 2023 20:29:36 +0200 Received: from [2a02:908:1861:d6a0::7959] (helo=workstation..) by sslproxy03.your-server.de with esmtpsa (TLSv1.3:TLS_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1q7grf-000ROv-Qq; Fri, 09 Jun 2023 20:29:35 +0200 From: Tim Lange To: dmalcolm@redhat.com, gcc-patches@gcc.gnu.org Cc: Tim Lange Subject: [PATCH 1/2] analyzer: Fix allocation size false positive on conjured svalue [PR109577] Date: Fri, 9 Jun 2023 20:28:12 +0200 Message-Id: <20230609182813.72319-1-mail@tim-lange.me> X-Mailer: git-send-email 2.40.1 MIME-Version: 1.0 X-Authenticated-Sender: mail@tim-lange.me X-Virus-Scanned: Clear (ClamAV 0.103.8/26934/Fri Jun 9 09:25:21 2023) 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: , Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" Currently, the analyzer tries to prove that the allocation size is a multiple of the pointee's type size. This patch reverses the behavior to try to prove that the expression is not a multiple of the pointee's type size. With this change, each unhandled case should be gracefully considered as correct. This fixes the bug reported in PR 109577 by Paul Eggert. Regression-tested on Linux x86-64 with -m32 and -m64. 2023-06-09 Tim Lange PR analyzer/109577 gcc/analyzer/ChangeLog: * constraint-manager.cc (class sval_finder): Visitor to find childs in svalue trees. (constraint_manager::sval_constrained_p): Add new function to check whether a sval might be part of an constraint. * constraint-manager.h: Add sval_constrained_p function. * region-model.cc (class size_visitor): Reverse behavior to not emit a warning on not explicitly considered cases. (region_model::check_region_size): Adapt to size_visitor changes. gcc/testsuite/ChangeLog: * gcc.dg/analyzer/allocation-size-2.c: Change expected output and add new test case. * gcc.dg/analyzer/pr109577.c: New test. --- gcc/analyzer/constraint-manager.cc | 131 ++++++++++++++++++ gcc/analyzer/constraint-manager.h | 1 + gcc/analyzer/region-model.cc | 80 ++++------- .../gcc.dg/analyzer/allocation-size-2.c | 24 ++-- gcc/testsuite/gcc.dg/analyzer/pr109577.c | 16 +++ 5 files changed, 194 insertions(+), 58 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr109577.c diff --git a/gcc/analyzer/constraint-manager.cc b/gcc/analyzer/constraint-manager.cc index 2c9c435527e..24cd8960098 100644 --- a/gcc/analyzer/constraint-manager.cc +++ b/gcc/analyzer/constraint-manager.cc @@ -2218,6 +2218,137 @@ constraint_manager::get_equiv_class_by_svalue (const svalue *sval, return false; } +/* Tries to find a svalue inside another svalue. */ + +class sval_finder : public visitor +{ +public: + sval_finder (const svalue *query) : m_query (query) + { + } + + bool found_query_p () + { + return m_found; + } + + void visit_region_svalue (const region_svalue *sval) + { + m_found |= m_query == sval; + } + + void visit_constant_svalue (const constant_svalue *sval) + { + m_found |= m_query == sval; + } + + void visit_unknown_svalue (const unknown_svalue *sval) + { + m_found |= m_query == sval; + } + + void visit_poisoned_svalue (const poisoned_svalue *sval) + { + m_found |= m_query == sval; + } + + void visit_setjmp_svalue (const setjmp_svalue *sval) + { + m_found |= m_query == sval; + } + + void visit_initial_svalue (const initial_svalue *sval) + { + m_found |= m_query == sval; + } + + void visit_unaryop_svalue (const unaryop_svalue *sval) + { + m_found |= m_query == sval; + } + + void visit_binop_svalue (const binop_svalue *sval) + { + m_found |= m_query == sval; + } + + void visit_sub_svalue (const sub_svalue *sval) + { + m_found |= m_query == sval; + } + + void visit_repeated_svalue (const repeated_svalue *sval) + { + m_found |= m_query == sval; + } + + void visit_bits_within_svalue (const bits_within_svalue *sval) + { + m_found |= m_query == sval; + } + + void visit_unmergeable_svalue (const unmergeable_svalue *sval) + { + m_found |= m_query == sval; + } + + void visit_placeholder_svalue (const placeholder_svalue *sval) + { + m_found |= m_query == sval; + } + + void visit_widening_svalue (const widening_svalue *sval) + { + m_found |= m_query == sval; + } + + void visit_compound_svalue (const compound_svalue *sval) + { + m_found |= m_query == sval; + } + + void visit_conjured_svalue (const conjured_svalue *sval) + { + m_found |= m_query == sval; + } + + void visit_asm_output_svalue (const asm_output_svalue *sval) + { + m_found |= m_query == sval; + } + + void visit_const_fn_result_svalue (const const_fn_result_svalue *sval) + { + m_found |= m_query == sval; + } + +private: + const svalue *m_query; + bool m_found; +}; + +/* Returns true if SVAL is constrained. */ + +bool +constraint_manager::sval_constrained_p (const svalue *sval) const +{ + int i; + equiv_class *ec; + sval_finder finder (sval); + FOR_EACH_VEC_ELT (m_equiv_classes, i, ec) + { + int j; + const svalue *iv; + FOR_EACH_VEC_ELT (ec->m_vars, j, iv) + { + iv->accept (&finder); + if (finder.found_query_p ()) + return true; + } + } + return false; +} + /* Ensure that SVAL has an equivalence class within this constraint_manager; return the ID of the class. */ diff --git a/gcc/analyzer/constraint-manager.h b/gcc/analyzer/constraint-manager.h index 3afbc7f848e..72753e43c96 100644 --- a/gcc/analyzer/constraint-manager.h +++ b/gcc/analyzer/constraint-manager.h @@ -459,6 +459,7 @@ public: bool get_equiv_class_by_svalue (const svalue *sval, equiv_class_id *out) const; + bool sval_constrained_p (const svalue *sval) const; equiv_class_id get_or_add_equiv_class (const svalue *sval); tristate eval_condition (equiv_class_id lhs, enum tree_code op, diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index 3bb3df2f063..27b4c03db27 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -2972,7 +2972,7 @@ capacity_compatible_with_type (tree cst, tree pointee_size_tree) 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. */ + node that is not a multiple of SIZE_CST to the RESULT_SET. */ class size_visitor : public visitor { @@ -2983,7 +2983,7 @@ public: m_root_sval->accept (this); } - bool get_result () + bool is_dubious_capacity () { return result_set.contains (m_root_sval); } @@ -2993,22 +2993,10 @@ public: check_constant (sval->get_constant (), 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) final override { - const svalue *arg = sval->get_arg (); - if (result_set.contains (arg)) + if (CONVERT_EXPR_CODE_P (sval->get_op ()) + && result_set.contains (sval->get_arg ())) result_set.add (sval); } @@ -3017,28 +3005,24 @@ public: 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 + switch (sval->get_op ()) { - if (result_set.contains (arg0) && result_set.contains (arg1)) - result_set.add (sval); + case MULT_EXPR: + if (result_set.contains (arg0) && result_set.contains (arg1)) + result_set.add (sval); + break; + case PLUS_EXPR: + case MINUS_EXPR: + if (result_set.contains (arg0) || result_set.contains (arg1)) + result_set.add (sval); + break; + default: + break; } } - void visit_repeated_svalue (const repeated_svalue *sval) final override - { - 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); } @@ -3048,33 +3032,30 @@ public: const svalue *base = sval->get_base_svalue (); const svalue *iter = sval->get_iter_svalue (); - if (result_set.contains (base) && result_set.contains (iter)) + 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 + void visit_initial_svalue (const initial_svalue *sval) final override { - equiv_class_id id (-1); + equiv_class_id id = equiv_class_id::null (); if (m_cm->get_equiv_class_by_svalue (sval, &id)) { if (tree cst = id.get_obj (*m_cm).get_any_constant ()) check_constant (cst, sval); - else - result_set.add (sval); + } + else if (!m_cm->sval_constrained_p (sval)) + { + 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 + void visit_conjured_svalue (const conjured_svalue *sval) final override { - result_set.add (sval); + equiv_class_id id = equiv_class_id::null (); + if (m_cm->get_equiv_class_by_svalue (sval, &id)) + if (tree cst = id.get_obj (*m_cm).get_any_constant ()) + check_constant (cst, sval); } private: @@ -3084,10 +3065,9 @@ private: { default: /* Assume all unhandled operands are compatible. */ - result_set.add (sval); break; case INTEGER_CST: - if (capacity_compatible_with_type (cst, m_size_cst)) + if (!capacity_compatible_with_type (cst, m_size_cst)) result_set.add (sval); break; } @@ -3210,7 +3190,7 @@ region_model::check_region_size (const region *lhs_reg, const svalue *rhs_sval, if (!is_struct) { size_visitor v (pointee_size_tree, capacity, m_constraints); - if (!v.get_result ()) + if (v.is_dubious_capacity ()) { tree expr = get_representative_tree (capacity); ctxt->warn (make_unique (lhs_reg, diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c b/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c index 2cf64e97b41..90427229228 100644 --- a/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c @@ -76,13 +76,13 @@ void *create_buffer(int32_t n) return malloc(n); } -void test_7(int32_t n) +void test_7(int32_t n) { int32_t *buf = create_buffer(n * sizeof (int32_t)); free (buf); } -void test_8(int32_t n) +void test_8(int32_t 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 @@ -98,13 +98,11 @@ void test_9 (void) { int32_t n; scanf("%i", &n); - /* n is a conjured_svalue. */ - void *ptr = malloc (n); /* { dg-message "'n' bytes" "note" } */ - int32_t *iptr = (int32_t *)ptr; /* { dg-line assign9 } */ + /* n is a conjured_svalue without any constraint. We have to assume + that is a multiple of sizeof (int32_t *); see PR analyzer/110014. */ + void *ptr = malloc (n); + int32_t *iptr = (int32_t *)ptr; free (iptr); - - /* { dg-warning "allocated buffer size is not a multiple of the pointee's size \\\[CWE-131\\\]" "warning" { target *-*-* } assign9 } */ - /* { dg-message "'int32_t \\*' (\\\{aka '(long )?int \\*'\\\})? here; 'sizeof \\(int32_t (\\\{aka (long )?int\\\})?\\)' is '4'" "note" { target *-*-* } assign9 } */ } void test_11 (void) @@ -157,3 +155,13 @@ void test_13 (void) else free (ptr); } + +int *test_14 (size_t n) +{ + int *ptr = NULL; + /* n is an initial_svalue and guarded such that there is no equiv_class + for n itself but only for an binop_svalue containing n. */ + if (n % sizeof (int) == 0) + ptr = malloc (n); + return ptr; +} diff --git a/gcc/testsuite/gcc.dg/analyzer/pr109577.c b/gcc/testsuite/gcc.dg/analyzer/pr109577.c new file mode 100644 index 00000000000..a6af6f7019f --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/pr109577.c @@ -0,0 +1,16 @@ +void *malloc (unsigned long); + +double * +unsafe (unsigned long n) +{ + return malloc (n * sizeof (double)); +} + +double * +safer (unsigned long n) +{ + unsigned long nbytes; + if (__builtin_mul_overflow (n, sizeof (double), &nbytes)) + return 0; + return malloc (nbytes); +}