cplus-demangler, free resource after a failed call to gnu_special.
Commit Message
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
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
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
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
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/
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
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
@@ -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;
}
}
@@ -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