On Fri, Apr 25, 2014 at 5:40 PM, Evan Huus <eapache@xxxxxxxxx> wrote:
> On Thu, Apr 24, 2014 at 5:47 PM, Jeff Morriss <jeff.morriss.ws@xxxxxxxxx> wrote:
>> On 04/23/2014 11:57 AM, Wireshark code review wrote:
>>>
>>> URL:
>>> https://code.wireshark.org/review/gitweb?p=wireshark.git;a=commit;h=fe195c0c978b4b92dc5a77daa6449a7f1314243d
>>> Submitter: Evan Huus (eapache@xxxxxxxxx)
>>> Changed: branch: master
>>> Repository: wireshark
>>>
>>> Commits:
>>>
>>> fe195c0 by Evan Huus (eapache@xxxxxxxxx):
>>>
>>> Don't throw for offset at end of TVB with len -1.
>>>
>>> g867a1827e7dc88896ee27a107eb35c4b3973d270 introduced a change to
>>> cleanup/fix
>>> handling of bounds checks for -1 length fields, but it ended up
>>> guaranteeing a
>>> throw for 0-length tvbs, which isn't good; we ought to be able to add
>>> 0-length
>>> FT_PROTOCOL items at the very least.
>>
>>
>> Well nuts...
>>
>>
>>> Better names for the function than _cheat are welcome, but I want to
>>> shut up the
>>> buildbot.
>>
>>
>> What I'm thinking at the moment is your new function should become
>> tvb_captured_length_remaining() (so: throwing an exception returns the old
>> -1 return value). I fail to see any real benefit in not throwing an
>> exception if someone's offset is beyond the end of a TVB (of course I also
>> haven't dug through the uses of the function yet).
>> tvb_ensure_captured_length_remaining() then remains to ensure there's at
>> least one byte there (though I admit I do have some doubts about whether
>> that's really necessary; if a caller wants to ensure there's one byte there
>> then they could/should just call tvb_captured_length_remaining() with an
>> offset of offset+1).
>>
>> Thoughts?
>
> In general, given a tvbuff with available length n, adding an item at
> offset n (zero-indexed) should throw an exception. This is true even
> if the item is a variable-length type (FT_STRING, etc.) with length 0
> (either explicitly, or implicitly via -1 and there being no bytes
> left).
>
> This is a problem for "meta"-fields (FT_PROTOCOL and possibly
> FT_NONE), since if there are no bytes left for a protocol, we probably
> want to have the exception occur on the first "real" field of the
> protocol (and thus in its subtree) rather than in the parent protocol.
> This showed up as a buildbot bug because for 0-bytes-captured tvbs, it
> caused packet-frame.c to throw an uncaught exception when trying to
> add the FT_PROTOCOL for the generic "frame" protocol.
>
> Open Questions:
> 1. Are there any other cases (besides FT_PROTOCOL) where we want to
> make this special exception? FT_NONE? Others?
>
> 2. What is the best way to code this exception? The hack I put in also
> makes it valid for FT_STRING, FT_BYTES, et al to go past the end of
> the tvb like this, which I now think is the wrong thing to do. Perhaps
> we go back to the existing tvb_ensure_captured_length_remaining() for
> most types (which guarantees at least one byte), but leave a
> fall-through exception for FT_PROTOCOL, something like (untested):
>
> case FT_PROTOCOL:
> /* special case so that exceptions end up in the right protocol tree */
> if (tvb_captured_length(tvb) == start) {
> *length = 0;
> break;
> }
> /* else fallthrough */
> case FT_NONE:
> case FT_BYTES:
> case FT_STRING:
> case FT_STRINGZPAD:
> *length = tvb_ensure_captured_length_remaining(tvb, start); /*
> guarantees at least one byte */
> /* DISSECTOR_ASSERT(*length >= 0); assertion unnecessary now
> since the above guarantees at least one byte remaining */
> break;
Nope, this wouldn't work either: packet-frame.c adds all sorts of
offset-0-length-0 items of different types (bools, ints, strings,
you-name-it). Under my conception of the API, these should *all* throw
for a 0-byte TVB and they currently don't. I'm confused.
I think conceptually what we need is a way to say "this item isn't
associated with any bytes at all, so don't do any bounds checks etc".
Negative offsets are already taken in the general case; do we need to
special-define a length of -2 for this? Something else?
Thoughts?