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