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.
