diff mbox

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

Message ID 1399646123-9960-1-git-send-email-aburgess@broadcom.com
State Committed
Headers show

Commit Message

Andrew Burgess May 9, 2014, 2:35 p.m. UTC 
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(+)

Comments

Ian Lance Taylor May 9, 2014, 8:53 p.m. UTC | #1 
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
Andrew Burgess May 10, 2014, 7:13 p.m. UTC | #2 
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
Ian Lance Taylor May 12, 2014, 5:40 a.m. UTC | #3 
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
Gary Benson May 14, 2014, 9:01 a.m. UTC | #4 
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/
Andrew Burgess May 14, 2014, 9:30 a.m. UTC | #5 
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
Gary Benson May 14, 2014, 2:20 p.m. UTC | #6 
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
diff mbox

Patch

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