Re: RFD: Limiting scope of ep_ memory

[Anders Broman <anders.broman () ericsson com>]

-----Original Message-----
From: wireshark-dev-bounces () wireshark org [mailto:wireshark-dev-bounces () wireshark org] On Behalf Of Evan Huus
Sent: den 11 oktober 2012 22:54
To: Developer support list for Wireshark
Subject: Re: [Wireshark-dev] RFD: Limiting scope of ep_ memory

On Thu, Oct 11, 2012 at 4:41 PM, Jakub Zawadzki <darkjames-ws () darkjames pl> wrote:
> > Hi,
> >
> > Right now ep_ memory is freed at the begining of epan_dissect_run(), 
> > which means that pointers allocated by ep_ can be safety accessed only 
> > before dissecting next packet.
> >
> > When using GUI epan_dissect_run() can be run when refreshing interface (think: packet list).
> > Which can happen at *any* time. Originally it caused bug #5284.
> >
> > Right now we (I and Evan) can't reproduce this bug, but there's still the problem.
> >
> >
> > To make it reproductable (and clear) I want to make ep_ memory 
> > available
> > *only* when dissecting packet, i.e.
> >
> > epan_dissect_run():
> >   ep_free_all();
> >   dissect_packet(edt, pseudo_header, data, fd, cinfo);
> >   ep_free_all();
> >
> > Taps also use ep_ memory, so I propose new function:
> >
> > epan_dissect_run_tap():
> >   ep_free_all();
> >   tap_queue_init(edt);
> >   dissect_packet(edt, pseudo_header, data, fd, cinfo);
> >   tap_push_tapped_queue(edt);
> >   ep_free_all();
> >
> > (For now I want to have ep_free_all() before and after dissecting, 
> > before release we  can remove the one before).
> >
> > Thanks to it, and memory scrubbing, any ep_ allocated pointer used 
> > after dissecting should be catched fast.
> >
> > It'll be no longer possible to save ep_ pointers in taps [1], or 
> > communicate with UI by storing ep_ memory in packet_info.
> >
> >
> > I hope this proposal is understable and sane.
> > If we want to allow using ep_ memory between epan_dissect_init() and 
> > epan_dissect_cleanup() we need more complicated allocator, which we currently don't have.
> > It's doable, but I'm not sure if really needed.
> >
> > Regards,
> >  Jakub.
> >
> > [1] http://www.wireshark.org/lists/wireshark-dev/201210/msg00094.html

+1

> I also think we should limit it so it's not possible to use ep_ memory while there isn't a packet being 
> dissected. During my original attempt to create a safer allocator I was forced to add a dummy memory pool to> 
> work around the numerous locations that use ep_ memory when there isn't a packet in scope. If there isn't a 
> packet currently in scope we have no guarantees when ep_ memory will next be freed, and so it isn't safe to be >used.
>
> Perhaps the
> >   ep_free_all();
> >   dissect_packet(edt, pseudo_header, data, fd, cinfo);
> >   ep_free_all();
in Jakub's patch could be
> >   ep_start_packet();
> >   dissect_packet(edt, pseudo_header, data, fd, cinfo);
> >   ep_end_packet();
> where the two new functions just call ep_free_all() and flip a boolean. If ep_alloc() is called when the 
> boolean is false (there is no packet in scope) then it should g_warning (or assert, but I figure trunk would be 
> > unusable for days if we do that).
>
> Other thoughts?
> Evan

Why not make a patch that developers can try out and start debugging?
Regards
Anders

___________________________________________________________________________
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
___________________________________________________________________________
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