Wireshark · Wireshark-bugs: [Wireshark-bugs] [Bug 2981] Patch to add extension support to the X11 dissector

[bugzilla-daemon@xxxxxxxxxxxxx]

https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=2981





--- Comment #21 from Peter Harris <peter.harris@xxxxxxxxxxxxxxx>  2009-09-10 10:09:17 PDT ---
(In reply to comment #20)
> Reading more code, I have concerns about:
> 
> +static void struct_TRAP(tvbuff_t *tvb, int *offsetp, proto_tree *root, int
> little_endian, int count)
> +{
> +    int i;
> +    for (i = 0; i < count; i++) {
> +          proto_item *item;
> +       proto_tree *t;   
> +
> +          item = proto_tree_add_item(root, hf_x11_struct_TRAP, tvb, *offsetp,
> 24, little_endian);
> +          t = proto_item_add_subtree(item, ett_x11_rectangle);
> +          struct_SPANFIX(tvb, offsetp, t, little_endian, 1);
> +          struct_SPANFIX(tvb, offsetp, t, little_endian, 1);
> +    }
> +}
> 
> Note how there's a 'count' but the offset never increases.

The offset increases inside struct_SPANFIX; offsetp is a pointer to the offset.

> struct_Event() has the same problem.

The UNUSED() macro (already defined in packet-x11.c, not in this patch)
increments the offset.

> In dispatch_composite() (and other dispatch functions) there's no default case
> in the switch statement. Should we tell the user "unrecognized minor number" or
> something?  Actually, most of the switch statements don't have default cases. 
> Is that intended?

The user will be told "Unknown opcode %d" already by col_append_fstr before the
switch. It was a while ago, but I think I intentionally omitted the default
case because telling the user the same thing again would be redundant. The only
thing left to do is mark the bits UNDECODED(), but dissect_x11_request already
does that for me.

> What's the point of code like:
> 
> +    f_grab_window = VALUE32(tvb, *offsetp);
> +    proto_tree_add_item(t, hf_x11_xinput_GrabDeviceKey_grab_window, tvb,
> *offsetp, 4, little_endian);
> 
> Why pull the value from the tvb if it's not going to be used?  In this function
> one of the values _is_ used, but only one.  I guess it just doesn't know which
> value will be used until it gets to the end of the function?  Ah, maybe that's
> this comment?
> 
> +#TODO
> +# - look ahead to see if values are ever used again before creating an "int"
> in the output

Yes, that's it exactly.

> You can remove all the calls to check_col()--it's no longer needed.

Okay.

> The hf_ entries need to have the empty strings ("") replaced with NULL:
> 
> +{ &hf_x11_above_sibling, { "above-sibling", "x11.above-sibling", FT_UINT32,
> BASE_HEX, NULL, 0, "", HFILL }},

Hmm? Oh, I see. process-x11-fields.pl has changed since then. I'll fix that for
v6.

> When I look in the sample captures provided, I see that the 'opcode' doesn't
> get decoded right in the tree:
> 
> : X11, Request, opcode: 140 (XKEYBOARD)
> :     opcode: Unknown (140)  <<<< here
> :         undecoded
> 
> The top line decodes the opcode correctly because it's using
> 'state->opcode_vals', but the hf_ entry is still using 'opcode_vals'.  Can
> anything be done about that?

The hf_ entry is statically declared, so overwriting it is a no-go. I suppose
we could move to a dynamically allocated list of hf_entries, or just output
that one line by hand (not using an hf_ entry).

I was trying to touch the existing dissector as little as possible. Would you
like me to look into this for v6 of the patch?

> Looking at some QueryExtension packets, I noticed that major-opcode doesn't
> have VALS entry (meaning that you only see the value, not the name of the
> opcode)--could/should it?

Maybe it could/should. Again, I was trying to touch the existing dissector as
little as possible. Would you like me to look into that for v6 of the patch?

>  Looks like you've done good work here.

Thanks.


-- 
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.