From patchwork Tue May 4 22:20:33 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jonathan Wakely X-Patchwork-Id: 1474042 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=2620:52:3:1:0:246e:9693:128c; helo=sourceware.org; envelope-from=gcc-patches-bounces@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.a=rsa-sha256 header.s=default header.b=yYx/tonl; dkim-atps=neutral Received: from sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4FZZ6j09tKz9s1l for ; Wed, 5 May 2021 08:20:45 +1000 (AEST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 38FD13A8C838; Tue, 4 May 2021 22:20:41 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 38FD13A8C838 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1620166841; bh=Mwdb6QzArAj5FsLLkJMXMQMbQfvQC9/KBB/yquTkeUE=; h=Date:To:Subject:References:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=yYx/tonlohB9tugzw0rruNo07Ce6iiNyAQDSBwnnQD80k/14rU5ETK6pv65LQrsHQ hZ6F+VItzZ8L1FNzkfI3KweGoywZmIpsx413dVdQ8Mrbz6vqJIu5U/opSY8N7QFeTs sPkSsdxZXBe0XRpIc85JjumaIdqe4+0dkuFC/8No= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 645663A8C822 for ; Tue, 4 May 2021 22:20:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 645663A8C822 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-134-h6W88ZD1MA6eHXghpwBDpw-1; Tue, 04 May 2021 18:20:35 -0400 X-MC-Unique: h6W88ZD1MA6eHXghpwBDpw-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 57A81824FA8; Tue, 4 May 2021 22:20:34 +0000 (UTC) Received: from localhost (unknown [10.33.36.164]) by smtp.corp.redhat.com (Postfix) with ESMTP id F0BC41002D71; Tue, 4 May 2021 22:20:33 +0000 (UTC) Date: Tue, 4 May 2021 23:20:33 +0100 To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: [committed 4/4] libstdc++: Fix null dereferences in std::promise Message-ID: <20210504222033.GO3008@redhat.com> References: MIME-Version: 1.0 In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Disposition: inline X-Spam-Status: No, score=-14.3 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Jonathan Wakely via Gcc-patches From: Jonathan Wakely Reply-To: Jonathan Wakely Errors-To: gcc-patches-bounces@gcc.gnu.org Sender: "Gcc-patches" This fixes some ubsan errors in std::promise: future:1153:34: runtime error: member call on null pointer of type 'struct element_type' future:1153:34: runtime error: member access within null pointer of type 'struct element_type' The problem is that the check for a null pointer is done inside the _State::__Setter function, which is only evaluated after evaluating the _M_future->_M_set_result postfix-expression. This change adds a new promise::_M_state() helper for accessing _M_future, and moves the check for no shared state into there, instead of inside the __setter functions. The __setter functions are made always_inline, to avoid the situation where the linker selects the old version of set_value (without the _S_check call) and the new version of __setter (without the _S_check call) and so there is no check. With the always_inline attribute the old version of set_value will either inline the old __setter or call an extern definition of it, and the new set_value will do the check itself, so both versions do the check. libstdc++-v3/ChangeLog: * include/std/future (promise::set_value): Check for existence of shared state before dereferncing it. (promise::set_exception, promise::set_value_at_thread_exit) (promise::set_exception_at_thread_exit): Likewise. (promise::set_value, promise::set_exception) (promise::set_value_at_thread_exit) (promise::set_exception_at_thread_exit): Likewise. (promise::set_value, promise::set_exception) (promise::set_value_at_thread_exit) (promise::set_exception_at_thread_exit): Likewise. * testsuite/30_threads/promise/members/at_thread_exit2.cc: Remove unused variable. Tested x86_64-linux. Committed to trunk. commit 058d6acefe8bac4a66c8e7fb4951276db188e2d8 Author: Jonathan Wakely Date: Tue May 4 16:28:57 2021 libstdc++: Fix null dereferences in std::promise This fixes some ubsan errors in std::promise: future:1153:34: runtime error: member call on null pointer of type 'struct element_type' future:1153:34: runtime error: member access within null pointer of type 'struct element_type' The problem is that the check for a null pointer is done inside the _State::__Setter function, which is only evaluated after evaluating the _M_future->_M_set_result postfix-expression. This change adds a new promise::_M_state() helper for accessing _M_future, and moves the check for no shared state into there, instead of inside the __setter functions. The __setter functions are made always_inline, to avoid the situation where the linker selects the old version of set_value (without the _S_check call) and the new version of __setter (without the _S_check call) and so there is no check. With the always_inline attribute the old version of set_value will either inline the old __setter or call an extern definition of it, and the new set_value will do the check itself, so both versions do the check. libstdc++-v3/ChangeLog: * include/std/future (promise::set_value): Check for existence of shared state before dereferncing it. (promise::set_exception, promise::set_value_at_thread_exit) (promise::set_exception_at_thread_exit): Likewise. (promise::set_value, promise::set_exception) (promise::set_value_at_thread_exit) (promise::set_exception_at_thread_exit): Likewise. (promise::set_value, promise::set_exception) (promise::set_value_at_thread_exit) (promise::set_exception_at_thread_exit): Likewise. * testsuite/30_threads/promise/members/at_thread_exit2.cc: Remove unused variable. diff --git a/libstdc++-v3/include/std/future b/libstdc++-v3/include/std/future index ef15fefa53c..09e54c3703b 100644 --- a/libstdc++-v3/include/std/future +++ b/libstdc++-v3/include/std/future @@ -532,26 +532,26 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION }; template + __attribute__((__always_inline__)) static _Setter<_Res, _Arg&&> - __setter(promise<_Res>* __prom, _Arg&& __arg) + __setter(promise<_Res>* __prom, _Arg&& __arg) noexcept { - _S_check(__prom->_M_future); return _Setter<_Res, _Arg&&>{ __prom, std::__addressof(__arg) }; } template + __attribute__((__always_inline__)) static _Setter<_Res, __exception_ptr_tag> - __setter(exception_ptr& __ex, promise<_Res>* __prom) + __setter(exception_ptr& __ex, promise<_Res>* __prom) noexcept { - _S_check(__prom->_M_future); return _Setter<_Res, __exception_ptr_tag>{ __prom, &__ex }; } template + __attribute__((__always_inline__)) static _Setter<_Res, void> - __setter(promise<_Res>* __prom) + __setter(promise<_Res>* __prom) noexcept { - _S_check(__prom->_M_future); return _Setter<_Res, void>{ __prom }; } @@ -1130,36 +1130,44 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION // Setting the result void set_value(const _Res& __r) - { _M_future->_M_set_result(_State::__setter(this, __r)); } + { _M_state()._M_set_result(_State::__setter(this, __r)); } void set_value(_Res&& __r) - { _M_future->_M_set_result(_State::__setter(this, std::move(__r))); } + { _M_state()._M_set_result(_State::__setter(this, std::move(__r))); } void set_exception(exception_ptr __p) - { _M_future->_M_set_result(_State::__setter(__p, this)); } + { _M_state()._M_set_result(_State::__setter(__p, this)); } void set_value_at_thread_exit(const _Res& __r) { - _M_future->_M_set_delayed_result(_State::__setter(this, __r), + _M_state()._M_set_delayed_result(_State::__setter(this, __r), _M_future); } void set_value_at_thread_exit(_Res&& __r) { - _M_future->_M_set_delayed_result( + _M_state()._M_set_delayed_result( _State::__setter(this, std::move(__r)), _M_future); } void set_exception_at_thread_exit(exception_ptr __p) { - _M_future->_M_set_delayed_result(_State::__setter(__p, this), + _M_state()._M_set_delayed_result(_State::__setter(__p, this), _M_future); } + + private: + _State& + _M_state() + { + __future_base::_State_base::_S_check(_M_future); + return *_M_future; + } }; template @@ -1241,25 +1249,33 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION // Setting the result void set_value(_Res& __r) - { _M_future->_M_set_result(_State::__setter(this, __r)); } + { _M_state()._M_set_result(_State::__setter(this, __r)); } void set_exception(exception_ptr __p) - { _M_future->_M_set_result(_State::__setter(__p, this)); } + { _M_state()._M_set_result(_State::__setter(__p, this)); } void set_value_at_thread_exit(_Res& __r) { - _M_future->_M_set_delayed_result(_State::__setter(this, __r), + _M_state()._M_set_delayed_result(_State::__setter(this, __r), _M_future); } void set_exception_at_thread_exit(exception_ptr __p) { - _M_future->_M_set_delayed_result(_State::__setter(__p, this), + _M_state()._M_set_delayed_result(_State::__setter(__p, this), _M_future); } + + private: + _State& + _M_state() + { + __future_base::_State_base::_S_check(_M_future); + return *_M_future; + } }; /// Explicit specialization for promise @@ -1333,22 +1349,30 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION // Setting the result void set_value() - { _M_future->_M_set_result(_State::__setter(this)); } + { _M_state()._M_set_result(_State::__setter(this)); } void set_exception(exception_ptr __p) - { _M_future->_M_set_result(_State::__setter(__p, this)); } + { _M_state()._M_set_result(_State::__setter(__p, this)); } void set_value_at_thread_exit() - { _M_future->_M_set_delayed_result(_State::__setter(this), _M_future); } + { _M_state()._M_set_delayed_result(_State::__setter(this), _M_future); } void set_exception_at_thread_exit(exception_ptr __p) { - _M_future->_M_set_delayed_result(_State::__setter(__p, this), + _M_state()._M_set_delayed_result(_State::__setter(__p, this), _M_future); } + + private: + _State& + _M_state() + { + __future_base::_State_base::_S_check(_M_future); + return *_M_future; + } }; template diff --git a/libstdc++-v3/testsuite/30_threads/promise/members/at_thread_exit2.cc b/libstdc++-v3/testsuite/30_threads/promise/members/at_thread_exit2.cc index 9f824efde52..679d580cee3 100644 --- a/libstdc++-v3/testsuite/30_threads/promise/members/at_thread_exit2.cc +++ b/libstdc++-v3/testsuite/30_threads/promise/members/at_thread_exit2.cc @@ -118,7 +118,6 @@ void test02() void test03() { std::promise p1; - int i = 0; p1.set_value(); try { p1.set_value_at_thread_exit();