Wireshark-dev: Re: [Wireshark-dev] DISSECTOR_ASSERT vs. expert_add_info vs. MALFORMED packets
From: Guy Harris <[email protected]>
Date: Tue, 10 Jun 2008 14:55:10 -0700
On Jun 10, 2008, at 1:07 PM, [email protected] 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.