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