On Thu, Oct 11, 2012 at 4:41 PM, Jakub Zawadzki <[email protected]> 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 ___________________________________________________________________________ Sent via: Wireshark-dev mailing list <[email protected]> Archives: http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:[email protected]?subject=unsubscribe
