Abhijit Menon-Sen wrote:
The protocol message that code is decoding contains a four byte signed
integer field. A positive value indicates a number of bytes to follow,
and the special value -1 is used to indicate an SQL NULL value (and no
bytes follow in that case).
What do other negative values indicate? Should they be considered
protocol errors? If so, then simply checking for "l < 0" is an error -
it should check both for "l == -1" (meaning "null value") and, if that
test fails, test for "l < 0" (meaning "the packet is invalid").
It seems to me, then, that the "right fix" would be to make
proto_tree_add_item not segfault on very large values rather than
introducing arbitrary tests here and there to compensate for its
failings.
It *doesn't* segfault on very large values.
However, the length value to "proto_tree_add_item()" (and many other
routines) is signed, not unsigned, because "-1" is a special value
meaning "to the end of the packet".
Therefore, the largest length value that can be passed to it is 2^31 -
1. Larger unsigned values are treated as negative values, so a value of
0xFFFFFFFE, for example, is interpreted as -2, not 4294967294, and is
thus *not* a very large value. (And it doesn't segfault on negative
values; it used to call abort(), and now it puts a "dissector bug"
notification in the protocol tree.)
Perhaps the length field could be made unsigned, with the maximum
unsigned integer value meaning "to the end of the packet", but that
would still mean that a length value of 0xFFFFFFFF wouldn't be handled
correctly - it wouldn't throw a dissector exception, it'd just go to the
end of the packet.
Or perhaps the special -1/"max unsigned int" value could be eliminated,
with a tvb_length_remaining() call being required, but that increases
the number of tvb_length_remaining() calls, which I'd prefer not to do,
as many such calls are inappropriate - it *doesn't* return the amount of
data left in the packet, it returns the amount of *captured* data left
in the packet, which isn't what dissectors should be using in most cases
- and would involve a lot of changes to existing code (especially if
it's eliminated throughout Ethereal).
