[4/7] Fix int overflow

---
 libiberty/cp-demangle.c               | 3 ++-
 libiberty/testsuite/demangle-expected | 4 ++++
 2 files changed, 6 insertions(+), 1 deletion(-)

On 07/06/2015 01:36 PM, Mikhail Maltsev wrote:
> ---
>   libiberty/cp-demangle.c               | 3 ++-
>   libiberty/testsuite/demangle-expected | 4 ++++
>   2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/libiberty/cp-demangle.c b/libiberty/cp-demangle.c
> index 44a0a9b..befa6b6 100644
> --- a/libiberty/cp-demangle.c
> +++ b/libiberty/cp-demangle.c
> @@ -103,6 +103,7 @@
>   #include "config.h"
>   #endif
>
> +#include <limits.h>
>   #include <stdio.h>
>
>   #ifdef HAVE_STDLIB_H
> @@ -1599,7 +1600,7 @@ d_source_name (struct d_info *di)
>     struct demangle_component *ret;
>
>     len = d_number (di);
> -  if (len <= 0)
> +  if (len <= 0 || len > INT_MAX)
>       return NULL;
>     ret = d_identifier (di, len);
>     di->last_name = ret;
Isn't this only helpful if sizeof (long) > sizeof (int)?  Otherwise the 
compiler is going to eliminate that newly added test, right?

So with that in mind, what happens on i686-unknown-linux with this test?


Jeff

On 07.07.2015 1:55, Jeff Law wrote:

>>     len = d_number (di);
>> -  if (len <= 0)
>> +  if (len <= 0 || len > INT_MAX)
>>       return NULL;
>>     ret = d_identifier (di, len);
>>     di->last_name = ret;
> Isn't this only helpful if sizeof (long) > sizeof (int)?  Otherwise the
> compiler is going to eliminate that newly added test, right?
> 
> So with that in mind, what happens on i686-unknown-linux with this test?
> 
> 
> Jeff
> 

Probably it should be fine, because the problem occurred when len became
negative after implicit conversion to int (d_identifier does not check
for negative length, but it does check that length does not exceed total
string length). In this case (i.e. on ILP32 targets) len will not change
sign after conversion to int (because it's a no-op).
I'm not completely sure about compiler warnings, but AFAIR, in multilib
build libiberty is also built for 32-bit target, and I did not get any
additional warnings.

On 07/06/2015 06:04 PM, Mikhail Maltsev wrote:
> On 07.07.2015 1:55, Jeff Law wrote:
>
>>>      len = d_number (di);
>>> -  if (len <= 0)
>>> +  if (len <= 0 || len > INT_MAX)
>>>        return NULL;
>>>      ret = d_identifier (di, len);
>>>      di->last_name = ret;
>> Isn't this only helpful if sizeof (long) > sizeof (int)?  Otherwise the
>> compiler is going to eliminate that newly added test, right?
>>
>> So with that in mind, what happens on i686-unknown-linux with this test?
>>
>>
>> Jeff
>>
>
> Probably it should be fine, because the problem occurred when len became
> negative after implicit conversion to int (d_identifier does not check
> for negative length, but it does check that length does not exceed total
> string length). In this case (i.e. on ILP32 targets) len will not change
> sign after conversion to int (because it's a no-op).
> I'm not completely sure about compiler warnings, but AFAIR, in multilib
> build libiberty is also built for 32-bit target, and I did not get any
> additional warnings.
You may need -Wtype-limits to see the warning.

I'm not questioning whether or not the test will cause a problem, but 
instead questioning if the test does what you expect it to do on a 32bit 
host.

On a host where sizeof (int) == sizeof (long), that len > INT_MAX test 
is always going to be false.

If you want to do overflow testing, you have to compute len in a wider 
type.  You might consider using "long long" or "int64_t" depending on 
the outcome of a configure test.  Falling back to a simple "long" if the 
host compiler doesn't have "long long" or "int64_t".

Interesting exercise feeding those tests into demangle.com  :-0  A 
suitably interested party might be able to exploit that overflow.


jeff

On 08.07.2015 1:48, Jeff Law wrote:
> I'm not questioning whether or not the test will cause a problem, but
> instead questioning if the test does what you expect it to do on a 32bit
> host.
> 
> On a host where sizeof (int) == sizeof (long), that len > INT_MAX test
> is always going to be false.
Yes, but in this case the call "d_identifier (di, len)" is also safe,
because implicit conversion from long (the type of len variable) to int
(the type of d_identifier's second argument) will never cause overflow.
I first wanted to change one of those types to match the other, but it
turned out that they are used rather consistently: all the code that
works with demangle_component.u.s_number.number uses type long (because
strtol was used for string-to-integer conversion before r73788), and
ints are used for lengths of various identifiers. Should I try to
refactor it somehow?

> If you want to do overflow testing, you have to compute len in a wider
> type.  You might consider using "long long" or "int64_t" depending on
> the outcome of a configure test.  Falling back to a simple "long" if the
> host compiler doesn't have "long long" or "int64_t".
So, it's probably vice-versa: the test is only needed if long is wider
than int.

> And a generic question on the testsuite -- presumably it turns on type demangling?
> I wanted to verify the flow through d_expression_1 was what I expected
> it to be and it took a while to realize that c++filt doesn't demangle types by
> default, thus Av32_f would demangle to Av32_f without ever getting into d_expression_1. 
Yes, the testsuite is based on libiberty/testsuite/test-demangle.c (OMG,
there are still K&R-style definitions there), and it uses (DMGL_PARAMS |
DMGL_ANSI | DMGL_TYPES) by default.

(snip)
> FWIW, demangler.com doesn't give any results for that case.  It just returns DpDv1_c 
But "_Z1fDpDv1_c" makes it crash :)

On Mon, Jul 6, 2015 at 12:36 PM, Mikhail Maltsev <maltsevm@gmail.com> wrote:
>
> diff --git a/libiberty/cp-demangle.c b/libiberty/cp-demangle.c
> index 44a0a9b..befa6b6 100644
> --- a/libiberty/cp-demangle.c
> +++ b/libiberty/cp-demangle.c
> @@ -103,6 +103,7 @@
>  #include "config.h"
>  #endif
>
> +#include <limits.h>

All existing uses of limits.h in libiberty are inside #ifdef
HAVE_LIMITS_H.  See other files in the directory.


> @@ -1599,7 +1600,7 @@ d_source_name (struct d_info *di)
>    struct demangle_component *ret;
>
>    len = d_number (di);
> -  if (len <= 0)
> +  if (len <= 0 || len > INT_MAX)
>      return NULL;

This is not, in my opinion, the best way to write this kind of thing.
Instead, write something like

    int ilen;


    ilen = (int) len:
    if ((long) ilen != len)
      return NULL;


But better still is to consider the larger context.  We want the
demangler to work the same on all hosts, if at all possible.
d_identifier is called exactly once.  Change it to take a parameter of
type long.  Don't worry about changing d_source_name.

Then look at the fact that d_number does not check for overflow.  We
should consider changing d_number to limit itself to 32-bit integers,
and to return an error indication on overflow.  From a quick glance I
don't see any need for the demangler to support numbers larger than 32
bits.  I think it's OK if we fail to demangle symbol names that are
more than 2 billion characters long.

Ian