Wireshark · Ethereal-dev: [Ethereal-dev] Re: packet-pgsql.c changes in 0.10.9

[Abhijit Menon-Sen <ams@xxxxxxxx>]

At 2005-03-05 19:15:07 -0800, gharris@xxxxxxxxx wrote:
>
> >    guint32 l = tvb_get_ntohl( ... );
> >
> >    if ( l > 0 && l < 1000000 )
> >        ...
> >    n += l;
> >
> >And that last increment should be done only if l > 0.
> 
> If l is 0, "n += l" isn't a problem; the only problem is if l >= 1000000.

Let me rephrase.

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).

Thus, the problem with the "n += l" is if l < 0 (which, in my original
code that declared it a gint, it could be). Again, I do not understand
the motivation behind the wholesale change from gint to guint32 in my
original code.

> The right fix for that is to get rid of the test against 1000000, and
> to, instead, use "tvb_ensure_bytes_exist()" before dissecting

That test (and the resulting bug I reported) was introduced to fix a
segfault ("XXX - if we don't limit l here, the function will assert
on very large values") by this change:

<http://anonsvn.ethereal.com/viewcvs/viewcvs.py/trunk/epan/dissectors/packet-pgsql.c?rev=12833&r1=12800&r2=12833>

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.

> "tvb_ensure_bytes_exist()" doesn't give a special meaning to a
> negative length, it just assumes that a negative length really
> means "very large positive length" and throws the appropriate
> exception.

There is no exception appropriate to the perfectly valid value of -1.

-- ams