Wireshark · Wireshark-dev: Re: [Wireshark-dev] [Wireshark-commits] rev 45566: /trunk/epan/dissectors/ /trunk/epan/dissectors/:

["Maynard, Chris" <Christopher.Maynard@xxxxxxxxx>]

> -----Original Message-----
> From: wireshark-dev-bounces@xxxxxxxxxxxxx [mailto:wireshark-dev-
> bounces@xxxxxxxxxxxxx] On Behalf Of Martin Kaiser
> Sent: Thursday, October 18, 2012 5:11 PM
> To: wireshark-dev@xxxxxxxxxxxxx
> Subject: Re: [Wireshark-dev] [Wireshark-commits] rev 45566:
> /trunk/epan/dissectors/ /trunk/epan/dissectors/: packet-rdp.c
> 
> Hi,
> 
> Thus wrote Maynard, Chris (Christopher.Maynard@xxxxxxxxx):
> 
> > Recently, I found and fixed some of these problems, but obviously I
> > didn't catch them all.  Are there any more thoughts about changing
> > tvb_length_remaining() and tvb_reported_length_remaining() to return
> 0
> > instead of -1?
> 
> it looks like there's quite a few places where people used an unsigned
> return value (I just fixed a few obvious cases).
> I guess we should do something about this in the tvb part rather than
> in the dissectors.
> 
> What's the difference between return value 0 and -1 now? Both are
> essentially saying there's no data left, -1 is an error case and 0
> isn't? Is that significant to the caller, what can he do other than
> stop reading?
> 
>    Martin

That's my understanding from the code; 0 means no more data and -1 means an invalid offset.  But I was wondering the same thing myself as to whether or not that mattered to the caller.  Those functions have been around for awhile, and I don't know the history of why the -1 was chosen.  Maybe it does matter; maybe not?

There are of course potential adverse ramifications of changing the return value from -1 to 0 though.  For example, a quick grep shows ~100 cases of things like the following (which is of course only a subset of all the problems):

    offset += tvb_length_remaining(...);

I haven't really looked at these occurrences in any detail, but I'm willing to bet that many of those assignments are incorrect since they're not considering either -1 or 0 as a possible return value.  And if 0 is returned instead of -1, then consider what will happen if this assignment is being made within a loop construct - you could end up getting stuck in an infinite loop (depending on loop conditions of course), since the offset will, at some point, no longer be increasing, whereas currently the offset would either be:

 1) decrementing by 1 if the offset is a signed integer, which will almost certainly produce weird/strange/incorrect results and could also potentially lead to an infinite loop in its own right, or 
 2) increasing by a very large value, which will probably cause a mal-formed packet condition the next time data is attempted to be grabbed from the tvb.
 
So, changing the -1 to 0 will correct *some* problems, but definitely not all of them, and for those that it doesn't fix, I guess there's no getting around having to manually fix each one.  Still, I didn't know if fixing *some* of them was enough motivation to make the change anyway?

(And then in addition to that, a great many of these very likely should be changed from tvb_length_remaining() to tvb_reported_length_remaining(), but that's another problem ...)

-- 


CONFIDENTIALITY NOTICE: The information contained in this email message is intended only for use of the intended recipient. If the reader of this message is not the intended recipient, you are hereby notified that any dissemination, distribution or copying of this communication is strictly prohibited. If you have received this communication in error, please immediately delete it from your system and notify the sender by replying to this email.  Thank you.