Wireshark · Wireshark-dev: Re: [Wireshark-dev] assertion for malformed packets?

[Guy Harris <guy@xxxxxxxxxxxx>]

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.