[h323plus] Double-delete Capability bug

[Simon Horne]

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