From patchwork Wed Aug 16 12:19:11 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: "Li, Pan2 via Gcc-patches" X-Patchwork-Id: 135765 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b82d:0:b0:3f2:4152:657d with SMTP id z13csp17696vqi; Wed, 16 Aug 2023 06:39:07 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFOr12b10GsGzJKIO+JxH33vPQRoI//y8VulCa6ElzuCwEDMYJECt0EDlljvdaRpVsgcqeo X-Received: by 2002:ac2:5e2e:0:b0:4fe:1681:9378 with SMTP id o14-20020ac25e2e000000b004fe16819378mr1285437lfg.66.1692193146965; Wed, 16 Aug 2023 06:39:06 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1692193146; cv=none; d=google.com; s=arc-20160816; b=P8H0ue3RA6Zj3Sdv6LgPB0OYm0KK3u17P9MlmBagV4FHUFdke04PfXDyF4e67npo+T tFMJRAmGjxV5IUcAj22/++qYt8RJqHOp4o6yAz/GDHAd8I+3QS0DWxlfZpBH3WbRQ5bq 6mHuGjDXyw4pZ1OcPJV9w2L9ZlKZGj0gHrQII9xXgRUw6isKapm4H961Jj59EBMYN1VH 12dr99NDNsB/2qyiPR79KXX6UrfCCqsXmT9malXo7xHztP8hDZhyV26t15wYVQXV43NX kuI9rdaqE0y5khevUhdXYgTQy7kC+xEBgi+L1TCSqSlOv1fH/IApMXCyw47odLfTZfAU xWTA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:reply-to:from:list-subscribe:list-help:list-post :list-archive:list-unsubscribe:list-id:precedence :content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:dmarc-filter:delivered-to :dkim-signature:dkim-filter; bh=5DEALh6ITfgbJmMa3V3phm4Vv/P5IaWrHxexRVOCHD8=; fh=se9AvlvO6nAQ+5ltUc700CnLSEFsJZH/SLcWO+v9loY=; b=al1YgalFsX7Pk53YjCfNkoq9lTcQueU86Y5DfSS2nF6NsXnRZlcgDwl8p5x2da8ehr fm2vKUBrUJHSj84EM04N73RGsvG4dJvZ7dDSidqX6g47sxiilBN+WNVn0SLrBWMmO2Pg 8YUiO+ryu5BYFxxSdPMND0wUSDXJCsNjEA0rlI+nEVkV6Z2N2U/LqaoQn+0LLEse5JC1 1Bdg/LAoerIe+0ARaZ7qxXXKNhfaKebK2HkfyhT3CluKZoX5hmE31yiv6JekweHwulMr B5bsl0mOdJ1FDPh7+nWBNK1nraH+iUzTkwgwpgw0fhlIvIyijvmG+/8MZnrF6RZmCb/Q RfJw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gcc.gnu.org header.s=default header.b=kxAs0pJ6; spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=gnu.org Received: from server2.sourceware.org (server2.sourceware.org. [2620:52:3:1:0:246e:9693:128c]) by mx.google.com with ESMTPS id bo7-20020a0564020b2700b00525665e8c36si5242577edb.430.2023.08.16.06.39.06 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 16 Aug 2023 06:39:06 -0700 (PDT) Received-SPF: pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) client-ip=2620:52:3:1:0:246e:9693:128c; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org header.s=default header.b=kxAs0pJ6; spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=gnu.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id B256D385770D for ; Wed, 16 Aug 2023 13:39:05 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org B256D385770D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1692193145; bh=5DEALh6ITfgbJmMa3V3phm4Vv/P5IaWrHxexRVOCHD8=; h=To:Cc:Subject:Date:In-Reply-To:References:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=kxAs0pJ6p0D7mWmJqTvxER/Diz1ytZMCVYDyovZ4XlETxsmZKxFN6tpH3fv/mvmrh KXyiOaYVJpgWZCU83yJgYcXTbHPTB+Yt0V0dVKCOoJNHHik/qEp0zp19PNlcwBXUkM 91H4+ne2JkmWnURLguh7E5A37z0XR7cK1ga8PA1M= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-wm1-x329.google.com (mail-wm1-x329.google.com [IPv6:2a00:1450:4864:20::329]) by sourceware.org (Postfix) with ESMTPS id 2C7A13858C52; Wed, 16 Aug 2023 13:38:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 2C7A13858C52 Received: by mail-wm1-x329.google.com with SMTP id 5b1f17b1804b1-3fe426b8583so66167925e9.2; Wed, 16 Aug 2023 06:38:13 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1692193092; x=1692797892; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=5DEALh6ITfgbJmMa3V3phm4Vv/P5IaWrHxexRVOCHD8=; b=Nakte31tkcl57Gg8n9P0Nhs/rzEOQ99ROlNxwniwQTlXAKLosHbU/g4ajNIRBQYY2j K1cfMbtHCnXHLI+JvpWQSXYsu9RFW27QprM73IY6UdNWciZ8nBrSiujRFAY15mYNabyY e/TxkW+aQ9V48DWkFanPybm69Xh4MuBIbzSEptQX3jIkOL16jVZpBTqh+Apo69T7aECP nYtAPUeDlXKZV1s312mmEdRPhKi54g6cbIoNLwkUNKDp7t1L5AVhtEpA9vQRvNued+hC 6qwwkiB/QHhSHfMEriXhzWF88FXPssM93eAaQ0rLkj4XUdHRNIw465okcnmG2BACuWGj BggA== X-Gm-Message-State: AOJu0YwBMcK7Fn4zeoQRPSDtud4skGPLBP/qo76EoI4+oGixMdKDYzWh /lZkUItZaNJpPuRcDWRfsM5gHnU7XA== X-Received: by 2002:a5d:4b91:0:b0:319:7487:9144 with SMTP id b17-20020a5d4b91000000b0031974879144mr1375791wrt.69.1692193091315; Wed, 16 Aug 2023 06:38:11 -0700 (PDT) Received: from localhost ([2a01:e0a:2ec:f0d0:8bba:b79c:b727:8a64]) by smtp.gmail.com with UTF8SMTPSA id t21-20020a1c7715000000b003fe0bb31a6asm21085811wmi.43.2023.08.16.06.38.10 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 16 Aug 2023 06:38:10 -0700 (PDT) X-Google-Original-From: vultkayn@gcc.gnu.org To: gcc-patches@gcc.gnu.org Cc: dmalcolm@redhat.com, benjamin priour Subject: [WIP RFC v2] analyzer: Add support of placement new and improved operator new [PR105948] Date: Wed, 16 Aug 2023 14:19:11 +0200 Message-Id: <20230816121909.53047-1-vultkayn@gcc.gnu.org> X-Mailer: git-send-email 2.34.1 In-Reply-To: References: MIME-Version: 1.0 X-Spam-Status: No, score=-10.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP 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-Patchwork-Original-From: Benjamin Priour via Gcc-patches From: "Li, Pan2 via Gcc-patches" Reply-To: priour.be@gmail.com Errors-To: gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org Sender: "Gcc-patches" X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1770508408992758233 X-GMAIL-MSGID: 1774393121481150035 From: benjamin priour Hi, (s/we/the analyzer/) I've been continuing my patch of supporting operator new variants in the analyzer, and have added a few more test cases. > > If "y" is null then the allocation failed and dereferencing "y" will > > cause > > a segfault, not a "use-of-uninitialized-value". > > Thus we should stick to 'dereference of NULL 'y'" only. > > If "y" is non-null then the allocation succeeded and "*y" is > > initialized > > since we are calling a default initialization with the empty > > parenthesis. > > I *think* it's possible to have the region_model have y pointing to a > heap_allocated_region of sizeof(int) size that's been initialized, but > still have the malloc state machine part of the program_state say that > the pointer is maybe-null. By maybe-null are you implying a new sm-malloc state ? I am not sure to follow on that front. > > > This led me to consider having "null-dereference" supersedes > > "use-of-uninitialized-value", but > > new PR 110830 made me reexamine it. > > > > I believe fixing PR 110830 is thus required before submitting this > > patch, > > or we would have some extra irrelevant warnings. > > How bad would the problem be? PR 110830 looks a little involved, so is > there a way to get the current patch in without dragging that extra > complexity in? Having "null-dereference" supersedes "use-of-uninitialized-value" would cause false negative upon conditional return statement (similarly as demonstrated in PR 110830). Since PR 110830 is off for the moment, I have tried solving this differently. I have considered using known NULL constraints on heap_allocated_region as "initialized_value". You can see below in the diff of region_model::get_store_value two versions of this approach. The version commented out proved to solve the issue of the spurious "use-of-unitialized-value" tagging along calls to "new(std::nothrow) ()". However, this version also shortcircuits the diagnostics of the "null-dereference" warning. Given /* { dg-additional-options "-O0 -fno-exceptions -fno-analyzer-suppress-followups" } */ #include struct A { int x; int y; }; void test_nonthrowing () { A* y = new(std::nothrow) A(); int z = y->x + 2; /* { dg-warning "dereference of NULL 'y'" } */ /* { dg-bogus "use of uninitialized value '\\*y'" "" { xfail *-*-* } .-1 } */ delete y; } The analyzer sees gimple : _7 = operator new (8, ¬hrow); if (_7 != 0B) goto ; [INV] else goto ; [INV] : MEM[(struct A *)_7].x = 0; MEM[(struct A *)_7].y = 0; iftmp.0_11 = _7; goto ; [INV] : iftmp.0_8 = _7; : # iftmp.0_2 = PHI y_12 = iftmp.0_2; _1 = y_12->x; z_13 = _1 + 2; y.1_14 = y_12; if (y.1_14 != 0B) goto ; [INV] else goto ; [INV] : *y.1_14 ={v} {CLOBBER}; operator delete (y.1_14, 8); The injurious path, causing the "use-of-uninit" warning is as follows: : _7 = operator new (8, ¬hrow); if (_7 != 0B) ... else <- Takes false branch goto ; [INV] ... : iftmp.0_8 = _7; <- MEM[(struct A*) _7] is left uninit in this bb : # iftmp.0_2 = PHI <- iftmp.0_2 = iftmp.0_8(4) y_12 = iftmp.0_2; _1 = y_12->x; // deref of null y_12, use of uninit y_12->x z_13 = _1 + 2; // check_for_poison sets _1 to unknown_svalue y.1_14 = y_12; if (y.1_14 != 0B) goto ; [INV] else goto ; [INV] Then using the "commented-out" fix, iftmp.0_8 which had an uninit value is forcibly set to constant_svalue(0), since the analyzer detects a NULL constraint on &heap_allocated_region. Unfortunately, this loses all clusters binding on _7 and the followings variables, such as when we arrive at "_1 = y_12->x", we emit a "null_deref" not because the heap_allocated_region is in a null state, but because we are dereferencing a constant "0". Thus the analysis path no longer tracks down the creation of this region, and the genese event is "iftmp.0_8 = _7". As you guess, this loss of information fails a lot of regression tests, although it achieves the goal of removing the "use-of-uninit" warning. The second attempt (see get_store_value diff below, the non-commented out block), actually does nothing, which as I understood through debugging was to be expected. We are doing the same "constraints" check as the former version, but only as a last resort before resorting to creating an initial or unknown svalue. And instead of creating a constant_svalue(0) as before, now a NULL constraint only prevents the creation of a poisoned_svalue(uninit) by setting "check_poisoned" to false. However in + if (reg->get_kind () == RK_FIELD || reg->get_kind () == RK_ELEMENT) + { + const region *base_reg = reg->get_base_region (); + const svalue *base_sval + = m_store.get_any_binding (m_mgr->get_store_manager (), base_reg); + if (base_sval) + { + ... + } base_sval is always NULL (which makes sense since otherwise we wouldn't need to create an initial_svalue). The spirit behind this second attempt is to not lose any sort of information, therefore to not forcibly create constant_svalues. Thus, instead of assigning a null_svalue at "iftmp.0_8 = _7", rather wait for the latest "_1 = y_12->x;" and prevent the creation of a "poisoned_svalue (uninit)". The failings of the second attempt is to find the NULL constraint of the heap_allocated_region (which still exists thanks to path splitting) given the COMPONENT_REF y_12->x; I am unsure as to how to do that, thus this RFC. Thanks, Benjamin. Patch below. --- Fixed spurious possibly-NULL warning always tagging along throwing operator new despite it never returning NULL. Now, operator new is correctly recognized as possibly returning NULL if and only if it is non-throwing or exceptions have been disabled. Different standard signatures of operator new are now properly recognized. Added support of placement new, so that is now properly recognized, and a 'heap_allocated' region is no longer created for it. Placement new size is also checked and a 'Wanalyzer-allocation-size' is emitted when relevant, as well as always a 'Wanalyzer-out-of-bounds'. 'operator new' nothrow variants are detected by checking the types of the parameters. Indeed, in a call to new (std::nothrow) (), although the chosen overload has signature 'operator new (void*, std::nothrow_t&)', where parameter 1 is a reference. In a placement new, parameter 1 will always be a [void] pointer. Prior to this patch, some buffers allocated with 'new', then deleted and thereafter used would result in a Wanalyzer-use-after-free warning. However, the wording was "use after 'free' of" instead of the expected "use after 'delete' of". This patch fixed this by introducing a new kind of poisoned value. Signed-off-by: benjamin priour gcc/analyzer/ChangeLog: * analyzer.h (is_placement_new_p): New declaration. * call-details.cc (call_details::maybe_get_arg_region): New function. Returns the region of the argument at given index if possible. * call-details.h: Declaration of the above function. * kf-lang-cp.cc (is_placement_new_p): Returns true if the gcall is recognized as a placement new. (kf_operator_delete::impl_call_post): Unbinding a region and its descendent now poisons with POISON_KIND_DELETED. (register_known_functions_lang_cp): Known function "operator delete" is now registered only once independently of its number of parameters. * region-model.cc (region_model::get_rvalue_1): (region_model::get_store_value): Attempt to use a region NULL constraint to emit a null_ptr_svalue instead of an uninit poisoned_svalue. (region_model::eval_condition): Now recursively calls itself if any of the operand is wrapped in a cast. * sm-malloc.cc (malloc_state_machine::on_stmt): Add placement new recognition. * svalue.cc (poison_kind_to_str): Wording for the new PK. * svalue.h (enum poison_kind): Add value POISON_KIND_DELETED. gcc/testsuite/ChangeLog: * g++.dg/analyzer/out-of-bounds-placement-new.C: Added a directive. * g++.dg/analyzer/placement-new.C: Added tests. * g++.dg/analyzer/new-2.C: New test. * g++.dg/analyzer/noexcept-new.C: New test. * g++.dg/analyzer/placement-new-size.C: New test. --- gcc/analyzer/analyzer.h | 1 + gcc/analyzer/call-details.cc | 11 ++ gcc/analyzer/call-details.h | 1 + gcc/analyzer/kf-lang-cp.cc | 111 +++++++++++++++--- gcc/analyzer/region-model.cc | 68 ++++++++++- gcc/analyzer/sm-malloc.cc | 17 ++- gcc/analyzer/svalue.cc | 2 + gcc/analyzer/svalue.h | 3 + gcc/testsuite/g++.dg/analyzer/new-2.C | 70 +++++++++++ gcc/testsuite/g++.dg/analyzer/noexcept-new.C | 48 ++++++++ .../analyzer/out-of-bounds-placement-new.C | 2 +- .../g++.dg/analyzer/placement-new-size.C | 27 +++++ gcc/testsuite/g++.dg/analyzer/placement-new.C | 92 ++++++++++++++- 13 files changed, 427 insertions(+), 26 deletions(-) create mode 100644 gcc/testsuite/g++.dg/analyzer/new-2.C create mode 100644 gcc/testsuite/g++.dg/analyzer/noexcept-new.C create mode 100644 gcc/testsuite/g++.dg/analyzer/placement-new-size.C diff --git a/gcc/analyzer/analyzer.h b/gcc/analyzer/analyzer.h index 93a28b4b5cf..f44e8296f5e 100644 --- a/gcc/analyzer/analyzer.h +++ b/gcc/analyzer/analyzer.h @@ -401,6 +401,7 @@ extern bool is_std_named_call_p (const_tree fndecl, const char *funcname, const gcall *call, unsigned int num_args); extern bool is_setjmp_call_p (const gcall *call); extern bool is_longjmp_call_p (const gcall *call); +extern bool is_placement_new_p (const gcall *call); extern const char *get_user_facing_name (const gcall *call); diff --git a/gcc/analyzer/call-details.cc b/gcc/analyzer/call-details.cc index fa86f55177a..0ac44f3726c 100644 --- a/gcc/analyzer/call-details.cc +++ b/gcc/analyzer/call-details.cc @@ -283,6 +283,17 @@ call_details::get_arg_svalue (unsigned idx) const return m_model->get_rvalue (arg, m_ctxt); } +/* If argument IDX's svalue at the callsite is a region_svalue, + return the region it points to. + Otherwise return NULL. */ + +const region * +call_details::maybe_get_arg_region (unsigned idx) const +{ + const svalue *sval = get_arg_svalue (idx); + return sval->maybe_get_region (); +} + /* Attempt to get the string literal for argument IDX, or return NULL otherwise. For use when implementing "__analyzer_*" functions that take diff --git a/gcc/analyzer/call-details.h b/gcc/analyzer/call-details.h index 0622cab7856..919338f88b9 100644 --- a/gcc/analyzer/call-details.h +++ b/gcc/analyzer/call-details.h @@ -60,6 +60,7 @@ public: tree get_arg_tree (unsigned idx) const; tree get_arg_type (unsigned idx) const; const svalue *get_arg_svalue (unsigned idx) const; + const region *maybe_get_arg_region (unsigned idx) const; const char *get_arg_string_literal (unsigned idx) const; tree get_fndecl_for_call () const; diff --git a/gcc/analyzer/kf-lang-cp.cc b/gcc/analyzer/kf-lang-cp.cc index 393b4f25e79..cfb0584979b 100644 --- a/gcc/analyzer/kf-lang-cp.cc +++ b/gcc/analyzer/kf-lang-cp.cc @@ -35,6 +35,38 @@ along with GCC; see the file COPYING3. If not see #if ENABLE_ANALYZER +/* Return true if CALL is a non-allocating operator new or operator new [] + that contains no user-defined args, i.e. having any signature of: + + - void* operator new (std::size_t count, void* ptr); + - void* operator new[] (std::size_t count, void* ptr); + + See https://en.cppreference.com/w/cpp/memory/new/operator_new. */ + +bool is_placement_new_p (const gcall *call) +{ + gcc_assert (call); + tree fndecl = gimple_call_fndecl (call); + + if (!fndecl || TREE_CODE (TREE_TYPE (fndecl)) == METHOD_TYPE) + /* Give up on overloaded operator new. */ + return false; + + if (!is_named_call_p (fndecl, "operator new", call, 2) + && !is_named_call_p (fndecl, "operator new []", call, 2)) + return false; + + /* We must distinguish between an allocating non-throwing new + and a non-allocating new. + + The former might have one of the following signatures : + void* operator new (std::size_t count, const std::nothrow_t& tag); + void* operator new[] (std::size_t count, const std::nothrow_t& tag); + Whereas a placement new would take a pointer. */ + tree arg1_type = TREE_CHAIN (TYPE_ARG_TYPES (TREE_TYPE (fndecl))); + return TREE_CODE (TREE_VALUE (arg1_type)) == POINTER_TYPE; +} + namespace ana { /* Implementations of specific functions. */ @@ -46,7 +78,11 @@ class kf_operator_new : public known_function public: bool matches_call_types_p (const call_details &cd) const final override { - return cd.num_args () == 1; + return (cd.num_args () == 1 + && cd.arg_is_size_p (0)) + || (cd.num_args () == 2 + && cd.arg_is_size_p (0) + && POINTER_TYPE_P (cd.get_arg_type (1))); } void impl_call_pre (const call_details &cd) const final override @@ -54,28 +90,74 @@ public: region_model *model = cd.get_model (); region_model_manager *mgr = cd.get_manager (); const svalue *size_sval = cd.get_arg_svalue (0); - const region *new_reg - = model->get_or_create_region_for_heap_alloc (size_sval, cd.get_ctxt ()); - if (cd.get_lhs_type ()) + region_model_context *ctxt = cd.get_ctxt (); + const gcall *call = cd.get_call_stmt (); + + /* If the call was actually a placement new, check that accessing + the buffer lhs is placed into does not result in out-of-bounds. */ + if (is_placement_new_p (call)) { + const region *ptr_reg = cd.maybe_get_arg_region (1); + if (ptr_reg && cd.get_lhs_type ()) + { + const region *base_reg = ptr_reg->get_base_region (); + const svalue *num_bytes_sval = cd.get_arg_svalue (0); + const region *sized_new_reg = mgr->get_sized_region (base_reg, + cd.get_lhs_type (), + num_bytes_sval); + model->check_region_for_write (sized_new_reg, + nullptr, + ctxt); const svalue *ptr_sval - = mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg); + = mgr->get_ptr_svalue (cd.get_lhs_type (), sized_new_reg); cd.maybe_set_lhs (ptr_sval); + } + } + /* If the call is an allocating new, then create a heap allocated + region. */ + else + { + const region *new_reg + = model->get_or_create_region_for_heap_alloc (size_sval, ctxt); + if (cd.get_lhs_type ()) + { + const svalue *ptr_sval + = mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg); + cd.maybe_set_lhs (ptr_sval); + } + } + } + + void impl_call_post (const call_details &cd) const final override + { + region_model *model = cd.get_model (); + region_model_manager *mgr = cd.get_manager (); + tree callee_fndecl = cd.get_fndecl_for_call (); + region_model_context *ctxt = cd.get_ctxt (); + + /* If the call is guaranteed to return nonnull + then add a nonnull constraint to the allocated region. */ + if (!TREE_NOTHROW (callee_fndecl) && flag_exceptions) + { + const svalue *null_sval + = mgr->get_or_create_null_ptr (cd.get_lhs_type ()); + const svalue *result + = model->get_store_value (cd.get_lhs_region (), ctxt); + model->add_constraint (result, NE_EXPR, null_sval, ctxt); } } }; -/* Handler for "operator delete", both the sized and unsized variants - (2 arguments and 1 argument respectively), and for "operator delete []" */ +/* Handler for "operator delete" and for "operator delete []", + both the sized and unsized variants + (2 arguments and 1 argument respectively). */ class kf_operator_delete : public known_function { public: - kf_operator_delete (unsigned num_args) : m_num_args (num_args) {} - bool matches_call_types_p (const call_details &cd) const final override { - return cd.num_args () == m_num_args; + return cd.num_args () == 1 or cd.num_args () == 2; } void impl_call_post (const call_details &cd) const final override @@ -86,12 +168,10 @@ public: { /* If the ptr points to an underlying heap region, delete it, poisoning pointers. */ - model->unbind_region_and_descendents (freed_reg, POISON_KIND_FREED); + model->unbind_region_and_descendents (freed_reg, POISON_KIND_DELETED); } } -private: - unsigned m_num_args; }; /* Populate KFM with instances of known functions relating to C++. */ @@ -101,9 +181,8 @@ register_known_functions_lang_cp (known_function_manager &kfm) { kfm.add ("operator new", make_unique ()); kfm.add ("operator new []", make_unique ()); - kfm.add ("operator delete", make_unique (1)); - kfm.add ("operator delete", make_unique (2)); - kfm.add ("operator delete []", make_unique (1)); + kfm.add ("operator delete", make_unique ()); + kfm.add ("operator delete []", make_unique ()); } } // namespace ana diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index 494a9cdf149..a358794356e 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -499,6 +499,7 @@ public: case POISON_KIND_UNINIT: return OPT_Wanalyzer_use_of_uninitialized_value; case POISON_KIND_FREED: + case POISON_KIND_DELETED: return OPT_Wanalyzer_use_after_free; case POISON_KIND_POPPED_STACK: return OPT_Wanalyzer_use_of_pointer_in_stale_stack_frame; @@ -531,6 +532,15 @@ public: m_expr); } break; + case POISON_KIND_DELETED: + { + diagnostic_metadata m; + m.add_cwe (416); /* "CWE-416: Use After Free". */ + return warning_meta (rich_loc, m, get_controlling_option (), + "use after % of %qE", + m_expr); + } + break; case POISON_KIND_POPPED_STACK: { /* TODO: which CWE? */ @@ -555,6 +565,9 @@ public: case POISON_KIND_FREED: return ev.formatted_print ("use after % of %qE here", m_expr); + case POISON_KIND_DELETED: + return ev.formatted_print ("use after % of %qE here", + m_expr); case POISON_KIND_POPPED_STACK: return ev.formatted_print ("dereferencing pointer %qE to within stale stack frame", @@ -2210,7 +2223,7 @@ region_model::get_rvalue_1 (path_var pv, region_model_context *ctxt) const case MEM_REF: { const region *ref_reg = get_lvalue (pv, ctxt); - return get_store_value (ref_reg, ctxt); + return get_store_value (ref_reg, ctxt); // INFO get store value of field_region(cast_region(heap_allocated_region(19), ‘struct A’), ‘int’, ‘int A::x’) } case OBJ_TYPE_REF: { @@ -2321,6 +2334,16 @@ region_model::get_store_value (const region *reg, = m_store.get_any_binding (m_mgr->get_store_manager (), reg); if (sval) { +/* if (m_constraints->sval_constrained_p (sval)) + { + equiv_class_id sval_ecid = equiv_class_id::null (); + if (m_constraints->get_equiv_class_by_svalue (sval, &sval_ecid)) + { + const equiv_class& sval_ec = sval_ecid.get_obj(*m_constraints); + if (sval_ec.get_any_constant () != NULL_TREE) + sval = sval_ec.m_cst_sval; + } + } */ if (reg->get_type ()) sval = m_mgr->get_or_create_cast (reg->get_type (), sval); return sval; @@ -2363,6 +2386,28 @@ region_model::get_store_value (const region *reg, == RK_GLOBALS) return get_initial_value_for_global (reg); + /* If there exists a NULL constraint on the parent region, + then do not create a poisoned_uninit value. */ + if (reg->get_kind () == RK_FIELD || reg->get_kind () == RK_ELEMENT) + { + const region *base_reg = reg->get_base_region (); + const svalue *base_sval + = m_store.get_any_binding (m_mgr->get_store_manager (), base_reg); + if (base_sval) + { + if (m_constraints->sval_constrained_p (base_sval)) + { + equiv_class_id sval_ecid = equiv_class_id::null (); + if (m_constraints->get_equiv_class_by_svalue (base_sval, &sval_ecid)) + { + const equiv_class& sval_ec = sval_ecid.get_obj(*m_constraints); + if (const_tree cst = sval_ec.get_any_constant ()) + if (::zerop (cst)) + check_poisoned = false; + } + } + } + } return m_mgr->get_or_create_initial_value (reg, check_poisoned); } @@ -3582,6 +3627,27 @@ region_model::eval_condition (const svalue *lhs, } } + /* Attempt to unwrap cast if there is one, and the types match. */ + tree lhs_type = lhs->get_type (); + tree rhs_type = rhs->get_type (); + if (lhs_type && rhs_type) + { + const unaryop_svalue *lhs_un_op = dyn_cast (lhs); + const unaryop_svalue *rhs_un_op = dyn_cast (rhs); + if (lhs_un_op && CONVERT_EXPR_CODE_P (lhs_un_op->get_op ()) + && rhs_un_op && CONVERT_EXPR_CODE_P (rhs_un_op->get_op ()) + && lhs_type == rhs_type) + return eval_condition (lhs_un_op->get_arg (), op, rhs_un_op->get_arg ()); + + else if (lhs_un_op && CONVERT_EXPR_CODE_P (lhs_un_op->get_op ()) + && lhs_type == rhs_type) + return eval_condition (lhs_un_op->get_arg (), op, rhs); + + else if (rhs_un_op && CONVERT_EXPR_CODE_P (rhs_un_op->get_op ()) + && lhs_type == rhs_type) + return eval_condition (lhs, op, rhs_un_op->get_arg ()); + } + /* Otherwise, try constraints. Cast to const to ensure we don't change the constraint_manager as we do this (e.g. by creating equivalence classes). */ diff --git a/gcc/analyzer/sm-malloc.cc b/gcc/analyzer/sm-malloc.cc index ec763254b29..567237cf1ae 100644 --- a/gcc/analyzer/sm-malloc.cc +++ b/gcc/analyzer/sm-malloc.cc @@ -759,7 +759,7 @@ public: override { if (change.m_old_state == m_sm.get_start_state () - && unchecked_p (change.m_new_state)) + && (unchecked_p (change.m_new_state) || nonnull_p (change.m_new_state))) // TODO: verify that it's the allocation stmt, not a copy return label_text::borrow ("allocated here"); if (unchecked_p (change.m_old_state) @@ -1915,11 +1915,16 @@ malloc_state_machine::on_stmt (sm_context *sm_ctxt, return true; } - if (is_named_call_p (callee_fndecl, "operator new", call, 1)) - on_allocator_call (sm_ctxt, call, &m_scalar_delete); - else if (is_named_call_p (callee_fndecl, "operator new []", call, 1)) - on_allocator_call (sm_ctxt, call, &m_vector_delete); - else if (is_named_call_p (callee_fndecl, "operator delete", call, 1) + if (!is_placement_new_p (call)) + { + bool returns_nonnull = !TREE_NOTHROW (callee_fndecl) && flag_exceptions; + if (is_named_call_p (callee_fndecl, "operator new")) + on_allocator_call (sm_ctxt, call, &m_scalar_delete, returns_nonnull); + else if (is_named_call_p (callee_fndecl, "operator new []")) + on_allocator_call (sm_ctxt, call, &m_vector_delete, returns_nonnull); + } + + if (is_named_call_p (callee_fndecl, "operator delete", call, 1) || is_named_call_p (callee_fndecl, "operator delete", call, 2)) { on_deallocator_call (sm_ctxt, node, call, diff --git a/gcc/analyzer/svalue.cc b/gcc/analyzer/svalue.cc index 064627f3dcc..35eb8307b20 100644 --- a/gcc/analyzer/svalue.cc +++ b/gcc/analyzer/svalue.cc @@ -970,6 +970,8 @@ poison_kind_to_str (enum poison_kind kind) return "uninit"; case POISON_KIND_FREED: return "freed"; + case POISON_KIND_DELETED: + return "deleted"; case POISON_KIND_POPPED_STACK: return "popped stack"; } diff --git a/gcc/analyzer/svalue.h b/gcc/analyzer/svalue.h index 5492b1e0b7c..263a0d7af6f 100644 --- a/gcc/analyzer/svalue.h +++ b/gcc/analyzer/svalue.h @@ -350,6 +350,9 @@ enum poison_kind /* For use to describe freed memory. */ POISON_KIND_FREED, + /* For use to describe deleted memory. */ + POISON_KIND_DELETED, + /* For use on pointers to regions within popped stack frames. */ POISON_KIND_POPPED_STACK }; diff --git a/gcc/testsuite/g++.dg/analyzer/new-2.C b/gcc/testsuite/g++.dg/analyzer/new-2.C new file mode 100644 index 00000000000..9e0315b3f44 --- /dev/null +++ b/gcc/testsuite/g++.dg/analyzer/new-2.C @@ -0,0 +1,70 @@ +// { dg-additional-options "-O0 -fno-analyzer-suppress-followups -fexceptions" } +#include + +struct A +{ + int x; + int y; +}; + +void test_spurious_null_warning_throwing () +{ + int *x = new int; /* { dg-bogus "dereference of possibly-NULL" } */ + int *y = new int (); /* { dg-bogus "dereference of possibly-NULL" "non-throwing" } */ + int *arr = new int[3]; /* { dg-bogus "dereference of possibly-NULL" } */ + A *a = new A (); /* { dg-bogus "dereference of possibly-NULL" "throwing new cannot be null" } */ + + int z = *y + 2; + z = *x + 4; /* { dg-bogus "dereference of possibly-NULL 'x'" } */ + /* { dg-warning "use of uninitialized value '\\*x'" "" { target *-*-* } .-1 } */ + z = arr[0] + 4; /* { dg-bogus "dereference of possibly-NULL" } */ + /* { dg-warning "use of uninitialized value '\\*arr'" "" { target *-*-* } .-1 } */ + + delete a; + delete y; + delete x; + delete[] arr; +} + +void test_default_initialization () +{ + int *y = ::new int; + int *x = ::new int (); /* { dg-bogus "dereference of possibly-NULL 'operator new" } */ + + int b = *x + 3; /* { dg-bogus "dereference of possibly-NULL" } */ + /* { dg-bogus "use of uninitialized ‘*x’" "" { target *-*-* } .-1 } */ + int a = *y + 2; /* { dg-bogus "dereference of possibly-NULL 'y'" } */ + /* { dg-warning "use of uninitialized value '\\*y'" "no default init" { target *-*-* } .-1 } */ + + delete x; + delete y; +} + +/* From clang core.uninitialized.NewArraySize +new[] should not be called with an undefined size argument */ + +void test_garbage_new_array () +{ + int n; + int *arr = ::new int[n]; /* { dg-warning "use of uninitialized value 'n'" } */ + arr[0] = 7; + ::delete[] arr; /* no warnings emitted here either */ +} + +void test_nonthrowing () +{ + int* x = new(std::nothrow) int; + int* y = new(std::nothrow) int(); + int* arr = new(std::nothrow) int[10]; + + int z = *y + 2; /* { dg-warning "dereference of NULL 'y'" } */ + /* { dg-bogus "use of uninitialized value '\\*y'" "" { xfail *-*-* } .-1 } */ + z = *x + 4; /* { dg-warning "dereference of possibly-NULL 'x'" } */ + /* { dg-warning "use of uninitialized value '\\*x'" "" { target *-*-* } .-1 } */ + z = arr[0] + 4; /* { dg-warning "dereference of possibly-NULL 'arr'" } */ + /* { dg-warning "use of uninitialized value '\\*arr'" "" { target *-*-* } .-1 } */ + + delete y; + delete x; + delete[] arr; +} diff --git a/gcc/testsuite/g++.dg/analyzer/noexcept-new.C b/gcc/testsuite/g++.dg/analyzer/noexcept-new.C new file mode 100644 index 00000000000..4a5f526b0f3 --- /dev/null +++ b/gcc/testsuite/g++.dg/analyzer/noexcept-new.C @@ -0,0 +1,48 @@ +/* { dg-additional-options "-O0 -fno-exceptions -fno-analyzer-suppress-followups" } */ +#include + +/* Test non-throwing variants of operator new */ + +struct A +{ + int x; + int y; +}; + +void test_throwing () +{ + int* x = new int; + int* y = new int(); /* { dg-warning "dereference of possibly-NULL" } */ + int* arr = new int[10]; + A *a = new A(); /* { dg-warning "dereference of possibly-NULL" } */ + + int z = *y + 2; + z = *x + 4; /* { dg-warning "dereference of possibly-NULL 'x'" } */ + /* { dg-warning "use of uninitialized value '\\*x'" "" { target *-*-* } .-1 } */ + z = arr[0] + 4; /* { dg-warning "dereference of possibly-NULL 'arr'" } */ + /* { dg-warning "use of uninitialized value '\\*arr'" "" { target *-*-* } .-1 } */ + a->y = a->x + 3; + + delete a; + delete y; + delete x; + delete[] arr; +} + +void test_nonthrowing () +{ + int* x = new(std::nothrow) int; + int* y = new(std::nothrow) int(); + int* arr = new(std::nothrow) int[10]; + + int z = *y + 2; /* { dg-warning "dereference of NULL 'y'" } */ + /* { dg-bogus "use of uninitialized value '\\*y'" "" { xfail *-*-* } .-1 } */ + z = *x + 4; /* { dg-warning "dereference of possibly-NULL 'x'" } */ + /* { dg-warning "use of uninitialized value '\\*x'" "" { target *-*-* } .-1 } */ + z = arr[0] + 4; /* { dg-warning "dereference of possibly-NULL 'arr'" } */ + /* { dg-warning "use of uninitialized value '\\*arr'" "" { target *-*-* } .-1 } */ + + delete y; + delete x; + delete[] arr; +} diff --git a/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C b/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C index d3076c3cf25..33872e6ddab 100644 --- a/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C +++ b/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C @@ -14,6 +14,6 @@ struct int_and_addr { int test (int_container ic) { - int_and_addr *iaddr = new (ic.addr ()) int_and_addr; + int_and_addr *iaddr = new (ic.addr ()) int_and_addr; /* { dg-warning "stack-based buffer overflow" } */ return iaddr->i; } diff --git a/gcc/testsuite/g++.dg/analyzer/placement-new-size.C b/gcc/testsuite/g++.dg/analyzer/placement-new-size.C new file mode 100644 index 00000000000..4536d361b4a --- /dev/null +++ b/gcc/testsuite/g++.dg/analyzer/placement-new-size.C @@ -0,0 +1,27 @@ +/* { dg-additional-options "-Wno-placement-new -Wno-analyzer-use-of-uninitialized-value" } */ + +#include +#include + +extern int get_buf_size (); + +void var_too_short () +{ + int8_t s; + int64_t *lp = new (&s) int64_t; /* { dg-warning "stack-based buffer overflow" } */ + /* { dg-warning "allocated buffer size is not a multiple of the pointee's size" "" { target *-*-* } .-1 } */ +} + +void static_buffer_too_short () +{ + int n = 16; + int buf[n]; + int *p = new (buf) int[n + 1]; /* { dg-warning "stack-based buffer overflow" } */ +} + +void symbolic_buffer_too_short () +{ + int n = get_buf_size (); + char buf[n]; + char *p = new (buf) char[n + 10]; /* { dg-warning "stack-based buffer overflow" } */ +} diff --git a/gcc/testsuite/g++.dg/analyzer/placement-new.C b/gcc/testsuite/g++.dg/analyzer/placement-new.C index 89055491a48..8edd778131b 100644 --- a/gcc/testsuite/g++.dg/analyzer/placement-new.C +++ b/gcc/testsuite/g++.dg/analyzer/placement-new.C @@ -1,3 +1,5 @@ +/* { dg-additional-options "-Wno-free-nonheap-object -fexceptions" } */ + #include /* Placement new. */ @@ -14,15 +16,101 @@ void test_2 (void) { char buf[sizeof(int) * 10]; int *p = new(buf) int[10]; -} +} // { dg-prune-output "-Wfree-nonheap-object" } /* Delete of placement new. */ void test_3 (void) { char buf[sizeof(int)]; // { dg-message "region created on stack here" } - int *p = new(buf) int (42); + int *p = new (buf) int (42); + delete p; // { dg-warning "memory on the stack" } +} + +// { dg-prune-output "-Wfree-nonheap-object" } + +void test_4 (void) +{ + int buf[5]; // { dg-message "region created on stack here" } + int *p = new (&buf[2]) int (42); delete p; // { dg-warning "memory on the stack" } } + // { dg-prune-output "-Wfree-nonheap-object" } + +void test_write_placement_after_delete (void) +{ + short *s = ::new short; + short *lp = ::new (s) short; + ::delete s; + *lp = 12; /* { dg-warning "use after 'delete' of 'lp'" "write placement new after buffer deletion" } */ +} + +void test_read_placement_after_delete (void) +{ + short *s = ::new short; + short *lp = ::new (s) short; + ::delete s; + short m = *lp; // { dg-warning "use after 'delete' of 'lp'" "read placement new after buffer deletion" } +} + +struct A +{ + int x; + int y; +}; + +void test_use_placement_after_destruction (void) +{ + A a; + int *lp = ::new (&a.y) int; + *lp = 2; /* { dg-bogus "-Wanalyzer-use-of-uninitialized-value" } */ + a.~A(); + int m = *lp; /* { dg-warning "use of uninitialized value '\\*lp'" "use of placement after the underlying buffer was destructed." } */ +} + +void test_initialization_through_placement (void) +{ + int x; + int *p = ::new (&x) int; + *p = 10; + int z = x + 2; /* { dg-bogus "use of uninitialized value 'x'" "x has been initialized through placement pointer" } */ +} + +void test_partial_initialization_through_placement (void) +{ + char buf[4]; + char *p = ::new (&buf[2]) char; + *p = 10; + char *y = ::new (&buf[0]) char; + char z = buf[2] + 2; /* { dg-bogus "use of uninitialized value" } */ + z = *y + 2; /* { dg-warning "use of uninitialized value '\\*y'" "y has only been partially initialized" } */ +} + + +void test_delete_placement (void) +{ + A *a = ::new A; /* { dg-bogus "use of possibly-NULL 'operator new(8)' where non-null expected" "throwing new cannot be null" } */ + int *z = ::new (&a->y) int; + a->~A(); // deconstruct properly + ::operator delete (a); + ::operator delete (z); /* { dg-warning "use after 'delete' of 'z'" } */ +} + +void test_delete_placement_2 (void) +{ + A *a = ::new A; /* { dg-bogus "use of possibly-NULL 'operator new(8)' where non-null expected" "throwing new cannot be null" } */ + int *z = ::new (&a->y) int; + delete a; + ::operator delete (z); /* { dg-warning "use after 'delete' of 'z'" } */ +} + +void test_use_placement_after_deallocation (void) +{ + A *a = ::new A (); + int *lp = ::new (&a->y) int; + *lp = 2; /* { dg-bogus "use of uninitialized value" } */ + ::operator delete (a); + int m = *lp; /* { dg-warning "use after 'delete' of 'lp'" "use of placement after the underlying buffer was deallocated." } */ +}