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@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@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