Guy Harris wrote:
Jeff Morriss wrote:
Bug 1511 replaced a g_assert() by a DISSECTOR_ASSERT() to avoid exiting
on a bad packet, but that will show up as a "dissector bug" when really
the problem is in the packet.
You're correct - neither g_assert() nor DISSECTOR_ASSERT() are
appropriate for problems with packet data. An assert is something that
should be used to test something that should *always* be true - i.e.,
there's an error in the code if it's not true. That applies to
assert(), g_assert(), and DISSECTOR_ASSERT() - and to any
DISSECTOR_ASSERT_... macro.
True. In this case the assert was checking that the next time through
the loop would not use the same offsets as this time, but that can only
happen if the length (from the packet) is 0.
Any objections to, say, DISSECTOR_ASSERT_MALFORMED_PACKET which would
throw a BoundsError for use in this kind of situations? (No, it's not
really a bounds error
Then it shouldn't be BoundsError.
Somehow I knew you'd say that ;-)
This raises several questions:
1) I seem to remember noticing that, at some point, generic
TVL/AVP/{whatever the particular protocol spec calls them} dissection
code was added for use by dissectors, although I can't remember where it
was. Am I misremembering? If not, where's that code, and could we
convert more dissectors to use it? Presumably if it can dissect
TVL's/AVP's/whatever's when the length is the total length of the item,
rather than the length of the value, it checks for a length that's
shorter than the length of the type field + the length of the length
field, and somehow reports an error.
I don't remember anything like that, but that doesn't mean much.
2) Perhaps some mechanism for flagging a packet as being somehow "bad"
would be useful, so a display filter could check for "bad" packets, and
various types of problems, including but not limited to actual
ReportedBoundsErrors?
Yes. One of the things I like about [Malformed packet] is that it's
obvious (it shows up in the Info column).
Would the expert mechanism be appropriate for that? We can already
mark a packet as having an error with an expert severity of PI_ERROR;
should we support display filters to check for particular expert severities?
Probably. But there doesn't appear to be any display of expert info in
tshark which limits its usefulness. And there some issues (see below).
Anyway, I just changed the DISSECTOR_ASSERT from bug 1511 into an expert
info and a return (to abandon dissecting any additional AVPs).
3) Speaking of the expert mechanism, why are the PI_ values defined in
proto.h rather than expert.h?
Probably for 'proto_item_set_expert_flags()'.
Why 'expert_add_info_format()' is in a different header file is another
question.
Why PI_ERRORs set with the 'proto_item_set_expert_flags()' don't show up
in the Expert dialogs is another question.
Why they also don't mark the whole packet as red (like
'expert_add_info_format()' does) is YAQ.
