Ethereal-dev: Re: [Ethereal-dev] Re: rev 17527: /trunk/plugins/profinet/: packet-pn-dcp.c

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

Date Index Thread Index Other Months All Mailing Lists
Date Prev Date Next Thread Prev Thread Next
From: "Lars Roland" <Lars.Roland@xxxxxxx>
Date: Fri, 10 Mar 2006 16:47:17 +0100
Use this updated patch instead.

Am 10.03.2006, 16:20 Uhr, schrieb Lars Roland <Lars.Roland@xxxxxxx>:


Am 10.03.2006, 09:47 Uhr, schrieb Ulf Lamping <ulf.lamping@xxxxxx>:


Lars Roland wrote:

I'm not familiar with this protocol, too.

I'm a bit more familiar with this protocol as I've written the
dissector, so I'm the one to blame :-)

However I guess that it is mandatory in this protocol to have a length
indicator after the type and subtype indicator.
Thus the length of the value should be stored unconditionally in
"block_length", like in dissect_PNDCP_Suboption_IP().

I'm not sure if that's really the case under all circumstances (just
can't remember, writing the dissector is already some time ago).


It makes only sense to have no length field, if the value consumes all
remaining bytes of the pdu.


If the remaining length of the pdu, which is stored in "length", is big
enough to contain the whole tlv block, should be checked seperately.

IMO in its current state the dissector is not very robust.

Ooops ...
I've done some fuzz-testing on this dissector so it shouldn't be too bad.
So practical tests invalidate my theory. It is probably more robust than I 
thought. :)


I have some time left to work on these issues on the weekend, but I
don't have any traces to test with or a specification to verify my
assumptions on the protocol structure.

Unfortunately, I can't hand you the specs or traces :-(

As a short time workaround, it should be sufficient to throw an
exception, if "block_length" is used uninitialized.

The DCP packets are usually short, so there shouldn't be problems with
fragmenting or such. So if block_length is used uninitialized it's worth
an exception and that might even be the "long time workaround" :-)

Or simply initialize block_length with 0 as that would be also ok.

If someone can pass me the coverity reports of this dissector, I'll have
a look.



Coverity just complains that block_length might be used uninitialized in
some weird conditions.
IMO it might be possible (I'm not sure about that) to get into an infinite 
loop, as this value can influence the stop condition of a while loop.

The attached patch will solve coverity's and my complains. If these
changes are OK for you, I'll check them in.

Best regards,
Lars

Attachment: packet-pn-dcp.c.diff
Description: Binary data