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

[Abhijit Menon-Sen <ams@xxxxxxxx>]

(I've Cc:ed this to ethereal-dev, but I don't know if it'll get there,
since I'm not subscribed any more.)

Hi Joerg, Gerald.

I noticed a few changes to packet-pgsql.c in the 0.10.9 release, which
unfortunately break the dissector. The problem is described below:

  @@ -521,13 +521,14 @@ void dissect_pgsql_fe_msg(guchar type, g
           shrub = proto_item_add_subtree(ti, ett_values);
           n += 2;
           while (i-- > 0) {
  -            l = tvb_get_ntohl(tvb, n);
  +            gint l = tvb_get_ntohl(tvb, n);
               proto_tree_add_int(shrub, hf_val_length, tvb, n, 4, l);
               n += 4;
  -            if (l > 0) {
  +            /* XXX - if we don't limit l here, the function will [...] */
  +            if (l > 0 && l < 1000000)
                   proto_tree_add_item(shrub, hf_val_data, tvb, n, l, FALSE);
  +            if ( l > 0 )
                   n += l;
  -            }
           }
   
           i = tvb_get_ntohs(tvb, n);

The basic problem is that "l" in the code above may be -1 for SQL NULL
values, and thus the change causes n to be _decremented_ in this case,
which did not happen before. Thus, the parsing of subsequent messages
is horribly broken. The problem is repeated elsewhere:

  @@ -652,10 +653,9 @@ void dissect_pgsql_fe_msg(guchar type, g
               l = tvb_get_ntohl(tvb, n);
               proto_tree_add_item(shrub, hf_val_length, tvb, n, 4, FALSE);
               n += 4;
  -            if (l > 0) {
  +            if (l > 0)
                   proto_tree_add_item(shrub, hf_val_data, tvb, n, l, FALSE);
  -                n += l;
  -            }
  +            n += l;
           }
   
           proto_tree_add_item(tree, hf_format, tvb, n, 2, FALSE);

And also:

  @@ -780,10 +780,10 @@ void dissect_pgsql_be_msg(guchar type, g
               l = tvb_get_ntohl(tvb, n);
               proto_tree_add_int(shrub, hf_val_length, tvb, n, 4, l);
               n += 4;
  -            if (l > 0) {
  +            /* XXX - if we don't limit l here, the function will [...] */
  +            if (l > 0 && l < 1000000)
                   proto_tree_add_item(shrub, hf_val_data, tvb, n, l, FALSE);
  -                n += l;
  -            }
  +            n += l;
           }
           break;

Since I do not understand the motivation for the wholesale change from
gint l to guint32 l, I have not appended a patch to fix this problem,
but it would be nice if they were resolved before the next release.

Thank you.
 
-- ams