Wireshark · Ethereal-dev: Re: [Ethereal-dev] Re: [Ethereal-cvs] rev13869: /trunk/epan/dissectors/: packet-wsp.c

["Olivier Biot (Ethereal)" <ethereal@xxxxxxxxxxxxxxx>]

From: Guy Harris


> Ulf Lamping wrote:
>
> > As this is far better than the former implementation, this still doesn't
> > be the right way to handle this.
>
> In that particular case, none of the routines whose return value is being 
> checked can return null pointers, so in none of those places is the 
> assertion doing a check that bad packet data could cause to fail.
>
> I've removed the DISSECTOR_ASSERT() calls in the WSP dissector - the 
> checks you inserted will catch null pointers passed to 
> "proto_tree_add_string()".

Ths patch basically backed out the previous 2 patches and nuked the 
g_assert() calls which were needless (something I was not aware of at the 
time of writing that code).

> > If incoming data is wrong, it should be shown as such in the protocol
> > tree, but not indicating a dissector bug. Not the dissector is buggy,
> > but the data is (important difference).
>
> Yes.

I agree. In this case, the g_assert() calls were only checking the return 
value of a "foreign" method for which I found documentation at that time 
that it might go wrong. It now looks like I got an incorrect pointer.

> > In a clean dissector implementation, calling DISSECTOR_ASSERT (and
> > g_assert) is not required at all. Just don't do: Oh I'm too lazy to
> > implement this right now, and this might never happen, so I place a
> > DISSECTOR_ASSERT here.
>
> Ideally, it should be implemented; if not, it should at least put 
> "Dissection not implemented yet" or something such as that into the 
> protocol tree.

This is something I already did on the WAP dissectors. You'll get protocol 
error feed-back in the dissection tree for WSP, and even "Not decoded (yet)" 
messages in WSP, WBXML and JFIF/JPEG.

Best regards,

Olivier