From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x42b.google.com (mail-pf1-x42b.google.com [IPv6:2607:f8b0:4864:20::42b]) by sourceware.org (Postfix) with ESMTPS id 622023857C65 for ; Fri, 4 Sep 2020 00:59:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 622023857C65 Received: by mail-pf1-x42b.google.com with SMTP id m8so3690320pfh.3 for ; Thu, 03 Sep 2020 17:59:45 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=Rdh6aEhArqccNGg+tBUiE+ldompjKzV5OnqxP1BaxxU=; b=EWT07b34UCAMbGx25AeEM7KHdlEPKBlsOeoV9XkVdRw0fwn7EDLh7DDW2jZWpXeIKH QqdQunbSYG2Iys8yFAkrdheUya3c7AA06NoaHAqoOxf2PIhXvj69/7uppgvkWNgj8Vhx XaFRW1S2vqWDbeB2bQFWW53IGKV1OsIt2OeopbDJ1oxkRwKTheR1pSd7RjENac6BYSgx MPcaLkWXGleaYaQf/gzRyd85qo2JInIdT2RpUWmW3Ujix28TuT7J68lS0eiYI1FQ8mQZ SyGQOZCbha+5NyB+w4cqzAY/6ltgK+b0xGcibzBQfoMyFSz3lS1mjnVANaXGk+CER0dW 3TVQ== X-Gm-Message-State: AOAM533mkj9zgfX0acBVNjriTRJNvPu6vD5SJIhS9FIu8QrppxJjJ4Wt FokW+Rk20JJAykonRU0suRY= X-Google-Smtp-Source: ABdhPJwNfG54HbidbHr5p23QXgGBArLSY2xJndB0ZUJX/e/I0xBpTblh2tifaduBD5EtObzeJDp9nA== X-Received: by 2002:a63:c64c:: with SMTP id x12mr5204262pgg.433.1599181184286; Thu, 03 Sep 2020 17:59:44 -0700 (PDT) Received: from bubble.grove.modra.org ([2406:3400:51d:8cc0:ac34:d205:6aa4:829]) by smtp.gmail.com with ESMTPSA id a19sm4511886pfn.10.2020.09.03.17.59.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 03 Sep 2020 17:59:43 -0700 (PDT) Received: by bubble.grove.modra.org (Postfix, from userid 1000) id A659081314; Fri, 4 Sep 2020 10:29:39 +0930 (ACST) Date: Fri, 4 Sep 2020 10:29:39 +0930 From: Alan Modra To: Iain Buclaw Cc: gcc-patches@gcc.gnu.org, Ian Lance Taylor Subject: Re: ubsan: d-demangle.c:214 signed integer overflow Message-ID: <20200904005939.GR15695@bubble.grove.modra.org> References: <20200903130116.GQ15695@bubble.grove.modra.org> <1599163400.8r2ly1k30n.astroid@galago.none> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1599163400.8r2ly1k30n.astroid@galago.none> User-Agent: Mutt/1.9.4 (2018-02-28) X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, 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-List-Received-Date: Fri, 04 Sep 2020 00:59:47 -0000 On Thu, Sep 03, 2020 at 11:02:50PM +0200, Iain Buclaw wrote: > Excerpts from Alan Modra's message of September 3, 2020 3:01 pm: > > Running the libiberty testsuite > > ./test-demangle < libiberty/testsuite/d-demangle-expected > > libiberty/d-demangle.c:214:14: runtime error: signed integer overflow: 922337203 * 10 cannot be represented in type 'long int' > > > > On looking at silencing ubsan, I found a real bug in dlang_number. > > For a 32-bit long, some overflows won't be detected. For example, > > 21474836480. Why? Well 214748364 * 10 is 0x7FFFFFF8 (no overflow so > > far). Adding 8 gives 0x80000000 (which does overflow but there is no > > test for that overflow in the code). Then multiplying 0x80000000 * 10 > > = 0x500000000 = 0 won't be caught by the multiplication overflow test. > > The same holds for a 64-bit long using similarly crafted digit > > sequences. > > > > This patch replaces the mod 10 test with a simpler limit test, and > > similarly the mod 26 test in dlang_decode_backref. > > > > About the limit test: > > val * 10 + digit > ULONG_MAX is the condition for overflow > > ie. > > val * 10 > ULONG_MAX - digit > > or > > val > (ULONG_MAX - digit) / 10 > > or assuming the largest digit > > val > (ULONG_MAX - 9) / 10 > > > > I resisted the aesthetic appeal of simplifying this further to > > val > -10UL / 10 > > since -1UL for ULONG_MAX is only correct for 2's complement numbers. > > > > Passes all the libiberty tests, on both 32-bit and 64-bit hosts. OK > > to apply? > > > > Thanks Alan, change seems reasonable, however on giving it a mull over, > I see that the largest number that dlang_number would need to be able to > handle is UINT_MAX. These two tests which decode a wchar value are > representative of that (first is valid, second invalid). > > # > --format=dlang > _D4test21__T3funVwi4294967295Z3funFNaNbNiNfZv > test.fun!('\Uffffffff').fun() > # > --format=dlang > _D4test21__T3funVwi4294967296Z3funFNaNbNiNfZv > _D4test21__T3funVwi4294967296Z3funFNaNbNiNfZv Hmm, OK, on a 32-bit host those both won't be handled due to the "(long) val < 0" test. Do you really want the last one to fail on a 64-bit host, ie. not produce "test.fun!('\U100000000').fun()"? I'm guessing you do, but I'd like that confirmed before posting a followup patch. > I'm fine with creating a new PR and dealing with the above in a separate > change though, as it will require a few more replacements to adjust the > result parameter type to 'unsigned' or 'long long'. Ha! When I wrote the original patch I changed most of the "long" parameters and variables to "unsigned long", because it was clear from the code (and my understanding of name mangling) that the values were in fact always unsigned. I also changed "int" to "size_t" where appropriate, not that you need to handle strings larger than 2G in length but simply that size_t is the correct type to use with string functions, malloc, memcpy and so on. I decided to remove all those changes before posting as they were just tidies, I thought. -- Alan Modra Australia Development Lab, IBM