On Fri, Apr 25, 2014 at 5:40 PM, Evan Huus <[email protected]> wrote: > On Thu, Apr 24, 2014 at 5:47 PM, Jeff Morriss <[email protected]> > 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 ([email protected]) >>> Changed: branch: master >>> Repository: wireshark >>> >>> Commits: >>> >>> fe195c0 by Evan Huus ([email protected]): >>> >>> 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 <[email protected]> Archives: http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:[email protected]?subject=unsubscribe
