Re: [Wireshark-commits] rev 45566: /trunk/epan/dissectors/ /trunk/epan/dissectors/: packet-rdp.c

[Jeff Morriss <jeff.morriss.ws () gmail com>]

Maynard, Chris wrote:
> > -----Original Message-----
> > From: wireshark-dev-bounces () wireshark org [mailto:wireshark-dev-
> > bounces () wireshark org] On Behalf Of Martin Kaiser
> > Sent: Thursday, October 18, 2012 5:11 PM
> > To: wireshark-dev () wireshark org
> > Subject: Re: [Wireshark-dev] [Wireshark-commits] rev 45566:
> > /trunk/epan/dissectors/ /trunk/epan/dissectors/: packet-rdp.c
> >
> > Hi,
> >
> > Thus wrote Maynard, Chris (Christopher.Maynard () GTECH COM):
> >
> > > 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 ...)

Should tvb_length_remaining() not be (mostly) globally replaced with 
tvb_ensure_length_remaining() (which throws an exception when offset is 
out of bounds)?

Maybe:

1) rename tvb_length_remaining() to tvb_length_remaining_no_exception() 
- for those users who really want to get (and can handle) the -1

2) rename tvb_ensure_length_remaining() to tvb_length_remaining()
___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
            mailto:wireshark-dev-request () wireshark org?subject=unsubscribe