[reSIProcate] Memory leak when creating the SDP from string

[Scott Godin]

Ah yes I see the problem with my first suggestion.

I'm noticing the Contents class has an addBuffer method.  The added buffers
are just tracked and deleted in the Contents destructor.  You could
allocate a manual buffer (character/byte array), assign your contents data
to it, pass this buffer to HeaderFieldValue, then create the contents and
then call addBuffer on the contents.  This should keep the data "inscope"
as long as the Contents class is around.

*Warning: Untested

Scott

On Mon, Nov 12, 2018 at 2:13 PM Diego Carvalho Domingos <
ddomingos at daitan.com> wrote:

> Hi Scott, thanks for answering. The problem seems to be when hfv is passed
> to Contents. Pidf was just an example. I’m using SdpContents. But both use
> the same structure for their constructors:
>
> Pidf::Pidf(const HeaderFieldValue& hfv, const Mime& contentsType)
>
>    : Contents(hfv, contentsType),
>
>      mNote()
>
> {
>
> }
>
> SdpContents::SdpContents(const HeaderFieldValue& hfv, const Mime&
> contentTypes)
>
>    : Contents(hfv, contentTypes)
>
> {
>
> }
>
> The problem is that the Contents constructor being called is:
>
>
>
> Contents::Contents(const HeaderFieldValue& headerFieldValue,
>
>                    const Mime& contentType)
>
>    : LazyParser(headerFieldValue),
>
>       mType(contentType)
>
> {
>
>    init();
>
> }
>
> And not:
>
> Contents::Contents(const HeaderFieldValue& headerFieldValue,
>
>                      *HeaderFieldValue::CopyPaddingEnum e*,
>
>                      const Mime& contentsType)
>
>     : LazyParser(headerFieldValue,e),
>
>     mType(contentsType)
>
> {
>
>    init();
>
> }
>
> If the second one was being used, the copied hvf would have the ownership.
> So, my function is pretty similar to that of the link in my previous
> message. I’ll copy over:
>
>
>
> SdpContents* generateBody()
>
>        {
>
>           Data* txt = new Data("v=0\r\n"
>
>                                "o=1900 369696545 369696545 IN IP4
> 192.168.2.15\r\n"
>
>                                "s=X-Lite\r\n"
>
>                                "c=IN IP4 192.168.2.15\r\n"
>
>                                "t=0 0\r\n"
>
>                                "m=audio 8000 RTP/AVP 8 3 101\r\n"
>
>                                "a=rtpmap:8 pcma/8000\r\n"
>
>                                "a=rtpmap:3 gsm/8000\r\n"
>
>                                "a=rtpmap:101 telephone-event/8000\r\n"
>
>                                "a=fmtp:101 0-15\r\n");
>
>
>
>           HeaderFieldValue hfv(txt->data(), txt->size());
>
>           SdpContents *sdp = new SdpContents(hfv, Mime("application",
> "sdp"));
>
>           return sdp;
>
>        }
>
>
>
> When “new SdpContents(hfv, Mime("application", "sdp"))” is called, that
> copy I explained above is performed. If I use the init method you
> suggested, it will delete the data when the local hfv goes out of scope and
> will crash since the ownership was not passed to SdpContents’ hfv. Note
> sure if this is a design problem or I’m missing something here. The only
> solution I see is to pass the data along and defer the creation of
> SdpContents to the moment it is used. Let me know if you have another idea.
> Thanks.
>
>
>
> Diego
>
>
>
> *From:* Scott Godin <sgodin at sipspectrum.com>
> *Sent:* Monday, November 12, 2018 12:46 PM
> *To:* Diego Carvalho Domingos <ddomingos at daitan.com>
> *Cc:* resiprocate-devel at resiprocate.org
> *Subject:* Re: [reSIProcate] Memory leak when creating the SDP from string
>
>
>
> Hi Diego,
>
>
>
> Many resip classes will try to use memory as efficiently as possible, by
> avoiding data copies.  If the a SIP message block received off the wire is
> stored in it's entirety, then the objects that attach to it to provide
> parsing, endeavor to avoid creating copies of all the header bytes, or body
> byte as they are parsed.   So what is expected in your examples is the the
> original Data block you provided stays around for as long as the resulting
> Pidf contents class.  If the Data goes away and the Pidf class sticks
> around longer, then it will core.
>
>
>
> In your case, you really want HeaderFieldValue to take ownership of the
> Data or to copy it and own it.  There is no interface to have
> HeaderFieldValue take ownership of a Data object, so....
>
>
>
> Looking at the HeaderFieldValue class, seems like the easiest thing to use
> would be the init method, which allows you to specify if it should own the
> data or not.  I haven't tested this, but give it a go....
>
>   Data text(publishBody);
>
>    HeaderFieldValue hfv;
>
>    hfv.init(text.data(), text.size(), true /* own? */);
>
>    Mime type("application", "pidf+xml");
>
>    Pidf pc(hfv, type);
>
>    boost::shared_ptr<resip::Contents> body(&pc);
>
>
>
> Scott
>
>
>
>
>
>
>
>
>
>
>
> On Fri, Nov 9, 2018 at 1:31 PM Diego Carvalho Domingos <
> ddomingos at daitan.com> wrote:
>
> Recently I found that my code has a leak when creating the SDP from
> string. I tried to find the correct way of doing this in examples/tests but
> could not understand who is responsible to delete the Data pointer. For
> instance, I found this:
>
> /*
>
>    WHAT PREVIOUSLY WORKED BUT NOW CORES...
>
>    Data text(publishBody);
>
>    HeaderFieldValue hfv(text.data(), text.size());
>
>    Mime type("application", "pidf+xml");
>
>    Pidf pc(&hfv, type);
>
>    boost::shared_ptr<resip::Contents> body(&pc);
>
>
>
>    ....
>
>    WHAT NOW NEEDS TO BE DONE INSTEAD..
>
>    */
>
>    Data* text = new Data(publishBody);
>
>    HeaderFieldValue hfv(text->data(), text->size());
>
>    Mime type("application", "pidf+xml");
>
>    Pidf* pc = new Pidf(hfv, type);
>
>    boost::shared_ptr<resip::Pidf> body(pc);
>
>    /*
>
>    *
>
> */ *Is this the correct way? If so, who deletes the data in this code? I
> could not find in either HeaderFieldValue (in the constructor that takes a
> char * and size, mMine is set to false) nor in Contents (in my case
> SdpContents).
> Another example, which is very similar to my case, is the generateBody in
> this example (I believe it is no longer in the current code base):
> http://svn.resiprocate.org/dox/dum/testSMIMEInvite_8cxx_source.html#l00232
> I did not test but I guess there is a leak in this example as well. Who
> deletes the pointer returned by generateBody and who deletes the Data *
> created inside the method?
>
> I appreciate any help. Thanks in advance. Att,
>
> Diego Domingos
>
> _______________________________________________
> resiprocate-devel mailing list
> resiprocate-devel at resiprocate.org
> https://list.resiprocate.org/mailman/listinfo/resiprocate-devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20181112/38c9f9d5/attachment.htm>