From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 118159 invoked by alias); 17 Oct 2018 18:38:55 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 118141 invoked by uid 89); 17 Oct 2018 18:38:54 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-0.3 required=5.0 tests=AWL,BAYES_50,KAM_SHORT,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=no version=3.3.2 spammy=_obstack_size_t, *obstackptr, sk:summary, html_node X-HELO: mail.baldwin.cx Received: from bigwig.baldwin.cx (HELO mail.baldwin.cx) (96.47.65.170) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 17 Oct 2018 18:38:51 +0000 Received: from John-Baldwins-MacBook-Pro-2.local (ralph.baldwin.cx [66.234.199.215]) by mail.baldwin.cx (Postfix) with ESMTPSA id 81EDA10A87D; Wed, 17 Oct 2018 14:38:48 -0400 (EDT) Subject: Re: [PATCH][gdb] fix unsigned overflow in charset.c To: Pedro Alves , Paul Koning References: <7B48D309-445E-4141-A87A-1F3D5FA70EFD@comcast.net> <1acace4a-a5c6-abaf-f070-9c2e6768b6f2@redhat.com> <9ea7a1f6-5c3f-c569-6bba-ca9e21711de1@FreeBSD.org> <2DCE0AB8-5647-4AD1-B0AA-3A8350C3BE6D@comcast.net> <17fcbb42-d694-af87-9a8d-d01addee992b@FreeBSD.org> <42e6b4b2-fb05-25f6-ef0a-73ce854116de@redhat.com> Cc: gdb-patches@sourceware.org From: John Baldwin Message-ID: <31fcba01-1f2c-d932-94f2-69b7cb14601a@FreeBSD.org> Date: Wed, 17 Oct 2018 18:38:00 -0000 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <42e6b4b2-fb05-25f6-ef0a-73ce854116de@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2018-10/txt/msg00381.txt.bz2 On 10/16/18 8:58 AM, Pedro Alves wrote: > On 10/11/2018 09:15 PM, John Baldwin wrote: >> On 10/10/18 1:50 AM, Pedro Alves wrote: >>> On 10/09/2018 08:58 PM, John Baldwin wrote: >>>> On 10/9/18 11:10 AM, Paul Koning wrote: >>>>> >>>>> >>>>>> On Oct 9, 2018, at 1:57 PM, John Baldwin wrote: >>>>>> >>>>>> On 10/9/18 10:40 AM, Paul Koning wrote: >>>>>>> >>>>>>> >>>>>>>> On Oct 9, 2018, at 1:31 PM, Pedro Alves wrote: >>>>>>>> >>> >>>>>> I also ran into the same failure using LLVM's ubsan on FreeBSD but in a different >>>>>> use of obstack_blank_fast(). If we wanted to fix this, I wonder if we'd instead >>>>>> want to fix it centrally in obstack_blank_fast (e.g. by using a ptrdiff_t cast) >>>>>> rather than fixing various consumers of the API. That would be a change to >>>>>> libiberty though, not just gdb. >>>>> >>>>> I suppose. But casts in macros scare me, they can hide mistakes. It seems more reasonable to have the caller be responsible for creating a value of the correct type. Since it's an adjustment, I suppose the cast should be for ptrdiff_t rather than ssize_t? >>>> >>>> So if obstack_blank_fast() were an inline function instead of a macro, I >>>> suspect it's second argument would be of type ptrdiff_t in which case the >>>> equivalent "hidden" cast would happen at the function call. That said, >>>> the obstack_blank() macro uses _OBSTACK_SIZE_T (which is an unsigned size_t) >>>> when it declares a local variable to pass as the offset, so it seems obstack >>>> really is relying on unsigned wrap around. >>> >>> The function is documented to take an int, at least: >>> >>> void obstack_blank_fast (struct obstack *obstack-ptr, int size) >>> >>> https://www.gnu.org/software/libc/manual/html_node/Summary-of-Obstacks.html >>> >>> Not sure what's best to do, but I think I leaning toward >>> agreeing with Paul, in that passing down a signed negative >>> integer rather than passing down a large unsigned integer >>> expecting it to cast to a negative integer ends up >>> being a little better. >> >> Ok. Do you have a preference on the type to use (ssize_t vs ptrdiff_t vs >> something else)? Paul's original patch used ssize_t. I'll probably patch >> the one case I found in minsyms.c to match whatever we use here. > > I don't really have much of a preference. > > In practice, it probably doesn't make much of a difference nowadays. > Likely ssize_t and ptrdiff_t have the same width on all supported > hosts. > > ssize_t is not standard C++ (it's standard POSIX), while ptrdiff_t is. > OTOH, we already use ssize_t in gdb. Pedantically incorrectly, I guess, > if we follow the letter of the original ssize_t intention [1]: > > The type ssize_t shall be capable of storing values at least in the range [-1, {SSIZE_MAX}]. > > [1] - http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html > > From an aesthetic perspective, "ssize_t" seems better, as the "obvious > signed version of size_t". From a pedantic perspective, ptrdiff_t > sounds better. Ok, I think ssize_t is probably fine, so I Think Paul's original patch is ok? -- John Baldwin                                                                            Â