On Wed, Aug 15, 2012 at 11:36 AM, Jakub Zawadzki <darkjames...@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...@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-requ...@wireshark.org?subject=unsubscribe
