[PATCH] cplus-demangler, free resource after a failed call to gnu_special.

[PATCH] cplus-demangler, free resource after a failed call to gnu_special.

[Andrew Burgess]

Fixes issue:
  https://sourceware.org/bugzilla/show_bug.cgi?id=16817

A call to gnu_special within internal_cplus_demangle could cause memory
resources to be allocated, even if the demangle eventually fails.  The
following call into demangle_prefix will then be passed some partially
initialised state.

I've only tested this against the libiberty "make check" and against gdb on
x86-64 linux.

I don't have write access for gcc svn, but Broadcom does have a copyright
assignment in place for gcc, so if this patch is approved, could someone
apply it please.

Thanks,
Andrew



libiberty/ChangeLog

	* cplus-dmem.c (internal_cplus_demangle): Free any resources
	allocated by possible previous call to gnu_special.
	(squangle_mop_up): Reset pointers to NULL after calling free.
	* testsuite/demangle-expected: New test case.
---
 libiberty/cplus-dem.c                 | 7 +++++++
 libiberty/testsuite/demangle-expected | 5 +++++
 2 files changed, 12 insertions(+)

diff --git a/libiberty/cplus-dem.c b/libiberty/cplus-dem.c
index e948487..1c41c6f 100644
--- a/libiberty/cplus-dem.c
+++ b/libiberty/cplus-dem.c
@@ -1175,6 +1175,11 @@ internal_cplus_demangle (struct work_stuff *work, const char *mangled)
       if ((AUTO_DEMANGLING || GNU_DEMANGLING))
 	{
 	  success = gnu_special (work, &mangled, &decl);
+          if (!success)
+            {
+              delete_work_stuff (work);
+              string_delete (&decl);
+            }
 	}
       if (!success)
 	{
@@ -1218,10 +1223,12 @@ squangle_mop_up (struct work_stuff *work)
   if (work -> btypevec != NULL)
     {
       free ((char *) work -> btypevec);
+      work->btypevec = NULL;
     }
   if (work -> ktypevec != NULL)
     {
       free ((char *) work -> ktypevec);
+      work->ktypevec = NULL;
     }
 }
 
