Re: ep_ memory is not garbage collected

[Evan Huus <eapache () gmail com>]

On Wed, Aug 15, 2012 at 11:36 AM, Jakub Zawadzki
<darkjames-ws () darkjames pl> wrote:
> On Wed, Aug 15, 2012 at 10:22:13AM -0400, Evan Huus wrote:
> > On Wed, Aug 15, 2012 at 9:53 AM, Jakub Zawadzki
> > <darkjames-ws () darkjames pl> wrote:
> > > From commit r42254 (reference counting of edt)
> > > we have problem with ep_ memory not being returned to pool after dissecting packet.
> > > Wireshark after initial opening file, always keep edt for currently selected packet,
> > > so edt_refs is always > 0.
> > >
> > > You can reproduce it by loading some bigger files, and several times open expert
> > > or conversation window.
> > >
> > > After r43188 (not part of wireshark-1.8) affected is also filtering (if at least one packet
> > > match filter), or selecting other packets.
> >
> > I'm not sure I follow. Are we calling epan_dissect_run() for each
> > packet selected without calling
> > epan_dissect_cleanup()/epan_dissect_init() in between?
>
> From r43188 sequence of selecting new packet (cf_select_packet() in file.c) is:
>    cf_read_frame()
>    old_edt = cf->edt;
>    cf->edt = epan_dissect_new()                ; edt_refs++
>    epan_dissect_run(cf->edt, ...)
>    if (old_edt)
>      epan_dissect_free(old_edt);               ; edt_refs--  (edt_refs != 0, no gc)
>
> Old code was doing something like:
>    cf_read_frame()
>    epan_dissect_free(cf->edt);               ; edt_refs--    (likely edt_refs == 0, gc ep memory)
>    cf->edt = epan_dissect_new()              ; edt_refs++
>
> I have working fix for this, but check below.

Based on the log for r43188 I expect there's someway to force clear
the GtkTreeStore, so that old_edt can be unrefed (triggering a gc)
before cf->edt is re-allocated?

Is your fix checked in or attached to a bug somewhere?

> > I understand that to be a bug in the caller, not a bug in ep_'s memory manager, or
> > did r42254 change the behaviour of the API in an unexpected way?
>
> cf_select_packet() don't call epan_dissect_free() but holds edt reference in cf->edt,
> so if we have selected packet than edt_refs > 0 (this is also true before r43188).

Yes, I understand now.

> If in such situation we do something which requires redissecting lot of packet (or do it several times)
> wireshark will eat all your memory ;-)
>
> I think it'd be best if each dissection have unique, single ep_pool (no ref-counting),
> but it's lot of work, so we need some workaround, also for 1.8 ;/

Agreed that this is desirable. Not 100% sure how much work it will be
- does it make sense to attach the ep_ memory pool to the edt struct,
and then just have a global stack whose top tracks the 'active' edt?

I was going to suggest just a global var to track the active edt, but
remembered that some weird nesting conditions were what required the
ref-counting in the first place. I think a stack should take care of
that?

Evan
___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe