Wireshark mailing list archives
Re: tvb_length_remaining() and tvb_reported_length_remaining()
From: Jaap Keuter <jaap.keuter () xs4all nl>
Date: Sun, 02 Sep 2012 13:12:30 +0200
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
___________________________________________________________________________ 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
Current thread:
- Re: tvb_length_remaining() and tvb_reported_length_remaining() Jaap Keuter (Sep 02)
- Re: [Wireshark-dev] tvb_length_remaining()and tvb_reported_length_remaining() Christopher Maynard (Sep 02)