Wireshark-dev: Re: [Wireshark-dev] [Wireshark-commits] rev 52578: /trunk/epan/ /trunk/epan/: pr

From: Evan Huus <eapache@xxxxxxxxx>
Date: Thu, 17 Oct 2013 08:27:22 -0400
On Sun, Oct 13, 2013 at 3:54 AM, Jakub Zawadzki
<darkjames-ws@xxxxxxxxxxxx> wrote:
>> On Sun, Oct 13, 2013 at 12:54 AM,  <eapache@xxxxxxxxxxxxx> 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).