patch for crash in d-lang.c's demangler

patch for crash in d-lang.c's demangler

[Brad Roberts]

There's a minor, but important bug in the d language symbol demangler.  I
haven't reviewed the whole thing for other bugs, just the one that I hit.

I don't have a copyright assignment form on file, but hopefully this diff is
small enough to not require one.  Consider the patch public domain or whatever
if that helps.

diff --git a/gdb/d-lang.c b/gdb/d-lang.c
index 6db521b..f17431b 100644
--- a/gdb/d-lang.c
+++ b/gdb/d-lang.c
@@ -37,8 +37,9 @@ extract_identifiers (const char *mangled_str, struct obstack
*tempbuf)

   while (isdigit (*mangled_str))
     {
-      i = strtol (mangled_str, NULL, 10);
-      mangled_str++;
+      char * end_ptr;
+      i = strtol (mangled_str, &end_ptr, 10);
+      mangled_str = end_ptr;
       if (i <= 0  && strlen (mangled_str) < i)
         return 0;
       obstack_grow (tempbuf, mangled_str, i);

Before this change, symbols with string fragments over 9 bytes long gets into a
bad state and might end up crashing.  Certainly ends up with a bad string.

And example that crashes for me:
    20src/core/atomic.d.9215__unittest_failFiZv

Thanks,
Brad

Re: patch for crash in d-lang.c's demangler

[Tom Tromey]

>>>>> "Brad" == Brad Roberts <braddr@puremagic.com> writes:

Brad> There's a minor, but important bug in the d language symbol
Brad> demangler.  I haven't reviewed the whole thing for other bugs,
Brad> just the one that I hit.

Thanks.

Brad> I don't have a copyright assignment form on file, but hopefully
Brad> this diff is small enough to not require one.

Yes, I agree.

I think your patch is reasonable, but the line just after your change is
weird:

>        if (i <= 0  && strlen (mangled_str) < i)
>          return 0;

I don't think that condition can ever be true.

What do you think of this patch, instead?

Tom

*** d-lang.c.~1.1.~	2010-04-29 08:45:38.000000000 -0600
--- d-lang.c	2010-08-10 16:14:51.000000000 -0600
***************
*** 37,45 ****
  
    while (isdigit (*mangled_str))
      {
!       i = strtol (mangled_str, NULL, 10);
!       mangled_str++;
!       if (i <= 0  && strlen (mangled_str) < i)
          return 0;
        obstack_grow (tempbuf, mangled_str, i);
        mangled_str += i;
--- 37,47 ----
  
    while (isdigit (*mangled_str))
      {
!       char *end_ptr;
! 
!       i = strtol (mangled_str, &end_ptr, 10);
!       mangled_str = end_ptr;
!       if (i <= 0 || strlen (mangled_str) < i)
          return 0;
        obstack_grow (tempbuf, mangled_str, i);
        mangled_str += i;

Re: patch for crash in d-lang.c's demangler

[Brad Roberts]

On Tue, 10 Aug 2010, Tom Tromey wrote:

> >>>>> "Brad" == Brad Roberts <braddr@puremagic.com> writes:
> 
> I think your patch is reasonable, but the line just after your change is
> weird:
> 
> >        if (i <= 0  && strlen (mangled_str) < i)
> >          return 0;
> 
> I don't think that condition can ever be true.

Agreed.  Your patch looks good.  I feel guilty about not contributing a 
unit test to cover this case, so I shouldn't poke you about not adding one 
either, but if you feel like feeling that guilty... :)

Thanks,
Brad

Re: patch for crash in d-lang.c's demangler

[Tom Tromey]

>>>>> "Brad" == Brad Roberts <braddr@puremagic.com> writes:

Brad> Agreed.  Your patch looks good.  I feel guilty about not
Brad> contributing a unit test to cover this case, so I shouldn't poke
Brad> you about not adding one either, but if you feel like feeling that
Brad> guilty... :)

:)

I think it would be good to have test cases for this code, but I'm not
in a good position to write them -- I don't know D.

I'll commit this patch tomorrow with a suitable ChangeLog entry.

Tom

Re: patch for crash in d-lang.c's demangler

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> I'll commit this patch tomorrow with a suitable ChangeLog entry.

I committed it with:

2010-08-11  Brad Roberts  <braddr@puremagic.com>

	* d-lang.c (extract_identifiers): Handle multiple digits.


I am going to put it on the 7.2 branch as well.
It seems like a safe fix.

Tom