[h323plus] Double-delete Capability bug

Simon Horne s.horne at packetizer.com
Sun Jan 24 16:22:29 EST 2016


Iurii

 

I’m confused. There is no difference in the functionality except to resolve the memory leak. H323ExtendedVideoFactory::CreateInstance may return a NULL (unlikely tho’) which also needs to be checked. The change simply separates the CreateInstance, checks for NULL then clone() in basically the same place (another function) as the old implementation.

 

You are correct in that VLD like Valgrind detects a leak with the codec factory system with plugin codecs because the codecs are loaded as pointers to the singleton. The singleton is only created at startup and destroyed at shutdown so do not grow on a call-by-call basis. 

 

There is also a small leak per call with some MediaOptions not being cleaned up. This is an issue as this is on a call-by-call basis. Fixes for that are welcome.

 

Simon

 

From: Iurii Gordiienko [mailto:hordi at ukr.net] 
Sent: Monday, January 25, 2016 4:27 AM
To: Simon Horne <s.horne at packetizer.com>
Cc: 'Jan Willamowius' <jan at willamowius.de>; h323plus at lists.packetizer.com
Subject: Re[2]: [h323plus] Double-delete Capability bug

 

I'm sure - current implementations ::AddCapability are wrong - we should't clone the pointer for H323CodecExtendedVideoCapability::AddCapability(H323Capability * capability) function and should clone for H323CodecExtendedVideoCapability::AddCapability(const PString & cap) function. I think, Valgrind shows memory leak for case when we are using extCapabilities.Add(H323ExtendedVideoFactory::CreateInstance(cap)->clone()) (forr H323CodecExtendedVideoCapability::AddCapability(const PString & cap)) because we have no any default mechanism to clear (delete all capabilities) capabilities factory - we have static factory objects as I remember and Valgrind don't see any desctructor execution. I will write (tomorrow ) simple app and show you proper behavior and destructors execution.

We can't use current version (with clone()) of H323CodecExtendedVideoCapability::AddCapability(H323Capability * capability) fuction because that function has been declared as owner of pointer to H323Capability and any user expects that H323CodecExtendedVideoCapability object will delete that pointer in own destructor. Whis current modiification we have 100% memory leak because it has not declared behaviour.

 

 


2023

void H323CodecExtendedVideoCapability::AddCapability(const PString & cap)


2024

{


2025

    extCapabilities.Add(H323ExtendedVideoFactory::CreateInstance(cap));


2026

}


2027

 


2028

void H323CodecExtendedVideoCapability::AddCapability(H323Capability * capability)


2029

{


2030

    extCapabilities.Add((H323Capability *)capability->Clone());


2031

}

 

--- Original message ---
From: "Simon Horne" <s.horne at packetizer.com <mailto:s.horne at packetizer.com> >
Date: 24 January 2016, 07:23:27

 

Fix checked in. 
The first instance is created in
H323ExtendedVideoCapability::AddAllCapabilities and a new function
H323CodecExtendedVideoCapability::AddCapability(H323Capability *) then
clones the first instance. The first instance is then deleted. This will
ensure every subsequent call to H323ExtendedVideoFactory::CreateInstance()
will create a new unique instance and there is no memory leak.
 
Simon
 
-----Original Message-----
From: h323plus [mailto:h323plus-bounces at lists.packetizer.com] On Behalf Of
Jan Willamowius
Sent: Sunday, January 24, 2016 2:35 AM
To: h323plus at lists.packetizer.com <mailto:h323plus at lists.packetizer.com> 
Subject: Re: [h323plus] Double-delete Capability bug
 
Hi Simon,
 
it does need a better fix. But a leak is still better than a crash.
 
The proper fix is probably to have CreateInstance() always allocate new
memory. But until thats fixed I would leave the Clone() in.
 
Regards,
Jan
 
