Re: Fix for PR70498 in Libiberty Demangler

[Bernd Schmidt]

On 05/01/2016 06:24 PM, Marcel Böhme wrote:


Please attach it (text/plain) instead.

Done.


That still seemed to be inlined, but I managed to apply this version. 
Committed.



Bernd

Re: Fix for PR70498 in Libiberty Demangler

[Marcel Böhme]

> On 28 Apr 2016, at 12:28 AM, Bernd Schmidt  wrote:
> 
> Please attach it (text/plain) instead.
Done.


Index: libiberty/ChangeLog
===
--- libiberty/ChangeLog (revision 235691)
+++ libiberty/ChangeLog (working copy)
@@ -1,3 +1,22 @@
+2016-04-16  Marcel Böhme  
+
+   PR c++/70498
+   * cp-demangle.c: Parse numbers as integer instead of long to avoid
+   overflow after sanity checks. Include  if available.
+   (INT_MAX): Define if necessary.
+   (d_make_template_param): Takes integer argument instead of long.
+   (d_make_function_param): Likewise.
+   (d_append_num): Likewise.
+   (d_identifier): Likewise.
+   (d_number): Parse as and return integer.
+   (d_compact_number): Handle overflow.
+   (d_source_name): Change variable type to integer for parsed number.
+   (d_java_resource): Likewise.
+   (d_special_name): Likewise.
+   (d_discriminator): Likewise.
+   (d_unnamed_type): Likewise.
+   * testsuite/demangle-expected: Add regression test cases.   
+
 2016-04-30  Oleg Endo  
 
* configure: Remove SH5 support.
Index: libiberty/cp-demangle.c
===
--- libiberty/cp-demangle.c (revision 235691)
+++ libiberty/cp-demangle.c (working copy)
@@ -128,6 +128,13 @@
 # endif /* alloca */
 #endif /* HAVE_ALLOCA_H */
 
+#ifdef HAVE_LIMITS_H
+#include 
+#endif
+#ifndef INT_MAX
+# define INT_MAX   (int)(((unsigned int) ~0) >> 1)  /* 0x7FFF 
*/ 
+#endif
+
 #include "ansidecl.h"
 #include "libiberty.h"
 #include "demangle.h"
@@ -398,7 +405,7 @@
  struct demangle_component *);
 
 static struct demangle_component *
-d_make_template_param (struct d_info *, long);
+d_make_template_param (struct d_info *, int);
 
 static struct demangle_component *
 d_make_sub (struct d_info *, const char *, int);
@@ -421,9 +428,9 @@
 
 static struct demangle_component *d_source_name (struct d_info *);
 
-static long d_number (struct d_info *);
+static int d_number (struct d_info *);
 
-static struct demangle_component *d_identifier (struct d_info *, long);
+static struct demangle_component *d_identifier (struct d_info *, int);
 
 static struct demangle_component *d_operator_name (struct d_info *);
 
@@ -1119,7 +1126,7 @@
 /* Add a new template parameter.  */
 
 static struct demangle_component *
