Wireshark mailing list archives
Re: [Wireshark-commits] master fe195c0: Don't throw for offset at end of TVB with len -1.
From: Evan Huus <eapache () gmail com>
Date: Fri, 25 Apr 2014 17:48:25 -0400
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
Current thread:
- Re: [Wireshark-commits] master fe195c0: Don't throw for offset at end of TVB with len -1. Jeff Morriss (Apr 24)
- Re: [Wireshark-commits] master fe195c0: Don't throw for offset at end of TVB with len -1. Evan Huus (Apr 25)
- Re: [Wireshark-commits] master fe195c0: Don't throw for offset at end of TVB with len -1. Evan Huus (Apr 25)
- Re: [Wireshark-commits] master fe195c0: Don't throw for offset at end of TVB with len -1. Evan Huus (Apr 25)