Huge thanks to our Platinum Members Endace and LiveAction,
and our Silver Member Veeam, for supporting the Wireshark Foundation and project.

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

From: Jeff Morriss <jeff.morriss@xxxxxxxxxxx>
Date: Wed, 18 Apr 2007 15:07:57 +0800


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.