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.
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.
but the effect is the desired one: the user gets
shown "malformed packet".) I have, in the past, also wanted a simple
assertion like that when a dissector finds illogical stuff in the packet.
In that particular case, dissection shouldn't necessarily be aborted if
an AVP has a bogus length - a bad AVP within a Grouped AVP should
terminate dissection of the grouped AVP, but the dissector should be
able to dissect the next AVP.
If there are problems at a low level in a packet that should truly abort
the dissection of that entire packet, not just a sub-item in the packet,
we should probably add a new exception for that purpose; we're already
abusing ReportedBoundsError for that in some places. If the problem is
associated with a particular item in the packet, the exception
shouldn't add its own item to the protocol tree, as happens with
ReportedBoundsError - the item in the packet should be displayed with an
error indication, and then the exception should be thrown. That
exception should be caught in the same places that ReportedBoundsError
is caught, so that if a given lower-level packet has data from more than
one higher-level packet, an error in the higher-level packet doesn't
abort dissection of the lower-level packet.
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.
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?
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?
3) Speaking of the expert mechanism, why are the PI_ values defined in
proto.h rather than expert.h?
4) In a number of dissectors, length checking when dissecting sub-items
within an item is done by creating a tvbuff for the item; that means
that if the item is too short, a ReportedBoundsError exception is thrown
if the item is too short, so dissection of the entire packet is aborted
even if items past the too-short item can be dissected.
