Fix for PR70498 in Libiberty Demangler

> On 2 Apr 2016, at 1:44 AM, Bernd Schmidt <bschmidt@redhat.com> wrote:
> 
> On 04/01/2016 07:41 PM, Pedro Alves wrote:
>> On 04/01/2016 11:21 AM, Marcel Böhme wrote:
>>>  static inline void
>>> -d_append_num (struct d_print_info *dpi, long l)
>>> +d_append_num (struct d_print_info *dpi, int l)
>>>  {
>>>    char buf[25];
>>>    sprintf (buf,"%ld", l);
>> 
>> Doesn't this warn about %ld format vs int type mismatch?
> 
> Well spotted. Marcel, please correct this issue and check for other warnings. Unless libiberty is somehow exempt from -Werror, this should have shown up in a bootstrap.

Bootstrapped and regression tested on x86_64-pc-linux-gnu. Test cases added + checked PR70498 is resolved. 

The patch now also accounts for overflows in d_compact_number which is supposed to return -1 in case of negative numbers.

Thanks,
- Marcel

--

On 04/02/2016 11:49 AM, Marcel Böhme wrote:
>
> Bootstrapped and regression tested on x86_64-pc-linux-gnu. Test cases
> added + checked PR70498 is resolved.

Richard - any objections from an RM perspective to check in such
potentially security-related fixes now even if not regressions?

> The patch now also accounts for overflows in d_compact_number which
> is supposed to return -1 in case of negative numbers.

I take it this isn't for the normal 'n' case, but for instances where we 
encounter overflows in d_number?


Bernd

> On 4 Apr 2016, at 9:24 PM, Bernd Schmidt <bschmidt@redhat.com> wrote:
> 
>> 
>> The patch now also accounts for overflows in d_compact_number which
>> is supposed to return -1 in case of negative numbers.
> 
> I take it this isn't for the normal 'n' case, but for instances where we encounter overflows in d_number?
Yes.

Best regards,
- Marcel

I've been looking through this patch. I had intended to commit it, but 
after looking through it a little more carefully I think there are a few 
things left to solve.

So, d_number/d_compact_number now return ints rather than longs, which 
makes sense since the lengths in things like struct demangle_component's 
s_name are integers. However, s_number there is defined as a long, so 
this does mean a tighter limit for things like 
d_template_param/d_make_template_param. Cc'ing Jason for an opinion on 
whether that's a problem or not (I suspect it isn't - t).

> -static long
> +static int
>   d_compact_number (struct d_info *di)
>   {
> -  long num;
> +  int num;
>     if (d_peek_char (di) == '_')
>       num = 0;
>     else if (d_peek_char (di) == 'n')
> @@ -2957,7 +2957,7 @@ d_compact_number (struct d_info *di)
>     else
>       num = d_number (di) + 1;
>
> -  if (! d_check_char (di, '_'))
> +  if (num < 0 || ! d_check_char (di, '_'))
>       return -1;
>     return num;
>   }

Shouldn't we check for overflows before performing the +1 addition (i.e. 
0 <= num < INT_MAX)? Ideally we'd also have a way to signal from 
d_number if we had an overflow while parsing that number.

There's also this, in d_expression_1:

	  index = d_compact_number (di) + 1;
	  if (index == 0)
	    return NULL;

which probably ought to have the same kind of check (I'll note that at 
this point we've accumulated two "+1"s, I'll assume that's what we want).

Please include a ChangeLog entry with the next patch.


Bernd

Hi Bernd,

> -static long
> +static int
>  d_compact_number (struct d_info *di)
>  {
> -  long num;
> +  int num;
>    if (d_peek_char (di) == '_')
>      num = 0;
>    else if (d_peek_char (di) == 'n')
> @@ -2957,7 +2957,7 @@ d_compact_number (struct d_info *di)
>    else
>      num = d_number (di) + 1;
> 
> -  if (! d_check_char (di, '_'))
> +  if (num < 0 || ! d_check_char (di, '_'))
>      return -1;
>    return num;
>  }

> Shouldn't we check for overflows before performing the +1 addition (i.e. 0 <= num < INT_MAX)? Ideally we'd also have a way to signal from d_number if we had an overflow while parsing that number.
Without an overflow signal, d_number will already be prone to return a negative number for supposedly non-negative numbers (those not preceded with ’n’). In that case an overflow check would be unnecessary in d_compact_number which is supposed to always return a positive number or a negative one (-1). If you decide in favour of an overflow signal, it must be handled by the call-sites. Not sure what the “default behaviour” should be then. Otherwise, we can simply assume that the call sites for d_number can handle negative numbers.

Kindly advice.

> 
> There's also this, in d_expression_1:
> 
> 	  index = d_compact_number (di) + 1;
> 	  if (index == 0)
> 	    return NULL;
> 
> which probably ought to have the same kind of check (I'll note that at this point we've accumulated two "+1"s, I'll assume that's what we want).
Yes. There should be an overflow check here. 

Thanks!
- Marcel

On 04/13/2016 03:04 PM, Marcel Böhme wrote:
> Hi Bernd,
>> Shouldn't we check for overflows before performing the +1 addition
>> (i.e. 0 <= num < INT_MAX)? Ideally we'd also have a way to signal
>> from d_number if we had an overflow while parsing that number.
> Without an overflow signal, d_number will already be prone to return
> a negative number for supposedly non-negative numbers (those not
> preceded with ’n’). In that case an overflow check would be
> unnecessary in d_compact_number which is supposed to always return a
> positive number or a negative one (-1). If you decide in favour of an
> overflow signal, it must be handled by the call-sites. Not sure what
> the “default behaviour” should be then. Otherwise, we can simply
> assume that the call sites for d_number can handle negative numbers.

Shouldn't we look into fixing d_number eventually so it can signal error?

>> index = d_compact_number (di) + 1; if (index == 0) return NULL;
>>
>> which probably ought to have the same kind of check (I'll note that
>> at this point we've accumulated two "+1"s, I'll assume that's what
>> we want).
> Yes. There should be an overflow check here.

Could you update the patch for that?


Bernd