Wireshark · Wireshark-dev: [Wireshark-dev] tvb_length_remaining() and tvb_reported_length_remaining()

[Christopher Maynard <Christopher.Maynard@xxxxxxxxx>]

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