From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by sourceware.org (Postfix) with ESMTP id 38D40393C86C for ; Fri, 13 Nov 2020 21:13:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 38D40393C86C 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-60-WpgkcP8TMludvv11c_nr2Q-1; Fri, 13 Nov 2020 16:12:56 -0500 X-MC-Unique: WpgkcP8TMludvv11c_nr2Q-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 90EA11016CE8; Fri, 13 Nov 2020 21:12:55 +0000 (UTC) Received: from localhost (unknown [10.33.36.92]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2378F7C452; Fri, 13 Nov 2020 21:12:54 +0000 (UTC) Date: Fri, 13 Nov 2020 21:12:54 +0000 From: Jonathan Wakely To: Mike Crowe Cc: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: Re: [committed] libstdc++: Optimise std::future::wait_for and fix futex polling Message-ID: <20201113211254.GJ503596@redhat.com> References: <20201112234940.GA475138@redhat.com> <20201113110218.GG503596@redhat.com> <20201113172522.GI503596@redhat.com> <20201113202934.GA23583@mcrowe.com> MIME-Version: 1.0 In-Reply-To: <20201113202934.GA23583@mcrowe.com> X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/mixed; boundary="0k4Rxg87Lb8yV0u3" Content-Disposition: inline X-Spam-Status: No, score=-14.1 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_NONE, RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=unavailable autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libstdc++@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libstdc++ mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 Nov 2020 21:13:02 -0000 --0k4Rxg87Lb8yV0u3 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline On 13/11/20 20:29 +0000, Mike Crowe via Libstdc++ wrote: >On Friday 13 November 2020 at 17:25:22 +0000, Jonathan Wakely wrote: >> + // Return the relative duration from (now_s + now_ns) to (abs_s + abs_ns) >> + // as a timespec. >> + struct timespec >> + relative_timespec(chrono::seconds abs_s, chrono::nanoseconds abs_ns, >> + time_t now_s, long now_ns) >> + { >> + struct timespec rt; >> + >> + // Did we already time out? >> + if (now_s > abs_s.count()) >> + { >> + rt.tv_sec = -1; >> + return rt; >> + } >> + >> + auto rel_s = abs_s.count() - now_s; >> + >> + // Avoid overflows >> + if (rel_s > __gnu_cxx::__int_traits::__max) >> + rel_s = __gnu_cxx::__int_traits::__max; >> + else if (rel_s < __gnu_cxx::__int_traits::__min) >> + rel_s = __gnu_cxx::__int_traits::__min; > >I may be missing something, but if the line above executes... > >> + >> + // Convert the absolute timeout value to a relative timeout >> + rt.tv_sec = rel_s; >> + rt.tv_nsec = abs_ns.count() - now_ns; >> + if (rt.tv_nsec < 0) >> + { >> + rt.tv_nsec += 1000000000; >> + --rt.tv_sec; > >...and so does this line above, then I think that we'll end up >underflowing. (Presumably rt.tv_sec will wrap round to being some time in >2038 on most 32-bit targets.) Ugh. >I'm currently trying to persuade myself that this can actually happen and >if so work out how to come up with a test case for it. Maybe something like: auto d = chrono::floor(system_clock::now().time_since_epoch() - seconds(INT_MAX + 2LL)); fut.wait_until(system_clock::time_point(d)); This will create a sys_time with a value that is slightly more than INT_MAX seconds before the current time, with a zero nanoseconds component. The difference between the gettimeofday result and this time will be slightly more negative than INT_MIN and so will overflow a 32-bit time_t, causing the code to use __int_traits::__min. As long as the gettimeofday call doesn't happen to also have a zero nanoseconds component, the difference of the nanoseconds values will be negative, and we will decrement the time_t value. >I don't believe that this part changed in your later patch. Right. Thanks for catching this. The attached patch should fix it. There's no point clamping very negative values to the minimum time_t value, since any relative timeout less than zero has already passed. So we can just use -1 there (and not bother with the tv_nsec field at all): else if (rel_s <= 0) [[unlikely]] { rt.tv_sec = -1; } But please check my working. >Oh how I wish all these functions didn't expect already cracked seconds and >nanoseconds. :( Indeed. At some point (probably not for GCC 11) I'm going to add a proper wrapper class for all these futex calls, and make it robust, and then replace the various hand-crafted uses of syscall(SYS_futex, ...) with that type so we only need to fix all this in one place. And it will just take a duration or time_point. --0k4Rxg87Lb8yV0u3 Content-Type: text/x-patch; charset=us-ascii Content-Disposition: attachment; filename="patch.txt" commit a77f131249202c7342cc385f2d0473ee032ae0eb Author: Jonathan Wakely Date: Fri Nov 13 20:57:15 2020 libstdc++: Fix another 32-bit time_t overflow in futex timeouts libstdc++-v3/ChangeLog: * src/c++11/futex.cc (relative_timespec): Avoid overflow if the difference of seconds is the minimum time_t value and the difference of nanoseconds is also negative. diff --git a/libstdc++-v3/src/c++11/futex.cc b/libstdc++-v3/src/c++11/futex.cc index 15959cebee57..50768403cc63 100644 --- a/libstdc++-v3/src/c++11/futex.cc +++ b/libstdc++-v3/src/c++11/futex.cc @@ -75,19 +75,25 @@ namespace auto rel_s = abs_s.count() - now_s; - // Avoid overflows + // Convert the absolute timeout to a relative timeout, without overflow. if (rel_s > __int_traits::__max) [[unlikely]] - rel_s = __int_traits::__max; - else if (rel_s < __int_traits::__min) [[unlikely]] - rel_s = __int_traits::__min; - - // Convert the absolute timeout value to a relative timeout - rt.tv_sec = rel_s; - rt.tv_nsec = abs_ns.count() - now_ns; - if (rt.tv_nsec < 0) { - rt.tv_nsec += 1000000000; - --rt.tv_sec; + rt.tv_sec = __int_traits::__max; + rt.tv_nsec = 999999999; + } + else if (rel_s <= 0) [[unlikely]] + { + rt.tv_sec = -1; + } + else + { + rt.tv_sec = rel_s; + rt.tv_nsec = abs_ns.count() - now_ns; + if (rt.tv_nsec < 0) + { + rt.tv_nsec += 1000000000; + --rt.tv_sec; + } } return rt; --0k4Rxg87Lb8yV0u3--