From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io1-xd30.google.com (mail-io1-xd30.google.com [IPv6:2607:f8b0:4864:20::d30]) by sourceware.org (Postfix) with ESMTPS id 7F7CE3857C5E for ; Tue, 18 Jan 2022 16:22:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7F7CE3857C5E Received: by mail-io1-xd30.google.com with SMTP id s11so18114141ioe.12 for ; Tue, 18 Jan 2022 08:22:51 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=S7TElhmeYLXIR0UlbJLCuMDhZGDGTSpPs/D6U9gqGYA=; b=XhdcjxUnp7rMRelzzlkV+pFfNw4XCdjj3PUAaz2GqgOpsEZq7cBxAoIuS976GMtnI4 VwvONEuqmIoSUBuaWFlC0jAX8ftFvkCZOejw6cYWJ+hdOZE/nnkUPfIutrb7k8rt9g4w z2iwdnYPBRys0DX4BrJqAFaxBQaLNB86eNfBjGRlxD/4ixdJ7FfL2AINOvBZ0+eLUACG a0ZPBA/e7neV9Dk0G9+7sgaG4CXwlAqkOPuljWNhZTn19+iNneMQ2nYU9jrW0hMb6UE/ kMgfAylIPVbGazpBsLal0kYZNZCg1wM0jZWHkLwlsjrpCDKwueaLw6lpgcPbanjRpiAz 3/yw== X-Gm-Message-State: AOAM532ZhZObWjdwSSIp/1mcbzC4b3tvpHkI6I3DoLZ/ph7Fa3v1Q880 R+mgV4xankGzYCLhOQN6hao= X-Google-Smtp-Source: ABdhPJwQiQnGPt45FC//s48oMXBtoKo1da08jk7AJ532az/pPwXwmcEfKJ+n6xUJsSDfg3XxrVNDuA== X-Received: by 2002:a02:cc3c:: with SMTP id o28mr6384493jap.305.1642522970740; Tue, 18 Jan 2022 08:22:50 -0800 (PST) Received: from [192.168.0.41] (97-118-100-142.hlrn.qwest.net. [97.118.100.142]) by smtp.gmail.com with ESMTPSA id z6sm4640688iol.38.2022.01.18.08.22.50 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 18 Jan 2022 08:22:50 -0800 (PST) Message-ID: <7598c5c4-08e4-0758-36a2-4b7fcf7145aa@gmail.com> Date: Tue, 18 Jan 2022 09:22:49 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.0 Subject: Re: [PATCH] middle-end/101292 - invalid memory access with warning control Content-Language: en-US To: Richard Biener Cc: gcc-patches@gcc.gnu.org References: <7pq3onsr-r66r-s43o-7p4-9n5o5672q1q@fhfr.qr> <7a70a23c-e084-7fab-d532-0c7d38a71142@gmail.com> From: Martin Sebor In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Tue, 18 Jan 2022 16:22:53 -0000 On 1/18/22 01:36, Richard Biener wrote: > On Mon, 17 Jan 2022, Martin Sebor wrote: > >> On 1/17/22 07:32, Richard Biener via Gcc-patches wrote: >>> The warning control falls into the C++ trap of using a reference >>> to old hashtable contents for a put operation which can end up >>> re-allocating that before reading from the old freed referenced to >>> source. Fixed by introducing a temporary. >> >> I think a better place to fix this and avoid the gotcha once and >> for all is in the GCC hash_map: C++ containers are expected to >> handle the insertion of own elements gracefully. > > I don't think that's reasonably possible if you consider > > T *a = map.get (X); > T *b = map.get (Y); > map.put (Z, *a); > map.put (W, *b); This case is up to the caller to handle, the same as anything else involving pointers or references into reallocated storage (it's no different in C than it is in C++). The specific case I'm referring to is passing a pointer or reference to a single element in a container to the first modifying call on the container. > > the only way to "fix" it would be to change the API to not > return by reference for get, remove get_or_insert (or change > its API to also require passing the new value). No, the fix is to have the modifying function create a copy of the element being inserted before reallocating the container. > > Note the above shows that making 'put' take the value by > value instead of by reference doesn't work either. > > IMHO the issue is that C++ doesn't make it obvious that 'put' > gets a pointer to the old element (stupid references). The problem isn't specific to references, it can come up with pointers just as easily. Pointers might just make it more obvious. Martin > > Richard. > >> Martin >> >>> >>> Bootstrap & regtest running on x86_64-unknown-linux-gnu. >>> >>> 2022-01-17 Richard Biener >>> >>> PR middle-end/101292 >>> * diagnostic-spec.c (copy_warning): Make sure to not >>> reference old hashtable content on possible resize. >>> * warning-control.cc (copy_warning): Likewise. >>> --- >>> gcc/diagnostic-spec.c | 5 ++++- >>> gcc/warning-control.cc | 3 ++- >>> 2 files changed, 6 insertions(+), 2 deletions(-) >>> >>> diff --git a/gcc/diagnostic-spec.c b/gcc/diagnostic-spec.c >>> index a8af229d677..4341ccfaae9 100644 >>> --- a/gcc/diagnostic-spec.c >>> +++ b/gcc/diagnostic-spec.c >>> @@ -195,7 +195,10 @@ copy_warning (location_t to, location_t from) >>> else >>> { >>> if (from_spec) >>> - nowarn_map->put (to, *from_spec); >>> + { >>> + nowarn_spec_t tem = *from_spec; >>> + nowarn_map->put (to, tem); >>> + } >>> else >>> nowarn_map->remove (to); >>> } >>> diff --git a/gcc/warning-control.cc b/gcc/warning-control.cc >>> index f9808bf4392..fa39ecab421 100644 >>> --- a/gcc/warning-control.cc >>> +++ b/gcc/warning-control.cc >>> @@ -206,7 +206,8 @@ void copy_warning (ToType to, FromType from) >>> gcc_assert (supp); >>> >>> gcc_checking_assert (nowarn_map); >>> - nowarn_map->put (to_loc, *from_spec); >>> + nowarn_spec_t tem = *from_spec; >>> + nowarn_map->put (to_loc, tem); >>> } >>> else >>> { >> >> >