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@lists.packetizer.com] On Behalf Of Jan Willamowius Sent: Sunday, January 24, 2016 2:35 AM To: h323plus@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@lists.packetizer.com] On Behalf Of Jan Willamowius Sent: Saturday, January 23, 2016 4:49 AM To: h323plus@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@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