Ethereal-dev: Re: [Ethereal-dev] Re: [Ethereal-cvs] rev 13924: /trunk/epan/dissectors/: packet
Michael Tuexen wrote:
Have the main loop for dissecting chunks check that the chunk size is
large enough for the chunk header, and have it pass the chunk size,
minus the size of the chunk header, to dissectors for particular chunk
types. Make those dissectors check that value to make sure it's large
enough for any fixed-length portion before subtracting the length of
that portion and using the result as a remaining data length.
Why?
So that the individual chunk dissectors don't each have to check whether
the chunk length is less than the chunk header size.
They weren't doing so - they were just (re-)fetching the chunk size and
subtracting the chunk header length from it, which, for corrupt packets
(such as the ones generated by the randpkt tests done by the buildbot),
means that the length might be *negative*.
The check to make sure the length is correct *HAS* to be done if you're
going to subtract something from the chunk length. There's not much
point in requiring each chunk dissector to make sure the chunk length is
>= CHUNK_HEADER_LENGTH - they'd *ALL* have to make that check, and, if
they have to make that check, we run the risk of somebody adding a new
chunk type dissector, if a new chunk type is defined, and forgetting to
make the check.
The principle I based the dissector on was that if the tvb is too short
I will call proto_tree_add_item() with an index out of bound. This raised
the 'malformed frame' exception and that is fine.
Unfortunately, that wasn't helping here. Instead, negative lengths were
being handed to various proto_tree_ routines, which:
before Ulf's changes to use the DISSECTOR_ASSERT macros, caused
g_assert()s to fail, causing core dumps, which is not fine;
after Ulf's changes to use the DISSECTOR_ASSERT macros, caused
"Dissector bug" messages to be produced, which is not fine, especially
when those messages started to be caught by the randpkt tests and mailed
to ethereal-dev as buildbot crash messages.
The old style was used with some thinking to handle the different cases
of chunk length values and the length of the tvbs (which is not always
equal) correctly.
Unfortunately, it wasn't handling too-short chunk lengths correctly, as
per the above.
The common chunk dissector is *ONLY* checking against
CHUNK_HEADER_LENGTH. The individual chunk dissectors are checking
against the appropriate length for the type of chunk in question.
