Wireshark-dev: [Wireshark-dev] tvb_length_remaining() and tvb_reported_length_remaining()
From: Christopher Maynard <[email protected]>
Date: Mon, 9 Jul 2012 16:10:53 +0000 (UTC)
Both tvb_length_remaining() and tvb_reported_length_remaining() return -1 to
indicate that the offset parameter is out of bounds, but there are a number of
cases where the return value is assigned to an unsigned integer, or where the
return value is incorrectly tested only for non-zero or simply not checked at 
all.

This can lead to very bad things.  For example:
Coverity CID 281367: (/asn1/c1222/packet-c1222-template.c:910)

guint32 len2;

len2 = tvb_length_remaining(tvb, offset);
if (len2 <= 0)
  return offset;
buffer = tvb_memdup(tvb, offset, len2);


While it's easy enough to fix this bug by declaring len2 as a gint instead of as
a guint32, I was wondering if it might possibly be better to change the behavior
of tvb_length_remaining() and tvb_reported_length_remaining() to return 0
instead of -1?  This would obviously lead to other problems though, such as code
that might only test for "tvb_length_remaining() < 0" for example.

Would replacement functions help here which would return an out-of-bounds error
in one of their arguments, so that things would look instead something like:

guint32 len2;
gint error;

len2 = tvb_length_remaining_new(tvb, offset, &error);
if (error == TBD)
  return offset;
buffer = tvb_memdup(tvb, offset, len2);

- Chris