Wireshark · Wireshark-dev: Re: [Wireshark-dev] [Wireshark-commits] master fe195c0: Don't throw for offset at end of TVB with le

[Evan Huus <eapache@xxxxxxxxx>]

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;