Re: [Wireshark-commits] master fe195c0: Don't throw for offset at end of TVB with len -1.

[Evan Huus <eapache () gmail com>]

On Fri, Apr 25, 2014 at 5:40 PM, Evan Huus <eapache () gmail com> wrote:
> On Thu, Apr 24, 2014 at 5:47 PM, Jeff Morriss <jeff.morriss.ws () gmail com> 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 () gmail com)
> > > Changed: branch: master
> > > Repository: wireshark
> > >
> > > Commits:
> > >
> > > fe195c0 by Evan Huus (eapache () gmail com):
> > >
> > >      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?
___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe