fix unsigned overflow in charset.c

  This fixed an overflow in pointer arithmetic that crashes GDB on Mac OS.

Ok for trunk?

	paul

gdb/ChangeLog:

2018-10-09  Paul Koning  <paul_koning@dell.com>

	* charset.c (convert_between_encodings): Fix unsigned overflow.
  

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


> 
> Ok for trunk?
> 
> 	paul
> 
> gdb/ChangeLog:
> 
> 2018-10-09  Paul Koning  <paul_koning@dell.com>
> 
> 	* charset.c (convert_between_encodings): Fix unsigned overflow.
> 
> diff --git a/gdb/charset.c b/gdb/charset.c
> index 8bb2b4d669..64757ab279 100644
> --- a/gdb/charset.c
> +++ b/gdb/charset.c
> @@ -548,7 +548,7 @@ convert_between_encodings (const char *from, const char *to,
>  
>        /* Now make sure that the object on the obstack only includes
>  	 bytes we have converted.  */
> -      obstack_blank_fast (output, -outleft);
> +      obstack_blank_fast (output, -(ssize_t) outleft);
>  
>        if (r == (size_t) -1)
>  	{
>
  

> 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.

	paul
  

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.
  

> 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?

	paul
  

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.
  

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

Looks like some of the "int"-ness was lost with the obstack v2 changes
a while ago, to support larger (64-bit) objects.

If I diff my system's obstack.h with libiberty's local copy, I see:

(This is Fedora 27, a little outdated wrt to glibc in use by now.
 Upstream glibc's obstack.h is in sync with liberty's IIRC.)

$ diff -upw /usr/include/obstack.h obstack.h | less

-#ifdef __PTRDIFF_TYPE__
-# define PTR_INT_TYPE __PTRDIFF_TYPE__
+#if _OBSTACK_INTERFACE_VERSION == 1
+/* For binary compatibility with obstack version 1, which used "int"
+   and "long" for these two types.  */
+# define _OBSTACK_SIZE_T unsigned int
+# define _CHUNK_SIZE_T unsigned long
+# define _OBSTACK_CAST(type, expr) ((type) (expr))
 #else
-# include <stddef.h>
-# define PTR_INT_TYPE ptrdiff_t
+/* Version 2 with sane types, especially for 64-bit hosts.  */
+# define _OBSTACK_SIZE_T size_t
+# define _CHUNK_SIZE_T size_t
+# define _OBSTACK_CAST(type, expr) (expr)
 #endif


and 

@@ -359,11 +375,10 @@ extern int obstack_exit_failure;
 # define obstack_blank(OBSTACK, length)                                              \
   __extension__                                                                      \
     ({ struct obstack *__o = (OBSTACK);                                              \
-       int __len = (length);                                                 \
-       if (__o->chunk_limit - __o->next_free < __len)                        \
+       _OBSTACK_SIZE_T __len = (length);                                     \
+       if (obstack_room (__o) < __len)                                       \
         _obstack_newchunk (__o, __len);                                      \
-       obstack_blank_fast (__o, __len);                                              \
-       (void) 0; })
+       obstack_blank_fast (__o, __len); })


Note how above we used to have "int __len = (length);"

But that's obstack_blank, not obstack_blank_fast.  The latter
never had a cast.

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.

Thanks,
Pedro Alves
  

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.
  

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 <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.

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.

Thanks,
Pedro Alves
  

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 <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.
> 
> 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?
  

> On Oct 17, 2018, at 2:38 PM, John Baldwin <jhb@FreeBSD.org> wrote:
> 
>> ...
>> 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

Please let me know; I can commit that patch if it is approved.  Or commit with changes if that's desired.

	paul
  

On 10/17/2018 07:47 PM, Paul Koning wrote:
> 
> 
>> On Oct 17, 2018, at 2:38 PM, John Baldwin <jhb@FreeBSD.org> wrote:
>>
>>> ...
>>> 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
> 
> Please let me know; I can commit that patch if it is approved.  Or commit with changes if that's desired.

Patch is OK as is.  Please push.

Thanks,
Pedro Alves
  

> On Oct 17, 2018, at 5:51 PM, Pedro Alves <palves@redhat.com> wrote:
> 
> On 10/17/2018 07:47 PM, Paul Koning wrote:
>> 
>> 
>>> On Oct 17, 2018, at 2:38 PM, John Baldwin <jhb@FreeBSD.org> wrote:
>>> 
>>>> ...
>>>> 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
>> 
>> Please let me know; I can commit that patch if it is approved.  Or commit with changes if that's desired.
> 
> Patch is OK as is.  Please push.
> 
> Thanks,
> Pedro Alves

Thank you.  Done.  

	paul