Ethereal-dev: Re: [Ethereal-dev] Reaction of g_assert maybe too hard for buggy dissectors?
Guy Harris wrote:
Ulf Lamping wrote:
This comes to the following: why don't we check for such things and
fire an exception, so a [Malformed packet] will be displayed.
Because the packet might not be malformed; it might just be that the
*dissector* is malformed.
Well, yes ;-)
Insead, the checks (which should be done in *all* versions, even
production versions) should probably throw a new exception, which
1) causes something such as "[Dissector bug]", perhaps with an
assertion string containing the file/line and assertion expression, as
well as the protocol name, so we know what protocol was being dissected;
2) call "report_failure()", so that a dialog box is popped up in
Ethereal and a message is printed in Tethereal, so that the user knows
that there's something wrong and is thus more likely to tell us about it.
I've added the new exception "FieldError", which should be triggered if
a buggy dissector wanted to add a new field with some buggy parameters.
I would think, that splitting it even further into something like
FieldTypeError, FieldLengthError, ... wouldn't add any real benefit.
In addition, I've recognized that the exception mechanism we use is able
to transfer a message from the THROW to the CATCH way, so I've added the
possibility to transfer such messages to show_exception.
Then I've added the following to proto.c line 2096:
if(*length < 0) {
error_descr = g_strdup_printf("\"%s\" - \"%s\" invalid length:
%d %s/%u",
hfinfo->name, hfinfo->abbrev, *length, __FILE__, __LINE__);
THROW_MESSAGE(FieldError, error_descr);
}
replacing the former g_assert(*length >= 0), so a buggy dissector will
now throw a detailed description. This also will make it a lot easier to
put some breakpoints in, to see what's going wrong while debugging. The
win32 glib handling of the g_assert call could be improved, but that's a
different story.
The show_exception function will then add to:
info column: [Dissector bug]
packet details: [FieldError "FrameID" - "pn_rt.frame_id" invalid length:
-2 proto.c/2098]
console: FieldError in packet: 700 ("FrameID" - "pn_rt.frame_id" invalid
length: -2 proto.c/2098)
...which should include all information to debug.
I don't think that putting up a dialog box each time this appears is a
good idea, as this could result in a lot of boxes going up which is not
a desired behaviour.
If that is the mechanism that everybody can live with, the next step to
do is to identify all the places where the g_assert calls should be
replaced (e.g. in proto.c).
Regards, ULFL
