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

[John Baldwin <jhb at FreeBSD dot org>]

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:
>>>>
>>>> On 10/09/2018 06:19 PM, Paul Koning wrote:
>>>>> This fixed an overflow in pointer arithmetic that crashes GDB on Mac OS.
>>>>
>>>> _unsigned_ overflow?  That isn't undefined.  Do we really want to trap
>>>> those?  I don't think GCC's version does that.
>>>>
>>>> From: 
>>>> https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#silencing-unsigned-integer-overflow
>>>> seems like there's a way to disable it.
>>>>
>>>> Thanks,
>>>> Pedro Alves
>>>
>>> You're right, it was an LLVM build.  I know unsigned overflow is well defined with integers; is that true for pointers?
>>>
>>> Given that GDB triggers this issue, should the GDB build do that LLVM workaround if LLVM is used to build it?  
>>>
>>> But it seems simpler to use the proposed patch; clearly the intent is to back up a pointer by -(space_left) and doing that operation on a signed type seems like a logical thing to do, it makes the intended meaning clear.
>>
>> 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.

-- 
John Baldwin