-d_make_template_param (struct d_info *di, long i)
+d_make_template_param (struct d_info *di, int i)
 {
   struct demangle_component *p;
 
@@ -1135,7 +1142,7 @@
 /* Add a new function parameter.  */
 
 static struct demangle_component *
-d_make_function_param (struct d_info *di, long i)
+d_make_function_param (struct d_info *di, int i)
 {
   struct demangle_component *p;
 
@@ -1620,7 +1627,7 @@
 static struct demangle_component *
 d_source_name (struct d_info *di)
 {
-  long len;
+  int len;
   struct demangle_component *ret;
 
   len = d_number (di);
@@ -1633,12 +1640,12 @@
 
 /* number ::= [n] <(non-negative decimal integer)>  */
 
-static long
+static int
 d_number (struct d_info *di)
 {
   int negative;
   char peek;
-  long ret;
+  int ret;
 
   negative = 0;
   peek = d_peek_char (di);
@@ -1681,7 +1688,7 @@
 /* identifier ::= <(unqualified source code identifier)>  */
 
 static struct demangle_component *
-d_identifier (struct d_info *di, long len)
+d_identifier (struct d_info *di, int len)
 {
   const char *name;
 
@@ -1702,7 +1709,7 @@
   /* Look for something which looks like a gcc encoding of an
  anonymous namespace, and replace it with a more user friendly
  name.  */
-  if (len >= (long) ANONYMOUS_NAMESPACE_PREFIX_LEN + 2
+  if (len >= (int) ANONYMOUS_NAMESPACE_PREFIX_LEN + 2
   && memcmp (name, ANONYMOUS_NAMESPACE_PREFIX,
 ANONYMOUS_NAMESPACE_PREFIX_LEN) == 0)
 {
@@ -1870,7 +1877,7 @@
 {
   struct demangle_component *p = NULL;
   struct demangle_component *next = NULL;
-  long len, i;
+  int len, i;
   char c;
   const char *str;
 
@@ -2012,7 +2019,7 @@
case 'C':
  {
struct demangle_component *derived_type;
-   long offset;
+   int offset;
struct demangle_component *base_type;
 
derived_type = cplus_demangle_type (di);
@@ -2946,10 +2953,10 @@
 
 /*  _ */
 
-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 +2964,7 @@
   else
 num = d_number (di) + 1;
 
-  if (! d_check_char (di, '_'))
+  if (num < 0 || ! d_check_char (di, '_'))
 return -1;
   return num;
 }
@@ -2969,7 +2976,7 @@
 static struct demangle_component *
 d_template_param (struct d_info *di)
 {
-  long param;
+  int param;
 
   if (! d_check_char (di, 'T'))
 return NULL;
@@ -3171,9 +3178,10 @@
}
   else
{
- 

Re: Fix for PR70498 in Libiberty Demangler

[Bernd Schmidt]

On 04/15/2016 07:39 PM, Marcel Böhme wrote:


Sure. The updated patch, including Changelog and more test cases. Regression 
tested.


This patch seems seriously damaged by sending it through the email body. 
Please attach it (text/plain) instead.



Bernd

Re: Fix for PR70498 in Libiberty Demangler

[Marcel Böhme]

Hi Bernd,

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

Sure. The updated patch, including Changelog and more test cases. Regression 
tested.

Best regards,
- Marcel

Index: ChangeLog
===
--- ChangeLog   (revision 235032)
+++ ChangeLog   (working copy)
@@ -1,3 +1,22 @@
+2016-04-16  Marcel Böhme  
+
+   PR c++/70498
+   * cp-demangle.c: Parse/handle numbers as integer instead of long.
+   Include  if available.
+   (INT_MAX): Define if necessary.
+   (d_make_template_param): Takes integer argument instead of long.
+   (d_make_function_param): Likewise.
+   (d_append_num): Likewise.
+   (d_identifier): Likewise.
+   (d_number): Parse as and return as integer.
+   (d_compact_number): Handle overflow.
+   (d_expression_1): Likewise.
+   (d_source_name): Change variable type to integer for parsed number.
+   (d_java_resource): Likewise.
+   (d_special_name): Likewise.
+   (d_discriminator): Likewise.
+   (d_unnamed_type): Likewise.
+   * testsuite/demangle-expected: Add regression test cases.   
+
 2016-04-08  Marcel Böhme  
 
PR c++/69687
Index: cp-demangle.c
===
--- cp-demangle.c   (revision 235032)
+++ cp-demangle.c   (working copy)
@@ -128,6 +128,13 @@ extern char *alloca ();
 # endif /* alloca */
 #endif /* HAVE_ALLOCA_H */
 
+#ifdef HAVE_LIMITS_H
+#include 
+#endif
+#ifndef INT_MAX
+# define INT_MAX   (int)(((unsigned int) ~0) >> 1)  /* 0x7FFF 
*/ 
+#endif
+
 #include "ansidecl.h"
 #include "libiberty.h"
 #include "demangle.h"
@@ -398,7 +405,7 @@ d_make_dtor (struct d_info *, enum gnu_v
  struct demangle_component *);
 
 static struct demangle_component *
-d_make_template_param (struct d_info *, long);
+d_make_template_param (struct d_info *, int);
 
 static struct demangle_component *
 d_make_sub (struct d_info *, const char *, int);
@@ -421,9 +428,9 @@ static struct demangle_component *d_unqu
 
 static struct demangle_component *d_source_name (struct d_info *);
 
-static long d_number (struct d_info *);
+static int d_number (struct d_info *);
 
-static struct demangle_component *d_identifier (struct d_info *, long);
+static struct demangle_component *d_identifier (struct d_info *, int);
 
 static struct demangle_component *d_operator_name (struct d_info *);
 
@@ -1119,7 +1126,7 @@ d_make_dtor (struct d_info *di, enum gnu
 /* Add a new template parameter.  */
 
 static struct demangle_component *
-d_make_template_param (struct d_info *di, long i)
+d_make_template_param (struct d_info *di, int i)
 {
   struct demangle_component *p;
 
@@ -1135,7 +1142,7 @@ d_make_template_param (struct d_info *di
 /* Add a new function parameter.  */
 
 static struct demangle_component *
-d_make_function_param (struct d_info *di, long i)
+d_make_function_param (struct d_info *di, int i)
 {
   struct demangle_component *p;
 
@@ -1620,7 +1627,7 @@ d_unqualified_name (struct d_info *di)
 static struct demangle_component *
 d_source_name (struct d_info *di)
 {
-  long len;
+  int len;
   struct demangle_component *ret;
 
   len = d_number (di);
@@ -1633,12 +1640,12 @@ d_source_name (struct d_info *di)
 
 /* number ::= [n] <(non-negative decimal integer)>  */
 
-static long
+static int
 d_number (struct d_info *di)
 {
   int negative;
   char peek;
-  long ret;
+  int ret;
 
   negative = 0;
   peek = d_peek_char (di);
@@ -1681,7 +1688,7 @@ d_number_component (struct d_info *di)
 /* identifier ::= <(unqualified source code identifier)>  */
 
 static struct demangle_component *
-d_identifier (struct d_info *di, long len)
+d_identifier (struct d_info *di, int len)
 {
   const char *name;
 
@@ -1702,7 +1709,7 @@ d_identifier (struct d_info *di, long le
   /* Look for something which looks like a gcc encoding of an
  anonymous namespace, and replace it with a more user friendly
  name.  */
-  if (len >= (long) ANONYMOUS_NAMESPACE_PREFIX_LEN + 2
+  if (len >= (int) ANONYMOUS_NAMESPACE_PREFIX_LEN + 2
   && memcmp (name, ANONYMOUS_NAMESPACE_PREFIX,
 ANONYMOUS_NAMESPACE_PREFIX_LEN) == 0)
 {
@@ -1870,7 +1877,7 @@ d_java_resource (struct d_info *di)
 {
   struct demangle_component *p = NULL;
   struct demangle_component *next = NULL;
-  long len, i;
+  int len, i;
   char c;
   const char *str;
 
@@ -2012,7 +2019,7 @@ d_special_name (struct d_info *di)
case 'C':
  {
struct demangle_component *derived_type;
-   long offset;
+   int offset;
struct demangle_component *base_type;
 
  

Re: Fix for PR70498 in Libiberty Demangler

[Bernd Schmidt]

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

Re: Fix for PR70498 in Libiberty Demangler

[Marcel Böhme]

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

Re: Fix for PR70498 in Libiberty Demangler

[Bernd Schmidt]

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

Re: Fix for PR70498 in Libiberty Demangler

[Marcel Böhme]

> On 4 Apr 2016, at 9:24 PM, Bernd Schmidt  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

Re: Fix for PR70498 in Libiberty Demangler

[Bernd Schmidt]

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

Re: Fix for PR70498 in Libiberty Demangler

[Marcel Böhme]

> On 2 Apr 2016, at 1:44 AM, Bernd Schmidt  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

--
Index: libiberty/cp-demangle.c
===
--- libiberty/cp-demangle.c (revision 234663)
+++ libiberty/cp-demangle.c (working copy)
@@ -398,7 +398,7 @@ d_make_dtor (struct d_info *, enum gnu_v
  struct demangle_component *);
 
 static struct demangle_component *
-d_make_template_param (struct d_info *, long);
+d_make_template_param (struct d_info *, int);
 
 static struct demangle_component *
 d_make_sub (struct d_info *, const char *, int);
@@ -421,9 +421,9 @@ static struct demangle_component *d_unqu
 
 static struct demangle_component *d_source_name (struct d_info *);
 
-static long d_number (struct d_info *);
+static int d_number (struct d_info *);
 
-static struct demangle_component *d_identifier (struct d_info *, long);
+static struct demangle_component *d_identifier (struct d_info *, int);
 
 static struct demangle_component *d_operator_name (struct d_info *);
 
@@ -1119,7 +1119,7 @@ d_make_dtor (struct d_info *di, enum gnu
 /* Add a new template parameter.  */
 
 static struct demangle_component *
-d_make_template_param (struct d_info *di, long i)
+d_make_template_param (struct d_info *di, int i)
 {
   struct demangle_component *p;
 
@@ -1135,7 +1135,7 @@ d_make_template_param (struct d_info *di
 /* Add a new function parameter.  */
 
 static struct demangle_component *
-d_make_function_param (struct d_info *di, long i)
+d_make_function_param (struct d_info *di, int i)
 {
   struct demangle_component *p;
 
@@ -1620,7 +1620,7 @@ d_unqualified_name (struct d_info *di)
 static struct demangle_component *
 d_source_name (struct d_info *di)
 {
-  long len;
+  int len;
   struct demangle_component *ret;
 
   len = d_number (di);
@@ -1633,12 +1633,12 @@ d_source_name (struct d_info *di)
 
 /* number ::= [n] <(non-negative decimal integer)>  */
 
-static long
+static int
 d_number (struct d_info *di)
 {
   int negative;
   char peek;
-  long ret;
+  int ret;
 
   negative = 0;
   peek = d_peek_char (di);
@@ -1681,7 +1681,7 @@ d_number_component (struct d_info *di)
 /* identifier ::= <(unqualified source code identifier)>  */
 
 static struct demangle_component *
-d_identifier (struct d_info *di, long len)
+d_identifier (struct d_info *di, int len)
 {
   const char *name;
 
@@ -1702,7 +1702,7 @@ d_identifier (struct d_info *di, long le
   /* Look for something which looks like a gcc encoding of an
  anonymous namespace, and replace it with a more user friendly
  name.  */
-  if (len >= (long) ANONYMOUS_NAMESPACE_PREFIX_LEN + 2
+  if (len >= (int) ANONYMOUS_NAMESPACE_PREFIX_LEN + 2
   && memcmp (name, ANONYMOUS_NAMESPACE_PREFIX,
 ANONYMOUS_NAMESPACE_PREFIX_LEN) == 0)
 {
@@ -1870,7 +1870,7 @@ d_java_resource (struct d_info *di)
 {
   struct demangle_component *p = NULL;
   struct demangle_component *next = NULL;
-  long len, i;
+  int len, i;
   char c;
   const char *str;
 
@@ -2012,7 +2012,7 @@ d_special_name (struct d_info *di)
case 'C':
  {
struct demangle_component *derived_type;
-   long offset;
+   int offset;
struct demangle_component *base_type;
 
derived_type = cplus_demangle_type (di);
@@ -2946,10 +2946,10 @@ d_pointer_to_member_type (struct d_info
 
 /*  _ */
 
-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;
 }
@@ -2969,7 +2969,7 @@ d_compact_number (struct d_info *di)
 static struct demangle_component *
 d_template_param (struct d_info *di)
 {
-  long param;
+  int param;
 
   if (! d_check_char (di, 'T'))
 return NULL;
@@ -3502,7 +3502,7 @@ d_local_name (struct d_info *di)
 static int
 d_discriminator (struct d_info *di)
 {
-  long discrim;
+  int discrim;
 
   if (d_peek_char (di) != '_')
 return 1;
@@ -3558,7 +3558,7 @@ static struct 

Re: Fix for PR70498 in Libiberty Demangler

[Bernd Schmidt]

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.



Bernd

Re: Fix for PR70498 in Libiberty Demangler

[Pedro Alves]

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?

Thanks,
Pedro Alves

Re: Fix for PR70498 in Libiberty Demangler

[Bernd Schmidt]

On 04/01/2016 03:39 PM, Marcel Böhme wrote:

Hi Bernd,

Thanks for the feedback!


Patches need to be bootstrapped and regression tested, and patch
submissions should include which target this was done on.

Ideally you'd also want to include testcases along with your
patches, although I'm not entirely sure how we can arrange for this
type of problem to be tested.

Regression tested on x86_64-pc-linux-gnu (make check). Test cases
added to libiberty/testsuite/demangler-expected and checked PR70498
is resolved. Not sure how to bootstrap the patch.


You configure gcc normally and build it - that should automatically 
bootstrap, unless you're cross-compiling. You'll have stage1-* and 
stage2-* directories at the end if that worked. You should then run the 
testsuite on the bootstrapped compiler.



Lastly, for this specific patch, I have trouble seeing how it fixes
anything. I'd need a more detailed explanation of how the problem
happens in the first place.

In the patched version, the values wrap around when they are parsed
in d_number. Since the mangled string may contain negative numbers,
there is usually proper handling of negative numbers in the clients
of d_number. Without the patch a value can become negative when cast
from long to int *after* these checks.

For instance, in d_source_name the length of the identifier is parsed
as long from the mangled string and checked whether it is negative.
Since d_identifier takes an int as length, d_identifier is called
with a negative length after the implicit cast:


Ok, I think I see it. Guess I'll queue this up and commit it for you in 
the next few days.



Bernd

Re: Fix for PR70498 in Libiberty Demangler

[Marcel Böhme]

> Since d_identifier takes an int as length, d_identifier is called with a 
> negative length after the implicit cast:
Sorry, d_make_name called from d_identifier in cp_demangle.c:1721 takes an int 
as length.

Best regards
- Marcel

Re: Fix for PR70498 in Libiberty Demangler

[Marcel Böhme]

Hi Bernd,

Thanks for the feedback!

> Patches need to be bootstrapped and regression tested, and patch submissions 
> should include which target this was done on.
> 
> Ideally you'd also want to include testcases along with your patches, 
> although I'm not entirely sure how we can arrange for this type of problem to 
> be tested.
Regression tested on x86_64-pc-linux-gnu (make check). Test cases added to 
libiberty/testsuite/demangler-expected and checked PR70498 is resolved. Not 
sure how to bootstrap the patch.

> Lastly, for this specific patch, I have trouble seeing how it fixes anything. 
> I'd need a more detailed explanation of how the problem happens in the first 
> place.
In the patched version, the values wrap around when they are parsed in 
d_number. Since the mangled string may contain negative numbers, there is 
usually proper handling of negative numbers in the clients of d_number. Without 
the patch a value can become negative when cast from long to int *after* these 
checks. 

For instance, in d_source_name the length of the identifier is parsed as long 
from the mangled string and checked whether it is negative. Since d_identifier 
takes an int as length, d_identifier is called with a negative length after the 
implicit cast:

static struct demangle_component *
d_source_name (struct d_info *di)
{
  int len;
  struct demangle_component *ret;

  len = d_number (di);
  if (len <= 0)
return NULL;
  ret = d_identifier (di, len);
  di->last_name = ret;
  return ret;
}

--

Index: libiberty/cp-demangle.c
===
--- libiberty/cp-demangle.c (revision 234663)
+++ libiberty/cp-demangle.c (working copy)
@@ -398,7 +398,7 @@
 struct demangle_component *);

static struct demangle_component *
-d_make_template_param (struct d_info *, long);
+d_make_template_param (struct d_info *, int);

static struct demangle_component *
d_make_sub (struct d_info *, const char *, int);
@@ -421,9 +421,9 @@

static struct demangle_component *d_source_name (struct d_info *);

-static long d_number (struct d_info *);
+static int d_number (struct d_info *);

-static struct demangle_component *d_identifier (struct d_info *, long);
+static struct demangle_component *d_identifier (struct d_info *, int);

static struct demangle_component *d_operator_name (struct d_info *);

@@ -1119,7 +1119,7 @@
/* Add a new template parameter.  */

static struct demangle_component *
-d_make_template_param (struct d_info *di, long i)
+d_make_template_param (struct d_info *di, int i)
{
  struct demangle_component *p;

@@ -1135,7 +1135,7 @@
/* Add a new function parameter.  */

static struct demangle_component *
-d_make_function_param (struct d_info *di, long i)
+d_make_function_param (struct d_info *di, int i)
{
  struct demangle_component *p;

@@ -1620,7 +1620,7 @@
static struct demangle_component *
d_source_name (struct d_info *di)
{
-  long len;
+  int len;
  struct demangle_component *ret;

  len = d_number (di);
@@ -1633,12 +1633,12 @@

/* number ::= [n] <(non-negative decimal integer)>  */

-static long
+static int
d_number (struct d_info *di)
{
  int negative;
  char peek;
-  long ret;
+  int ret;

  negative = 0;
  peek = d_peek_char (di);
@@ -1681,7 +1681,7 @@
/* identifier ::= <(unqualified source code identifier)>  */

static struct demangle_component *
-d_identifier (struct d_info *di, long len)
+d_identifier (struct d_info *di, int len)
{
  const char *name;

@@ -1702,7 +1702,7 @@
  /* Look for something which looks like a gcc encoding of an
 anonymous namespace, and replace it with a more user friendly
 name.  */
-  if (len >= (long) ANONYMOUS_NAMESPACE_PREFIX_LEN + 2
+  if (len >= ANONYMOUS_NAMESPACE_PREFIX_LEN + 2
  && memcmp (name, ANONYMOUS_NAMESPACE_PREFIX,
ANONYMOUS_NAMESPACE_PREFIX_LEN) == 0)
{
@@ -1870,7 +1870,7 @@
{
  struct demangle_component *p = NULL;
  struct demangle_component *next = NULL;
-  long len, i;
+  int len, i;
  char c;
  const char *str;

@@ -2012,7 +2012,7 @@
   case 'C':
 {
   struct demangle_component *derived_type;
-   long offset;
+   int offset;
   struct demangle_component *base_type;

   derived_type = cplus_demangle_type (di);
@@ -2946,10 +2946,10 @@

/*  _ */

-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')
@@ -2969,7 +2969,7 @@
static struct demangle_component *
d_template_param (struct d_info *di)
{
-  long param;
+  int param;

  if (! d_check_char (di, 'T'))
return NULL;
@@ -3502,7 +3502,7 @@
static int
d_discriminator (struct d_info *di)
{
-  long discrim;
+  int discrim;

  if (d_peek_char (di) != '_')
return 1;
@@ -3558,7 +3558,7 @@
d_unnamed_type (struct d_info *di)
{
  struct demangle_component *ret;
-  long num;
+  int num;

  if (! d_check_char (di, 'U'))
return NULL;
@@ -4086,7 +4086,7 @@

Re: Fix for PR70498 in Libiberty Demangler

[Bernd Schmidt]

On 04/01/2016 12:21 PM, Marcel Böhme wrote:

This fixes the write access violation detailed in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70498 (and a few other
unreported cases).

Sometimes length-variables for strings and arrays are of type long
other times of type int. Since cp-demangle.h exports structs and
methods with length-variables of type int, this has been made
consistent in cp-demangle.c.


Patches need to be bootstrapped and regression tested, and patch 
submissions should include which target this was done on.


Ideally you'd also want to include testcases along with your patches, 
although I'm not entirely sure how we can arrange for this type of 
problem to be tested.


Lastly, for this specific patch, I have trouble seeing how it fixes 
anything. I'd need a more detailed explanation of how the problem 
happens in the first place.



Bernd