From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 101558 invoked by alias); 14 Dec 2016 06:06:08 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 101548 invoked by uid 89); 14 Dec 2016 06:06:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=BAYES_50,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=sk:host_wi, sk:HOST_WI, INT_MIN, int_min X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 14 Dec 2016 06:05:57 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B3D413B703; Wed, 14 Dec 2016 06:05:55 +0000 (UTC) Received: from localhost.localdomain (ovpn-116-182.phx2.redhat.com [10.3.116.182]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uBE65rL8025888; Wed, 14 Dec 2016 01:05:54 -0500 From: Jeff Law Subject: Re: [PATCH] fix integer overflow bugs in gimple-ssa-sprintf.c (PR 78608) To: Martin Sebor , Gcc Patch List References: <7258e3eb-6942-faac-cbb3-80a4ee966521@gmail.com> Message-ID: Date: Wed, 14 Dec 2016 06:08:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <7258e3eb-6942-faac-cbb3-80a4ee966521@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2016-12/txt/msg01217.txt.bz2 On 12/02/2016 05:36 PM, Martin Sebor wrote: > Bug 78608 - gimple-ssa-sprintf.c:570:17: runtime error: negation > of -9223372036854775808 cannot be represented in type 'long int' > points out an integer overflow bug in the pass caught by ubsan. > The bug was due to negating a number without checking for equality > to INT_MIN. > > In addition, my recent change to fix 78521 introduced a call to > abs() that broke the Solaris bootstrap: > > https://gcc.gnu.org/ml/gcc-patches/2016-12/msg00161.html > > While fixing these two problems I noticed that the rest of the pass > wasn't handling the corner case of a width with the value of INT_MIN > specified via an argument to the asterisk, such as in: > > int n = snprintf(0, 0, "%*i", INT_MIN, 0); > > This is undefined behavior because negative width is supposed to be > treated as the left justification flag followed by a positive width > (thus resulting in INT_MAX + 1 bytes). This problem affected all > integer and floating point directives. > > Finally, while there, I decided to include in information messages > a bit of detail about ranges of floating point values that was > missing. I did this to help answer questions like those raised > earlier this week by Gerald here ("where does the 317 come from?): > > https://gcc.gnu.org/ml/gcc/2016-11/msg00102.html > > The attached patch adjusts the pass to handle these problems. > > Martin > > gcc-78608.diff > > > PR tree-optimization/78608 - gimple-ssa-sprintf.c:570:17: runtime error: negation of -9223372036854775808 cannot be represented in type 'long int' > > gcc/ChangeLog: > > PR tree-optimization/78608 > * gimple-ssa-sprintf.c (tree_digits): Avoid negating a minimum signed > value. > (get_width_and_precision): Same. > (format_integer): Use HOST_WIDE_INT for expected sprintf return value > to allow for width being the absolte value of INT_MIN. > (format_floating): Use HOST_WIDE_INT for width and precision. Store > argument type for use in diagnostics. Use target INT_MAX rather than > the host constant. > (format_floating): Reuse get_width_and_precision instead of hadcoding s/hadcoding/hardcoding > the same. > (maybe_get_value_range_from_type): New function. > (format_directive): Treat INT_MAX + 1 as a valid (if excessive) byte > count. Call maybe_get_value_range_from_type. > > gcc/testsuite/ChangeLog: > > PR tree-optimization/78608 > * gcc.dg/tree-ssa/builtin-sprintf-warn-1.c: Add test cases. So I've been going back and forth on whether or not to suggest a slight change in how we're working. Specifically the practice of lumping multiple fixes into a single patch -- I realize that it's usually the case that you're finding related issues so there's a desire to address them as a group. Doing things that way also tends to avoid interdependent patches. The problem is that makes reviewing more difficult and thus I'm much less likely to knock it out as soon as it flys by my inbox. And I think I've seen several trivial to review, but important fixes inside larger, harder to review changes. Let's try to keep patches to addressing one issue for the future. If you have multiple related issues, you should consider a series of patches. Hopefully that will cut down on the number of things are getting stalled. > > Index: gcc/gimple-ssa-sprintf.c > =================================================================== > --- gcc/gimple-ssa-sprintf.c (revision 243196) > +++ gcc/gimple-ssa-sprintf.c (working copy) > @@ -565,8 +565,14 @@ tree_digits (tree x, int base, bool plus, bool pre > if (tree_fits_shwi_p (x)) > { > HOST_WIDE_INT i = tree_to_shwi (x); > - if (i < 0) > + if (HOST_WIDE_INT_MIN == i) nit. I think most folks would probably prefer this as if (i == HOST_WIDE_INT_MIN). HOST_WIDE_INT_MIN is a constant and when we can write an expression in either order variable OP const is the preferred order. You seem to be going back and forth between both styles. Let' try to stick with variable OP const. I don't think you need to go back and fix all of the existing const OP variable instances right now, but we may in the future. > @@ -1221,13 +1251,15 @@ static fmtresult > { > /* The minimum output is "0x.p+0". */ > res.range.min = 6 + (prec > 0 ? prec : 0); > - res.range.max = (width == INT_MIN > - ? HOST_WIDE_INT_MAX > + /* The maximum is the greater of widh and the number of bytes s/widh/width/ > @@ -1316,42 +1350,23 @@ format_floating (const conversion_spec &spec, tree > /* Set WIDTH to -1 when it's not specified, to INT_MIN when it is > specified by the asterisk to an unknown value, and otherwise to > a non-negative value corresponding to the specified width. */ > - int width = -1; > - int prec = -1; > + HOST_WIDE_INT width; > + HOST_WIDE_INT prec; > + get_width_and_precision (spec, &width, &prec); > > + /* FIXME: Handle specified precision with an unknown value. */ > + if (prec == HOST_WIDE_INT_MIN) > + return fmtresult (); Is this something that needs to be fixed now or was this a marker for future? > + /* Adjust the count if it's less than the specified width. */ > + if (0 < width && *minmax[i] < (unsigned HOST_WIDE_INT)width) > + *minmax[i] = width; width > 0 && ... > @@ -1684,6 +1701,44 @@ format_string (const conversion_spec &spec, tree a > return res; > } > > +/* Helper to construct tree nodes corresponding to the minimum and > + maxium values for a type. When *ARGMIN is a real floating type s/maxium/maximum/ > + > + /* Get the real type format desription for the target. */ s/desription/description/ If the FIXME was a future thing, then this is OK with the nits fixed. If the FIXME was a marker for something you intended to address now and just forgot, then we either need another iteration or a follow-up patch depending on the severity of the FIXME in your mind. jeff