Hi,
Two problems here.
The use of _new in a (function) name is to be deprecated. Yes it's new at this
moment in time, but after a year or two people start to wonder, "what's new
here, it's been here for ages?". And if an even newer feature would be thought
of, what do you name it then? _newer, _even_more_new, _latest_new ?
Call it what it is: tvb_length_remaining_error() or something. BTW: _ex is as
bad as _new (sorry M$).
But the real issue is that now you've move the problem to checking the return
value error, which absence may not even be detected by Coverity / QA tooling.
Basically it comes down to: check your functions nul return value, whatever it
is. That's a reviewers job, with Coverity's help.
Thanks,
Jaap
On 07/09/2012 06:10 PM, Christopher Maynard wrote:
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