<html><body><span class="xfm_15936331"><div>I'm sure - current implementations ::AddCapability are wrong - we should't clone the pointer for <span class="xfmc1" style="font-family:monospace;font-size:medium;white-space:pre;">H323CodecExtendedVideoCapability</span><span class="xfmc2" style="font-family:monospace;font-size:medium;white-space:pre;color:rgb(102, 102, 102);">::</span><span class="xfmc1" style="font-family:monospace;font-size:medium;white-space:pre;">AddCapability</span><span class="xfmc3" style="font-family:monospace;font-size:medium;white-space:pre;">(</span><span class="xfmc1" style="font-family:monospace;font-size:medium;white-space:pre;">H323Capability</span><span style="font-family:monospace;font-size:medium;white-space:pre;"> </span><span class="xfmc2" style="font-family:monospace;font-size:medium;white-space:pre;color:rgb(102, 102, 102);">*</span><span style="font-family:monospace;font-size:medium;white-space:pre;"> </span><span class="xfmc1" style="font-family:monospace;font-size:medium;white-space:pre;">capability</span><span class="xfmc3" style="font-family:monospace;font-size:medium;white-space:pre;">)</span> function and should clone for <span class="xfmc1" style="font-family:monospace;font-size:medium;white-space:pre;">H323CodecExtendedVideoCapability</span><span class="xfmc2" style="font-family:monospace;font-size:medium;white-space:pre;color:rgb(102, 102, 102);">::</span><span class="xfmc1" style="font-family:monospace;font-size:medium;white-space:pre;">AddCapability</span><span class="xfmc3" style="font-family:monospace;font-size:medium;white-space:pre;">(</span><span class="xfmc4" style="font-family:monospace;font-size:medium;white-space:pre;color:rgb(0, 128, 0);font-weight:bold;">const</span><span style="font-family:monospace;font-size:medium;white-space:pre;"> </span><span class="xfmc1" style="font-family:monospace;font-size:medium;white-space:pre;">PString</span><span style="font-family:monospace;font-size:medium;white-space:pre;"> </span><span class="xfmc2" style="font-family:monospace;font-size:medium;white-space:pre;color:rgb(102, 102, 102);">&</span><span style="font-family:monospace;font-size:medium;white-space:pre;"> </span><span class="xfmc1" style="font-family:monospace;font-size:medium;white-space:pre;">cap</span><span class="xfmc3" style="font-family:monospace;font-size:medium;white-space:pre;">) </span>function. I think, Valgrind shows memory leak for case when we are using <span class="xfmc1" style="font-family:monospace;font-size:medium;white-space:pre;">extCapabilities</span><span class="xfmc3" style="font-family:monospace;font-size:medium;white-space:pre;">.</span><span class="xfmc1" style="font-family:monospace;font-size:medium;white-space:pre;">Add</span><span class="xfmc3" style="font-family:monospace;font-size:medium;white-space:pre;">(</span><span class="xfmc1" style="font-family:monospace;font-size:medium;white-space:pre;">H323ExtendedVideoFactory</span><span class="xfmc2" style="font-family:monospace;font-size:medium;white-space:pre;color:rgb(102, 102, 102);">::</span><span class="xfmc1" style="font-family:monospace;font-size:medium;white-space:pre;">CreateInstance</span><span class="xfmc3" style="font-family:monospace;font-size:medium;white-space:pre;">(</span><span class="xfmc1" style="font-family:monospace;font-size:medium;white-space:pre;">cap</span><span class="xfmc3" style="font-family:monospace;font-size:medium;white-space:pre;">)->clone()) (forr </span><span class="xfmc1" style="font-family:monospace;font-size:medium;white-space:pre;">H323CodecExtendedVideoCapability</span><span class="xfmc2" style="font-family:monospace;font-size:medium;white-space:pre;color:rgb(102, 102, 102);">::</span><span class="xfmc1" style="font-family:monospace;font-size:medium;white-space:pre;">AddCapability</span><span class="xfmc3" style="font-family:monospace;font-size:medium;white-space:pre;">(</span><span class="xfmc4" style="font-family:monospace;font-size:medium;white-space:pre;color:rgb(0, 128, 0);font-weight:bold;">const</span><span style="font-family:monospace;font-size:medium;white-space:pre;"> </span><span class="xfmc1" style="font-family:monospace;font-size:medium;white-space:pre;">PString</span><span style="font-family:monospace;font-size:medium;white-space:pre;"> </span><span class="xfmc2" style="font-family:monospace;font-size:medium;white-space:pre;color:rgb(102, 102, 102);">&</span><span style="font-family:monospace;font-size:medium;white-space:pre;"> </span><span class="xfmc1" style="font-family:monospace;font-size:medium;white-space:pre;">cap</span><span class="xfmc3" style="font-family:monospace;font-size:medium;white-space:pre;">)</span><span class="xfmc3" style="font-family:monospace;font-size:medium;white-space:pre;">)</span> 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.</div><div>We can't use current version (with clone()) of <span class="xfmc1" style="font-family:monospace;font-size:medium;white-space:pre;">H323CodecExtendedVideoCapability</span><span class="xfmc2" style="font-family:monospace;font-size:medium;white-space:pre;color:rgb(102, 102, 102);">::</span><span class="xfmc1" style="font-family:monospace;font-size:medium;white-space:pre;">AddCapability</span><span class="xfmc3" style="font-family:monospace;font-size:medium;white-space:pre;">(</span><span class="xfmc1" style="font-family:monospace;font-size:medium;white-space:pre;">H323Capability</span><span style="font-family:monospace;font-size:medium;white-space:pre;"> </span><span class="xfmc2" style="font-family:monospace;font-size:medium;white-space:pre;color:rgb(102, 102, 102);">*</span><span style="font-family:monospace;font-size:medium;white-space:pre;"> </span><span class="xfmc1" style="font-family:monospace;font-size:medium;white-space:pre;">capability</span><span class="xfmc3" style="font-family:monospace;font-size:medium;white-space:pre;">) </span>fuction because that function has been declared as owner of pointer to H323Capability and any user expects that <span style="font-family:monospace;font-size:medium;white-space:pre;">H323CodecExtendedVideoCapability </span>object will delete that pointer in own destructor. Whis current modiification we have 100% memory leak because it has not declared behaviour.</div><div><br/></div><div><br/></div><div><table cellspacing="0" cellpadding="0" style="width:1209px;margin:0px;border:none;font-family:sans-serif;font-size:medium;"><tbody><tr class="xfmc5" id="xfmi1" style="vertical-align:top;background-color:rgb(240, 240, 240);"><td class="xfmc6" style="vertical-align:top;border-right-width:1px;color:rgb(80, 80, 80);text-align:right;border-right-style:solid;border-right-color:rgb(80, 80, 80);padding:1px 5px;background-color:rgb(238, 238, 238);">2023</td><td class="xfmc7" style="vertical-align:top;border-right-width:0px;font-family:monospace;white-space:pre;width:1152px;border-right-style:solid;border-right-color:rgb(80, 80, 80);padding:1px 5px;background-color:white;"><span class="xfmc8" style="color:rgb(176, 0, 64);">void</span> <span class="xfmc1">H323CodecExtendedVideoCapability</span><span class="xfmc2" style="color:rgb(102, 102, 102);">::</span><span class="xfmc1">AddCapability</span><span class="xfmc3">(</span><span class="xfmc4" style="color:rgb(0, 128, 0);font-weight:bold;">const</span> <span class="xfmc1">PString</span> <span class="xfmc2" style="color:rgb(102, 102, 102);">&</span> <span class="xfmc1">cap</span><span class="xfmc3">)</span>
</td></tr><tr class="xfmc5" id="xfmi2" style="vertical-align:top;background-color:rgb(240, 240, 240);"><td class="xfmc6" style="vertical-align:top;border-right-width:1px;color:rgb(80, 80, 80);text-align:right;border-right-style:solid;border-right-color:rgb(80, 80, 80);padding:1px 5px;background-color:rgb(238, 238, 238);">2024</td><td class="xfmc7" style="vertical-align:top;border-right-width:0px;font-family:monospace;white-space:pre;width:1152px;border-right-style:solid;border-right-color:rgb(80, 80, 80);padding:1px 5px;background-color:white;"><span class="xfmc3">{</span>
</td></tr><tr class="xfmc5" id="xfmi3" style="vertical-align:top;background-color:rgb(240, 240, 240);"><td class="xfmc6" style="vertical-align:top;border-right-width:1px;color:rgb(80, 80, 80);text-align:right;border-right-style:solid;border-right-color:rgb(80, 80, 80);padding:1px 5px;background-color:rgb(238, 238, 238);">2025</td><td class="xfmc7" style="vertical-align:top;border-right-width:0px;font-family:monospace;white-space:pre;width:1152px;border-right-style:solid;border-right-color:rgb(80, 80, 80);padding:1px 5px;background-color:white;">    <span class="xfmc1">extCapabilities</span><span class="xfmc3">.</span><span class="xfmc1">Add</span><span class="xfmc3">(</span><span class="xfmc1">H323ExtendedVideoFactory</span><span class="xfmc2" style="color:rgb(102, 102, 102);">::</span><span class="xfmc1">CreateInstance</span><span class="xfmc3">(</span><span class="xfmc1">cap</span><span class="xfmc3">));</span>
</td></tr><tr class="xfmc5" id="xfmi4" style="vertical-align:top;background-color:rgb(240, 240, 240);"><td class="xfmc6" style="vertical-align:top;border-right-width:1px;color:rgb(80, 80, 80);text-align:right;border-right-style:solid;border-right-color:rgb(80, 80, 80);padding:1px 5px;background-color:rgb(238, 238, 238);">2026</td><td class="xfmc7" style="vertical-align:top;border-right-width:0px;font-family:monospace;white-space:pre;width:1152px;border-right-style:solid;border-right-color:rgb(80, 80, 80);padding:1px 5px;background-color:white;"><span class="xfmc3">}</span>
</td></tr><tr class="xfmc5" id="xfmi5" style="vertical-align:top;background-color:rgb(240, 240, 240);"><td class="xfmc6" style="vertical-align:top;border-right-width:1px;color:rgb(80, 80, 80);text-align:right;border-right-style:solid;border-right-color:rgb(80, 80, 80);padding:1px 5px;background-color:rgb(238, 238, 238);">2027</td><td class="xfmc7" style="vertical-align:top;border-right-width:0px;font-family:monospace;white-space:pre;width:1152px;border-right-style:solid;border-right-color:rgb(80, 80, 80);padding:1px 5px;background-color:white;">
</td></tr><tr class="xfmc5" id="xfmi6" style="vertical-align:top;background-color:rgb(240, 240, 240);"><td class="xfmc6" style="vertical-align:top;border-right-width:1px;color:rgb(80, 80, 80);text-align:right;border-right-style:solid;border-right-color:rgb(80, 80, 80);padding:1px 5px;background-color:rgb(238, 238, 238);">2028</td><td class="xfmc7" style="vertical-align:top;border-right-width:0px;font-family:monospace;white-space:pre;width:1152px;border-right-style:solid;border-right-color:rgb(80, 80, 80);padding:1px 5px;background-color:white;"><span class="xfmc8" style="color:rgb(176, 0, 64);">void</span> <span class="xfmc1">H323CodecExtendedVideoCapability</span><span class="xfmc2" style="color:rgb(102, 102, 102);">::</span><span class="xfmc1">AddCapability</span><span class="xfmc3">(</span><span class="xfmc1">H323Capability</span> <span class="xfmc2" style="color:rgb(102, 102, 102);">*</span> <span class="xfmc1">capability</span><span class="xfmc3">)</span>
</td></tr><tr class="xfmc5" id="xfmi7" style="vertical-align:top;background-color:rgb(240, 240, 240);"><td class="xfmc6" style="vertical-align:top;border-right-width:1px;color:rgb(80, 80, 80);text-align:right;border-right-style:solid;border-right-color:rgb(80, 80, 80);padding:1px 5px;background-color:rgb(238, 238, 238);">2029</td><td class="xfmc7" style="vertical-align:top;border-right-width:0px;font-family:monospace;white-space:pre;width:1152px;border-right-style:solid;border-right-color:rgb(80, 80, 80);padding:1px 5px;background-color:white;"><span class="xfmc3">{</span>
</td></tr><tr class="xfmc5" id="xfmi8" style="vertical-align:top;background-color:rgb(240, 240, 240);"><td class="xfmc6" style="vertical-align:top;border-right-width:1px;color:rgb(80, 80, 80);text-align:right;border-right-style:solid;border-right-color:rgb(80, 80, 80);padding:1px 5px;background-color:rgb(238, 238, 238);">2030</td><td class="xfmc7" style="vertical-align:top;border-right-width:0px;font-family:monospace;white-space:pre;width:1152px;border-right-style:solid;border-right-color:rgb(80, 80, 80);padding:1px 5px;background-color:white;">    <span class="xfmc1">extCapabilities</span><span class="xfmc3">.</span><span class="xfmc1">Add</span><span class="xfmc3">((</span><span class="xfmc1">H323Capability</span> <span class="xfmc2" style="color:rgb(102, 102, 102);">*</span><span class="xfmc3">)</span><span class="xfmc1">capability</span><span class="xfmc2" style="color:rgb(102, 102, 102);">-></span><span class="xfmc1">Clone</span><span class="xfmc3">());</span>
</td></tr><tr class="xfmc5" id="xfmi9" style="vertical-align:top;background-color:rgb(240, 240, 240);"><td class="xfmc6" style="vertical-align:top;border-right-width:1px;color:rgb(80, 80, 80);text-align:right;border-right-style:solid;border-right-color:rgb(80, 80, 80);padding:1px 5px;background-color:rgb(238, 238, 238);">2031</td><td class="xfmc7" style="vertical-align:top;border-right-width:0px;font-family:monospace;white-space:pre;width:1152px;border-right-style:solid;border-right-color:rgb(80, 80, 80);padding:1px 5px;background-color:white;"><span class="xfmc3">}</span></td></tr></tbody></table></div><br/><br/><div style="font-size:0.9em;font-style:italic;"> --- Original message ---<br/> From: "Simon Horne" <s.horne@packetizer.com><br/>  Date: 24 January 2016, 07:23:27<br/></div> <br/><blockquote class="xfmc9" style="border-left:1px solid rgb(204, 204, 204);margin:0px 0px 0px 0.8ex;padding-left:1ex;">
  <pre>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: <a href="mailto:h323plus@lists.packetizer.com" target="_blank">h323plus@lists.packetizer.com</a>
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: <a href="mailto:h323plus@lists.packetizer.com" target="_blank">h323plus@lists.packetizer.com</a>
> 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  :
> <a href="mailto:jan@willamowius.de" target="_blank">jan@willamowius.de</a>
> Website: <a href="http://www.gnugk.org" target="_blank">http://www.gnugk.org</a>
> Support: <a href="http://www.willamowius.com/gnugk-support.html" target="_blank">http://www.willamowius.com/gnugk-support.html</a>

> 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


</pre>
</blockquote>   </span><img src="https://mail.ukr.net/api/public/message_read?a=nKmgvdFnKHmsk7uvZLEpXibnDNU6F8_qU1ZGc7zEJB-vdUcS2srd_e9yhYiMfmjk2T54SqWV9K_Bl-5oL7JgA5LCVqJz2cFDPXNVLT_FmOI=" alt="" width="1" height="1" style="visibility: hidden; width: 1px; height: 1px;"/>                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                       </body></html>