diff --git a/libiberty/testsuite/demangle-expected b/libiberty/testsuite/demangle-expected
index 453f9a3..864ee7e 100644
--- a/libiberty/testsuite/demangle-expected
+++ b/libiberty/testsuite/demangle-expected
@@ -4343,3 +4343,8 @@ cereal::detail::InputBindingMap<cereal::JSONInputArchive>::Serializers cereal::p
 --format=gnu-v3
 _ZNSt9_Any_data9_M_accessIPZ4postISt8functionIFvvEEEvOT_EUlvE_EERS5_v
 void post<std::function<void ()> >(std::function<void ()>&&)::{lambda()#1}*& std::_Any_data::_M_access<void post<std::function<void ()> >(void post<std::function<void ()> >(std::function<void ()>&&)::{lambda()#1}*&&)::{lambda()#1}*>()
+# https://sourceware.org/bugzilla/show_bug.cgi?id=16817
+--format=auto --no-params
+_QueueNotification_QueueController__$4PPPPPPPM_A_INotice___Z
+_QueueNotification_QueueController__$4PPPPPPPM_A_INotice___Z
+_QueueNotification_QueueController__$4PPPPPPPM_A_INotice___Z
-- 
1.8.1.3

Re: [PATCH] cplus-demangler, free resource after a failed call to gnu_special.

[Ian Lance Taylor]

On Fri, May 9, 2014 at 7:35 AM, Andrew Burgess <aburgess@broadcom.com> wrote:

>        if ((AUTO_DEMANGLING || GNU_DEMANGLING))
>         {
>           success = gnu_special (work, &mangled, &decl);
> +          if (!success)
> +            {
> +              delete_work_stuff (work);
> +              string_delete (&decl);
> +            }

As far as I can see, decl may be uninitialized at this point.  I don't
think you can call string_delete.  You need to ensure that decl is
initialized somehow.

Ian

Re: [PATCH] cplus-demangler, free resource after a failed call to gnu_special.

[Andrew Burgess]

On 09/05/2014 9:53 PM, Ian Lance Taylor wrote:
> On Fri, May 9, 2014 at 7:35 AM, Andrew Burgess <aburgess@broadcom.com> wrote:
> 
>>        if ((AUTO_DEMANGLING || GNU_DEMANGLING))
>>         {
>>           success = gnu_special (work, &mangled, &decl);
>> +          if (!success)
>> +            {
>> +              delete_work_stuff (work);
>> +              string_delete (&decl);
>> +            }
> 
> As far as I can see, decl may be uninitialized at this point.  I don't
> think you can call string_delete.  You need to ensure that decl is
> initialized somehow.

There's a call to string_init on decl about 10 lines above the
above diff, just outside of context, but it's unconditional, so
I figured that would be enough.

Also, if gnu_special returns false, and the call to
demangle_prefix returns false then we call (near the bottom of
internal_cplus_demangle) mop_up, which calls string_delete.

Given that decl is initialised once, assuming that the string is
only released using delete_string then the internal state will
have been reset back to NULL, so calling delete_string should
be safe again.

Could you let me know if this is enough, or give me more details
on where you think the problem is as I'm missing it :)

Thanks for taking a look at this.

Andrew

Re: [PATCH] cplus-demangler, free resource after a failed call to gnu_special.

[Ian Lance Taylor]

On Sat, May 10, 2014 at 12:13 PM, Andrew Burgess <aburgess@broadcom.com> wrote:
> On 09/05/2014 9:53 PM, Ian Lance Taylor wrote:
>> On Fri, May 9, 2014 at 7:35 AM, Andrew Burgess <aburgess@broadcom.com> wrote:
>>
>>>        if ((AUTO_DEMANGLING || GNU_DEMANGLING))
>>>         {
>>>           success = gnu_special (work, &mangled, &decl);
>>> +          if (!success)
>>> +            {
>>> +              delete_work_stuff (work);
>>> +              string_delete (&decl);
>>> +            }
>>
>> As far as I can see, decl may be uninitialized at this point.  I don't
>> think you can call string_delete.  You need to ensure that decl is
>> initialized somehow.
>
> There's a call to string_init on decl about 10 lines above the
> above diff, just outside of context, but it's unconditional, so
> I figured that would be enough.
>
> Also, if gnu_special returns false, and the call to
> demangle_prefix returns false then we call (near the bottom of
> internal_cplus_demangle) mop_up, which calls string_delete.
>
> Given that decl is initialised once, assuming that the string is
> only released using delete_string then the internal state will
> have been reset back to NULL, so calling delete_string should
> be safe again.

Right, sorry for the noise.

This patch is OK.

Thanks.

Ian

Re: [PATCH] cplus-demangler, free resource after a failed call to gnu_special.

[Gary Benson]

Ian Lance Taylor wrote:
> Andrew Burgess <aburgess@broadcom.com> wrote:
> > On 09/05/2014 9:53 PM, Ian Lance Taylor wrote:
> > > Andrew Burgess <aburgess@broadcom.com> wrote:
> > > >        if ((AUTO_DEMANGLING || GNU_DEMANGLING))
> > > >         {
> > > >           success = gnu_special (work, &mangled, &decl);
> > > > +          if (!success)
> > > > +            {
> > > > +              delete_work_stuff (work);
> > > > +              string_delete (&decl);
> > > > +            }
> > >
> > > As far as I can see, decl may be uninitialized at this point.  I
> > > don't think you can call string_delete.  You need to ensure that
> > > decl is initialized somehow.
> >
> > There's a call to string_init on decl about 10 lines above the
> > above diff, just outside of context, but it's unconditional, so
> > I figured that would be enough.
> >
> > Also, if gnu_special returns false, and the call to
> > demangle_prefix returns false then we call (near the bottom of
> > internal_cplus_demangle) mop_up, which calls string_delete.
> >
> > Given that decl is initialised once, assuming that the string is
> > only released using delete_string then the internal state will
> > have been reset back to NULL, so calling delete_string should be
> > safe again.
> 
> Right, sorry for the noise.
> 
> This patch is OK.

Andrew, would you like me to commit this?

Thanks,
Gary

--
http://gbenson.net/

Re: [PATCH] cplus-demangler, free resource after a failed call to gnu_special.

[Andrew Burgess]

On 14/05/2014 10:01 AM, Gary Benson wrote:
> Ian Lance Taylor wrote:
>> Andrew Burgess <aburgess@broadcom.com> wrote:
>>> On 09/05/2014 9:53 PM, Ian Lance Taylor wrote:
>>>> Andrew Burgess <aburgess@broadcom.com> wrote:
>>>>>        if ((AUTO_DEMANGLING || GNU_DEMANGLING))
>>>>>         {
>>>>>           success = gnu_special (work, &mangled, &decl);
>>>>> +          if (!success)
>>>>> +            {
>>>>> +              delete_work_stuff (work);
>>>>> +              string_delete (&decl);
>>>>> +            }
>>>>
>>
>> This patch is OK.
> 
> Andrew, would you like me to commit this?

Yes please.

Thanks,
Andrew

Re: [PATCH] cplus-demangler, free resource after a failed call to gnu_special.

[Gary Benson]

Andrew Burgess wrote:
> On 14/05/2014 10:01 AM, Gary Benson wrote:
> > Ian Lance Taylor wrote:
> > > Andrew Burgess <aburgess@broadcom.com> wrote:
> > > > On 09/05/2014 9:53 PM, Ian Lance Taylor wrote:
> > > > > Andrew Burgess <aburgess@broadcom.com> wrote:
> > > > > >        if ((AUTO_DEMANGLING || GNU_DEMANGLING))
> > > > > >         {
> > > > > >           success = gnu_special (work, &mangled, &decl);
> > > > > > +          if (!success)
> > > > > > +            {
> > > > > > +              delete_work_stuff (work);
> > > > > > +              string_delete (&decl);
> > > > > > +            }
> > > > >
> > >
> > > This patch is OK.
> > 
> > Andrew, would you like me to commit this?
> 
> Yes please.

Done:
https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=210425

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH] cplus-demangler, free resource after a failed call to gnu_special.

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 2998 bytes --]

Hi!

On Wed, 14 May 2014 15:20:16 +0100, Gary Benson <gbenson@redhat.com> wrote:
> Andrew Burgess wrote:
> > On 14/05/2014 10:01 AM, Gary Benson wrote:
> > > Ian Lance Taylor wrote:
> > > > Andrew Burgess <aburgess@broadcom.com> wrote:
> > > > > On 09/05/2014 9:53 PM, Ian Lance Taylor wrote:
> > > > > > Andrew Burgess <aburgess@broadcom.com> wrote:
> > > > > > >        if ((AUTO_DEMANGLING || GNU_DEMANGLING))
> > > > > > >         {
> > > > > > >           success = gnu_special (work, &mangled, &decl);
> > > > > > > +          if (!success)
> > > > > > > +            {
> > > > > > > +              delete_work_stuff (work);
> > > > > > > +              string_delete (&decl);
> > > > > > > +            }
> > > > > >
> > > >
> > > > This patch is OK.
> > > 
> > > Andrew, would you like me to commit this?
> > 
> > Yes please.
> 
> Done:
> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=210425

In GCC, I'm consistenly seeing the following new failure:

    ./test-demangle < ../../../source/libiberty/testsuite/demangle-expected
    FAIL at line 4350, options --format=auto --no-params:
    in:  _QueueNotification_QueueController__$4PPPPPPPM_A_INotice___Z
    out: (null)
    exp: 
    ./test-demangle: 895 tests, 1 failures
    make[2]: *** [check-cplus-dem] Error 1

The patch was committed incompletely; I added the missing last line in
r210803:

commit 8207b6a22d5955c41109399cb09f0af661a593ea
Author:     tschwinge <tschwinge@138bc75d-0d04-0410-961f-82ee72b054a4>
AuthorDate: Thu May 22 11:56:45 2014 +0000
Commit:     tschwinge <tschwinge@138bc75d-0d04-0410-961f-82ee72b054a4>
CommitDate: Thu May 22 11:56:45 2014 +0000

    Fix test in libiberty/testsuite/demangle-expected.
    
    	libiberty/
    	* testsuite/demangle-expected: Fix last commit.
    
    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@210803 138bc75d-0d04-0410-961f-82ee72b054a4
---
 libiberty/ChangeLog                   | 4 ++++
 libiberty/testsuite/demangle-expected | 1 +
 2 files changed, 5 insertions(+)

diff --git libiberty/ChangeLog libiberty/ChangeLog
index 7156be7..7b25c7e 100644
--- libiberty/ChangeLog
+++ libiberty/ChangeLog
@@ -1,3 +1,7 @@
+2014-05-22  Thomas Schwinge  <thomas@codesourcery.com>
+
+	* testsuite/demangle-expected: Fix last commit.
+
 2014-05-14  Andrew Burgess  <aburgess@broadcom.com>
 
 	* cplus-dmem.c (internal_cplus_demangle): Free any resources
diff --git libiberty/testsuite/demangle-expected libiberty/testsuite/demangle-expected
index 823a1c4..864ee7e 100644
--- libiberty/testsuite/demangle-expected
+++ libiberty/testsuite/demangle-expected
@@ -4347,3 +4347,4 @@ void post<std::function<void ()> >(std::function<void ()>&&)::{lambda()#1}*& std
 --format=auto --no-params
 _QueueNotification_QueueController__$4PPPPPPPM_A_INotice___Z
 _QueueNotification_QueueController__$4PPPPPPPM_A_INotice___Z
+_QueueNotification_QueueController__$4PPPPPPPM_A_INotice___Z


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 472 bytes --]

Re: [PATCH] cplus-demangler, free resource after a failed call to gnu_special.

[Gary Benson]

Hi Thomas,

Thomas Schwinge wrote:
> In GCC, I'm consistenly seeing the following new failure:
> 
>     ./test-demangle < ../../../source/libiberty/testsuite/demangle-expected
>     FAIL at line 4350, options --format=auto --no-params:
>     in:  _QueueNotification_QueueController__$4PPPPPPPM_A_INotice___Z
>     out: (null)
>     exp: 
>     ./test-demangle: 895 tests, 1 failures
>     make[2]: *** [check-cplus-dem] Error 1
> 
> The patch was committed incompletely; I added the missing last line in
> r210803:
[snip]
> @@ -4347,3 +4347,4 @@ void post<std::function<void ()> >(std::function<void ()>&&)::{lambda()#1}*& std
>  --format=auto --no-params
>  _QueueNotification_QueueController__$4PPPPPPPM_A_INotice___Z
>  _QueueNotification_QueueController__$4PPPPPPPM_A_INotice___Z
> +_QueueNotification_QueueController__$4PPPPPPPM_A_INotice___Z

I thought that extra line was a mistake; I thought each test was
precisely three lines:

  # options
  # input to be demangled
  # expected output

What is the extra line here?

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH] cplus-demangler, free resource after a failed call to gnu_special.

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 1450 bytes --]

Hi Gary!

On Thu, 22 May 2014 17:02:08 +0100, Gary Benson <gbenson@redhat.com> wrote:
> Thomas Schwinge wrote:
> > In GCC, I'm consistenly seeing the following new failure:
> > 
> >     ./test-demangle < ../../../source/libiberty/testsuite/demangle-expected
> >     FAIL at line 4350, options --format=auto --no-params:
> >     in:  _QueueNotification_QueueController__$4PPPPPPPM_A_INotice___Z
> >     out: (null)
> >     exp: 
> >     ./test-demangle: 895 tests, 1 failures
> >     make[2]: *** [check-cplus-dem] Error 1
> > 
> > The patch was committed incompletely; I added the missing last line in
> > r210803:
> [snip]
> > @@ -4347,3 +4347,4 @@ void post<std::function<void ()> >(std::function<void ()>&&)::{lambda()#1}*& std
> >  --format=auto --no-params
> >  _QueueNotification_QueueController__$4PPPPPPPM_A_INotice___Z
> >  _QueueNotification_QueueController__$4PPPPPPPM_A_INotice___Z
> > +_QueueNotification_QueueController__$4PPPPPPPM_A_INotice___Z
> 
> I thought that extra line was a mistake; I thought each test was
> precisely three lines:
> 
>   # options
>   # input to be demangled
>   # expected output
> 
> What is the extra line here?

I too had to look it up -- see the explanation at the beginning of the
file:

    #    --no-params         There are two lines of expected output; the first
    #                        is with DMGL_PARAMS, the second is without it.


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 472 bytes --]

Re: [PATCH] cplus-demangler, free resource after a failed call to gnu_special.

[Gary Benson]

Thomas Schwinge wrote:
> On Thu, 22 May 2014 17:02:08 +0100, Gary Benson <gbenson@redhat.com> wrote:
> > Thomas Schwinge wrote:
> > > In GCC, I'm consistenly seeing the following new failure:
> > > 
> > >     ./test-demangle < ../../../source/libiberty/testsuite/demangle-expected
> > >     FAIL at line 4350, options --format=auto --no-params:
> > >     in:  _QueueNotification_QueueController__$4PPPPPPPM_A_INotice___Z
> > >     out: (null)
> > >     exp: 
> > >     ./test-demangle: 895 tests, 1 failures
> > >     make[2]: *** [check-cplus-dem] Error 1
> > > 
> > > The patch was committed incompletely; I added the missing last line in
> > > r210803:
> > [snip]
> > > @@ -4347,3 +4347,4 @@ void post<std::function<void ()> >(std::function<void ()>&&)::{lambda()#1}*& std
> > >  --format=auto --no-params
> > >  _QueueNotification_QueueController__$4PPPPPPPM_A_INotice___Z
> > >  _QueueNotification_QueueController__$4PPPPPPPM_A_INotice___Z
> > > +_QueueNotification_QueueController__$4PPPPPPPM_A_INotice___Z
> > 
> > I thought that extra line was a mistake; I thought each test was
> > precisely three lines:
> > 
> >   # options
> >   # input to be demangled
> >   # expected output
> > 
> > What is the extra line here?
> 
> I too had to look it up -- see the explanation at the beginning of
> the file:
> 
>     #    --no-params         There are two lines of expected output; the first
>     #                        is with DMGL_PARAMS, the second is without it.

Ah, I missed that.  Thank you for fixing this!

Gary

--
http://gbenson.net/

Re: [PATCH] cplus-demangler, free resource after a failed call to gnu_special.

[Pedro Alves]

On 05/22/2014 12:57 PM, Thomas Schwinge wrote:
> On Wed, 14 May 2014 15:20:16 +0100, Gary Benson <gbenson@redhat.com> wrote:
>> Andrew Burgess wrote:
>>> On 14/05/2014 10:01 AM, Gary Benson wrote:
>>>> Ian Lance Taylor wrote:
>>>>> This patch is OK.
>>>>
>>>> Andrew, would you like me to commit this?
>>>
>>> Yes please.
>>
>> Done:
>> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=210425
> 
...

>     Fix test in libiberty/testsuite/demangle-expected.
>     
>     	libiberty/
>     	* testsuite/demangle-expected: Fix last commit.

Thanks.

I've merged this and Andrew's patch to the binutils-gdb git repo.

-- 
Pedro Alves