On Thu, Oct 11, 2012 at 4:41 PM, Jakub Zawadzki
<darkjames-ws@xxxxxxxxxxxx> 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