Re: register_tap_listener memleak

[Peter Wu <peter () lekensteyn nl>]

Hi Jakub,

On Thu, Mar 15, 2018 at 06:33:28PM +0100, Jakub Zawadzki wrote:
> Hi Peter,
>
> W dniu 2018-03-15 13:24, Peter Wu napisał(a):
> > I was looking at memleaks as reported by LSAN while running the
> > decryption test suite, there are quite a number of occurrences.
> >
> > One of them is tap (return value of register_tap_listener) which is a
> > GString which seems unnecessary since it is an error message which the
> > caller should not have to modify. Dario tried to convert that to a gchar
> > before in https://code.wireshark.org/review/15270 but that particular
> > patch was reverted in v2.1.1rc0-197-ga383e692c8.
> >
> > Pascal tried again in https://code.wireshark.org/review/16053, but
> > somehow it also got stuck. Before trying to touch this again, is there
> > something to be aware of? I just want to modify register_tap_listener:
> >
> > - Change GString to char *
> > - Either use NULL wmem scope or use g_strdup_printf.
> > - Add G_GNUC_WARN_UNUSED_RESULT such that callers will not accidentally
> >   leak any error messages.
> > - Modify callers such that they do check the error. (g_warning?)
>
>
> Can register_tap_listener() return enum code (one of: success, not found,
> wrong filter)?
> You will get rid of memleak, and users will get nicely translated message
> error.

That sounds like a great idea at first, but it seems not viable given
that the error messages are:

- "Tap <tapname parameter> not found"
- "Filter "<fstring parameter>" is invalid - <dfilter_compile error>"

The few dissectors that do check the return value just use
"report_failure" or something like that. Do you think it is better to
introduce an "error" argument, receiving a gchar*? If NULL, then use
"g_warning" within the API, otherwise return it.
-- 
Kind regards,
Peter Wu
https://lekensteyn.nl
___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe