On Sun, Oct 13, 2013 at 3:54 AM, Jakub Zawadzki <[email protected]> wrote: >> On Sun, Oct 13, 2013 at 12:54 AM, <[email protected]> wrote: >> > http://anonsvn.wireshark.org/viewvc/viewvc.cgi?view=rev&revision=52578 >> > >> > User: eapache >> > Date: 2013/10/13 04:54 AM >> > >> > Log: > [...] >> > All of this is (theoretically) unnecessary - simply checking the offset >> > without >> > worrying about the length will still catch the very-long-loops, since it >> > is the >> > offset that increases in each iteration, not the length. >> > >> > All that to justify: >> > - implement tvb_ensure_offset_exists which throws an exception if the >> > offset is >> > not within the tvb >> > - use it instead of all the complicated other logic in the >> > pre-short-circuit >> > step of proto_tree_add_item and friends >> > >> > This gives us back about 3/4 of the performance we lost in the original >> > patch. >> > We're still ~2% slower than without any check, but this is the best I can >> > think >> > of right now. >> > > > On Sun, Oct 13, 2013 at 12:58:12AM -0400, Evan Huus wrote: >> Jeff (and/or anyone else who cares) I'd appreciate a verification that >> my logic here is correct, and that I've not accidentally undone your >> good work. > > I think it's fine, unless in loops there're some weird offset integer > overflows (which returns back to offset + 0), like easy case: > > proto_tree_add_item(..., offset, 0xfffffffe /* -2 */, ...); offset += > (-2); > proto_tree_add_item(..., offset, +2, ...); offset += (+2);
You were right, the fuzz-bot found some infinite loops of this type, so I reverted the change. I guess we're just going to have to take the performance hit (sorry Anders). ___________________________________________________________________________ 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
