Re: [PATCH][gdb] fix unsigned overflow in charset.c

[John Baldwin <jhb at FreeBSD dot org>]

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 <jhb@FreeBSD.org> wrote:
>>>>
>>>> On 10/9/18 10:40 AM, Paul Koning wrote:
>>>>>
>>>>>
>>>>>> On Oct 9, 2018, at 1:31 PM, Pedro Alves <palves@redhat.com> 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.

-- 
John Baldwin