Simon Horne wrote:
> Jan
> 
> As I suspected, I just check with VLD on windows and the change causes 
> a 9kB/call memory leak (as the capability gets cloned 3 times per 
> call). Needs to be reverted out.
> 
> Simon
> 
> -----Original Message-----
> From: h323plus [mailto:h323plus-bounces at lists.packetizer.com] On 
> Behalf Of Jan Willamowius
> Sent: Saturday, January 23, 2016 4:49 AM
> To: h323plus at lists.packetizer.com <mailto:h323plus at lists.packetizer.com> 
> Subject: Re: [h323plus] Double-delete Capability bug
> 
> Hi Iurii,
> 
> I have added the Clone() in the CVS, but could you please check (wg.
> with Valgrind) if we now have a memory leak in some cases instead of a 
> crash ?
> 
> Regards,
> Jan
> 
> --
> Jan Willamowius, Founder of the GNU Gatekeeper Project EMail  :
> jan at willamowius.de <mailto:jan at willamowius.de> 
> Website: http://www.gnugk.org
> Support: http://www.willamowius.com/gnugk-support.html
> 
> Relaxed Communications GmbH
> Frahmredder 91
> 22393 Hamburg
> Geschäftsführer: Jan Willamowius
> HRB 125261 (Amtsgericht Hamburg)
> USt-IdNr: DE286003584
> 
> Iurii Gordiienko wrote:
> > Hi,
> > I have found something... For
> H323ExtendedVideoCapability::AddAllCapabilities function we are using 
> H323CodecExtendedVideoCapability::AddCapability function for
> extCapability->AddCapability(*r).
> H323CodecExtendedVideoCapability::AddCapability function uses
> extCapabilities.Add(H323ExtendedVideoFactory::CreateInstance(cap))
function.
> The function H323ExtendedVideoFactory::CreateInstance(cap) will call "new"
> if any same "cap" does not is exists for now< for other case it will 
> return the pointer to current actual Capability. But 
> H323CodecExtendedVideoCapability::extCapabilities will delete all 
> items in time of execution  extCapabilities.RemoveAll() function from 
> H323CodecExtendedVideoCapability::~H323CodecExtendedVideoCapability().
> In result we have "double delete" and undefined behavior (crash for 
> worst case).
> > 
> > --------------------------------
> > void H323ExtendedVideoCapability::AddAllCapabilities(
> H323Capabilities & basecapabilities, PINDEX descriptorNum,PINDEX
> simultaneous) {   H323ExtendedVideoFactory::KeyList_T extCaps = 
> H323ExtendedVideoFactory::GetKeyList();   if (extCaps.size() > 0) { 
> H323CodecExtendedVideoCapability * capability = new 
> H323CodecExtendedVideoCapability();
> H323ExtendedVideoFactory::KeyList_T::const_iterator r;         PINDEX 
> num = P_MAX_INDEX;         for (r = extCaps.begin(); r != 
> extCaps.end(); ++r) {
>          H323CodecExtendedVideoCapability * extCapability = 
> (H323CodecExtendedVideoCapability *)capability->Clone();
>  extCapability->AddCapability(*r);            num = 
> basecapabilities.SetCapability(descriptorNum, 
> simultaneous,extCapability);
>          simultaneous = num;         }     simultaneous = P_MAX_INDEX; 
> basecapabilities.SetCapability(descriptorNum, simultaneous,new 
> H323ControlExtendedVideoCapability());  !
> >     delete capability;   }  }
> > void H323CodecExtendedVideoCapability::AddCapability(const PString &
> > cap) {
> > extCapabilities.Add(H323ExtendedVideoFactory::CreateInstance(cap)); 
> > }
> > 
> > -------------------------------------------------
> > I propose to replace the "void
> H323CodecExtendedVideoCapability::AddCapability(const PString & cap)" 
> to this one:
> > void H323CodecExtendedVideoCapability::AddCapability(const PString &
> > cap) {
> > extCapabilities.Add(H323ExtendedVideoFactory::CreateInstance(cap)->c
> > lo
> > ne()); }
> > 
> > 
> > 
> > Thanks -- Iurii Gordiienko
 
 

  <https://mail.ukr.net/api/public/message_read?a=nKmgvdFnKHmsk7uvZLEpXibnDNU6F8_qU1ZGc7zEJB-vdUcJx5CB_-1itriBdHD13nlyTrTQ8rTFlWqJAzfnfYiae8iJaxgScA==> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.packetizer.com/pipermail/h323plus/attachments/20160125/f569ce60/attachment-0002.html>


More information about the h323plus mailing list