Huge thanks to our Platinum Members Endace and LiveAction,
and our Silver Member Veeam, for supporting the Wireshark Foundation and project.

Ethereal-dev: Re: [Ethereal-dev] Malformed packet

Note: This archive is from the project's previous web site, ethereal.com. This list is no longer active.

From: Guy Harris <guy@xxxxxxxxxx>
Date: Wed, 26 Mar 2003 11:26:44 -0800
On Wed, Mar 26, 2003 at 12:47:43PM +0100, ana wrote:
> I continuos working on the DSR (Dynamic Source Route) protocol.

Is that "The Dynamic Source Routing Protocol for Mobile Ad Hoc Networks
(DSR)":

	http://www.ietf.org/internet-drafts/draft-ietf-manet-dsr-08.txt

It looks, from the dissector code in your message, as if it is; I will
assume that it is.

> This is the dissector code:
> 
> guint8 optdatalen,offset= 0x00;

No dissector should *EVER* have a one-byte or a two-byte variable that
represents an offset in a packet; if you do that, you run the risk of
that variable overflowing.

That's true even if the lengths of individual items in the packet *do*
fit into 1 byte.

You are less likely to run that risk with a 32-bit offset, although you
might still have to be careful.  Declare it as

	int offset = 0;

instead.

> guint16 header_len;
> 
> while ((header_len-offset)!=0)

You haven't yet given "header_len" a value; I assume you give it a value
elsewhere.

>             {
> 
>             pdu_type = tvb_get_guint8(tvb,4+offset);

The code might be a little bit cleaner if, before the loop processing
the options, you did

	offset += 4;

to skip the Next Header, F/Reserved, and Payload Length fields - or do
so when you fetch the values of those fields (presumably "header_len" is
set to the value from the Payload Length field).  Then just use "offset"
to fetch "pdu_type".

>             optdatalen = tvb_get_guint8(tvb,5+offset);

The Pad1 option, according to the specification whose URL is given
above, doesn't *have* an Opt Data Len field, so you shouldn't fetch
"optdatalen" if pdu_type is the value for Pad1.

>             header_len-=(optdatalen+2);

So you're subtracting from the payload length the length of the option?

>             if (pdu_type==DSR_PAD0)

Do you mean "PAD0" or "PAD1"?  The DSR spec calls the 1-byte padding
option, which has only an Option Type field, "Pad1".

>                 optdatalen+=1;

If you meant "PAD1", then that's not valid, as optdatalen isn't valid -
it was set to whatever the byte was that followed the Pad1 option.

>             else
>                 optdatalen+=2;

The maximum value of "optdatalen" is 255, and if you add 1 to that, it
becomes 256, which doesn't fit into an 8-bit variable; therefore, you
can't do that if it's an 8-bit variable.

>             offset+=optdatalen;

So you're adding the length of the option to the offset - but you're
also subtracting from the payload length the length of the option, so
the loop shouldn't be checking whether "header_len-offset" is 0.  You
should either

	1) subtract from the header length the length of the option, and
	   check whether the header length is 0

or

	2) leave the header length alone and check whether the offset
	   *relative to the beginning of the option data* is greater
	   than or equal to the header length.

I'd go with the first of those choices.