Wireshark · Wireshark-dev: Re: [Wireshark-dev] DISSECTOR_ASSERT vs. expert_add_info vs. MALFORMED packets

[Guy Harris <guy@xxxxxxxxxxxx>]

On Jun 10, 2008, at 1:07 PM, david_aggeler@xxxxxxxxxx wrote:

> In order to better help the end user reading captures, I'm trying to  
> set hints, when decoding problems arise, due to packet data.
>
> My first attempt was using DISSECTOR_ASSERT(), but this causes a  
> 'bug in dissector'. After reading a thread the dev-archive, this is  
> apparently by design, but about 50% of the DISSECTOR_ASSERT() code  
> lines I came across are to catch somewhat expected data errors.

If that's true, that's a bug.

DISSECTOR_ASSERT() is to use to catch dissector *bugs*, i.e. stuff  
that "shouldn't happen" no matter *what* problems there are in the  
data, as per

	http://www.wireshark.org/lists/wireshark-dev/200704/msg00233.html

"You should never call DISSECTOR_ASSERT() for bad packet data.
DISSECTOR_ASSERT() should only be used to detect bad dissector  
code(e.g. causing an endless recursion)."

and

	http://www.wireshark.org/lists/wireshark-dev/200704/msg00234.html

"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."

> As suggested there, I switched to expert_add_info, but with the  
> result, that my packets are not marked MALFORMED, which I think is a  
> pity.

The "malformed" indication originally referred only to packets that  
were too short, so that the dissector, when trying to dissect a field  
that is supposed to be there, or a variable-length field that has a  
specified length, or..., gets an exception thrown by the tvbuff code.

There is now also the PI_MALFORMED expert information group.  The two  
are, confusingly, not connected.  There should probably be a way to  
look for various expert info indications with a display filter - and  
that could perhaps replace the existing "malformed" "protocol".  The  
various exceptions that would cause a "malformed" indication could  
enter an item into the protocol tree with the expert info in question  
attached to it.

> - Is there a 'best practice' to MALFORMED PACKETS without 'bug in  
> dissector'?

There's a *current* practice, which is to lookup the "malformed"  
protocol and put an entry into the protocol tree for that "protocol".   
Whether that's the *best* practice is another matter.

> - In case nobody is working on 'DISSECTOR_VERIFY_DATA' yet, I'm  
> willing to contribute code, but so far only worked on a dissector  
> for a while. In addition I am not really an exception handler  
> specialist, and I guess, this is pretty core code. A rough hint of  
> what would need to be done could maybe get me started.

Note that not all protocol problems should throw an exception; for  
example, if a protocol has an "XYZZY" TLV, which always has a 4-byte  
integer value, but the packet has an XYZZY TLV with a value length of  
8, that should be reported as a malformed TLV, *but* the dissector can  
skip that TLV (as it has the length) and keep dissecting TLVs.  Thus,  
DISSECTOR_VERIFY_DATA() shouldn't *always* be what's used to report  
packet problems; it should only be used to report packet problems that  
prevent any further dissection of the packet.