Wireshark · Wireshark-dev: Re: [Wireshark-dev] [Wireshark-commits] rev 40312: /trunk/ /trunk/epan/dissectors/: packet-bt-dht.c

["Maynard, Chris" <Christopher.Maynard@xxxxxxxxx>]

> -----Original Message-----
> From: wireshark-dev-bounces@xxxxxxxxxxxxx [mailto:wireshark-dev-
> bounces@xxxxxxxxxxxxx] On Behalf Of Maynard, Chris
> Sent: Wednesday, December 28, 2011 12:21 PM
> To: 'wireshark-dev@xxxxxxxxxxxxx'
> Subject: Re: [Wireshark-dev] [Wireshark-commits] rev 40312: /trunk/
> /trunk/epan/dissectors/: packet-bt-dht.c packet-gopher.c packet-
> gsm_ipa.c packet-meta.c packet-mux27010.c packet-nfs.c packet-rdp.c
> packet-sametime.c packet-ua.c packet-xtp.c ...
> 
> There are 2 other files that I believe also have tvb_get_string()-
> related memory leaks, but they are somewhat confusing to me, and so I
> would appreciate it if someone could take a look at them.
> 
> 1) packet-ldap.c:dissect_ldap_Mechanism(): 

I'm still not completely sure about this one if someone cares to have a look.

> 2) packet-wsp.c: At line 1281, there is a nice NOTE warning about
> ensuring to call g_free() since tvb_get_stringz() returns g_malloc()ed
> memory.  There is actually only 1 instance of tvb_get_stringz();
> however, it is called from the get_text_string() macro.
> get_text_string(), in turn, is called by a bunch of other macros, such
> as get_token_text(), get_extension_media(), get_text_value(),
> get_quoted_string(), get_uri_value() and get_version_value().  Because
> all of those therefore end up calling tvb_get_stringz(), I believe we
> need to see some g_free()'s after all of them.
> 
> This doesn't look like the case, but before I went to try to fix them,
> I noticed this confusing code/comment at line 2243:
>             get_token_text(val_str, tvb, off, len, ok); \
>             /* As we're using val_str, it is automatically g_free()d */
> \
> 
> There is no g_free() following that code, so I think there is a leak
> here and that the comment can't be true ... can it?  Am I missing
> something here?  I stopped looking for leaks at this point for this
> dissector until I can clear up this confusion.

Well, as far as I can tell, there must be leaks with the current implementation.  Attached are 2 different proposed patches to try to plug them.  The first one adds the g_free's where needed, and the second one just has get_text_string() return ep_alloc'd memory instead of g_malloc'd memory, so there's no need to have all the g_free's sprinkled all over the place anymore.  I prefer patch #2 as I think it's less error prone.  If no preference or problems with it, I'll commit that one soon ...

Thanks.
- Chris
P.S. Oh yes, maybe you could also take a look at the NOTE I added, as I don't understand why the call to get_uri_value() is made in that case.

> > -----Original Message-----
> > From: wireshark-commits-bounces@xxxxxxxxxxxxx [mailto:wireshark-
> > commits-bounces@xxxxxxxxxxxxx] On Behalf Of cmaynard@xxxxxxxxxxxxx
> > Sent: Wednesday, December 28, 2011 11:37 AM
> > To: wireshark-commits@xxxxxxxxxxxxx
> > Subject: [Wireshark-commits] rev 40312: /trunk/
> > /trunk/epan/dissectors/: packet-bt-dht.c packet-gopher.c packet-
> > gsm_ipa.c packet-meta.c packet-mux27010.c packet-nfs.c packet-rdp.c
> > packet-sametime.c packet-ua.c packet-xtp.c ...
> >
> >
> http://anonsvn.wireshark.org/viewvc/viewvc.cgi?view=rev&revision=40312
> >
> > User: cmaynard
> > Date: 2011/12/28 08:36 AM
> >
> > Log:
> >  Fix memory leaks involving tvb_get_string[z]().
> >
> > Directory: /trunk/epan/dissectors/
> >   Changes    Path                 Action
> >   +5 -5      packet-bt-dht.c      Modified
> >   +1 -0      packet-gopher.c      Modified
> >   +1 -1      packet-gsm_ipa.c     Modified
> >   +3 -0      packet-meta.c        Modified
> >   +7 -5      packet-mux27010.c    Modified
> >   +3 -0      packet-nfs.c         Modified
> >   +6 -6      packet-rdp.c         Modified
> >   +1 -1      packet-sametime.c    Modified
> >   +4 -4      packet-ua.c          Modified
> >   +1 -0      packet-xtp.c         Modified
> >
> >
> > (1 file not shown)

--

Attachment: packet-wsp_2.patch
Description: packet-wsp_2.patch

Attachment: packet-wsp_1.patch
Description: packet-wsp_1.patch

CONFIDENTIALITY NOTICE: The information contained in this email message is intended only for use of the intended recipient. If the reader of this message is not the intended recipient, you are hereby notified that any dissemination, distribution or copying of this communication is strictly prohibited. If you have received this communication in error, please immediately delete it from your system and notify the sender by replying to this email.  Thank you.