Patchwork fix unsigned overflow in charset.c

login
register
mail settings
Submitter Paul Koning
Date Oct. 9, 2018, 5:19 p.m.
Message ID <7B48D309-445E-4141-A87A-1F3D5FA70EFD@comcast.net>
Download mbox | patch
Permalink /patch/29681/
State New
Headers show

Comments

Paul Koning - Oct. 9, 2018, 5:19 p.m.
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.
Pedro Alves - Oct. 9, 2018, 5:31 p.m.
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)
>  	{
>
Paul Koning - Oct. 9, 2018, 5:40 p.m.
> 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
John Baldwin - Oct. 9, 2018, 5:57 p.m.
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.
Paul Koning - Oct. 9, 2018, 6:10 p.m.
> 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
John Baldwin - Oct. 9, 2018, 7:58 p.m.
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.
Pedro Alves - Oct. 10, 2018, 8:50 a.m.
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
John Baldwin - Oct. 11, 2018, 8:15 p.m.
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.

Patch

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)
 	{