We often keep the scripts used to generate files in our source tree, but if there are various programs required to generate the files, I would say we can keep a skeleton of code in the tree then have a document that explains how to get the programs required to generate the code.

This sounds similar to our ASN.1 generated dissectors.  When we make a change, we check in the updated code generator in asn1/<dissector name>/ and then we run it again locally and check in the updated files at the same time.

Created attachment 2492 [details]
How to rebuild X11 extension dissectors

Created attachment 2493 [details]
Patch to add extension support, updated tor26786

If I understand correctly, the only thing really missing from the patch is a README file describing the exact steps needed to rebuild the files.

I've attached just such a README, as well as including it in an updated version of the patch.

If there's anything else needed, please let me know.

Comment on attachment 2493 [details]
Patch to add extension support, updated tor26786

Mark for review

A couple of questions:

- has this been fuzz tested?
- Can you supply a sample capture (either here or on the Wiki SampleCaptures page) for testing?

Created attachment 2792 [details]
Patch to add extension support v3: minor bug fix, updated to Wiresharkr27564, Mesa mesa_7_1_rc4-8575-g17c7852 and xcb/proto 1.4-1-g8e5a650

I hadn't previously fuzz-tested this patch. It passed a couple of hours with the one capture file I have handy; I'll do some more fuzz-testing with a wider variety of captures when I get to the office tomorrow.

It didn't occur to me that anyone would need a sample capture until you asked; I forgot that some people still use Windows. While looking at this quick capture, I noticed a couple of minor errors. Fixed in version 3 (along with updating the upstream XML protocol descriptions to their latest versions).

Created attachment 2793 [details]
Small capture from a GTK app

Small capture from a GTK app; mostly demonstrates the RENDER extension.

I'll add a mostly-GLX capture when I get to the office tomorrow.

(In reply to comment #7)
> It didn't occur to me that anyone would need a sample capture until you asked;
> I forgot that some people still use Windows. While looking at this quick
> capture, I noticed a couple of minor errors. Fixed in version 3 (along with
> updating the upstream XML protocol descriptions to their latest versions).

In fact I generally use X for most of my work (using Exceed in some cases ;-)) but I figured you might have sample captures with some of the more "exotic" extensions (if such things exist--I use X but don't pay much attention to how it works).

(It also occurs to me after some reflection that most distros install X with "-nolisten tcp" by default, which makes it a pain to capture X)

The most 'exotic' (mostly obsolete) extensions aren't covered by XCB yet, so there wouldn't be any dissection to test. :-) But, yeah, pretty much everything draws via RENDER these days, which makes core-X-only dissection less useful.

I'll see what I can dig up at the office. I should be able to capture a reasonable variety, although complete coverage isn't likely.

No need to cover everything, though the more the better--especially so it can be used for reference.  (Maybe you could put the captures on the Wiki's SampleCaptures page, too?)

Regarding the patch, I think it mostly looks good.  I didn't review the Perl script but the code it puts out looks sane.  One exception is the code coming out of:

+    # The length parameter may be unused. Shut up compiler warning.
+    # (Worse than that, since wireshark uses -Werror)
+    print $impl "    (void)length;\n";
+    if (!@elements) {
+       print $impl "    (void)tvb;\n";
+	   print $impl "    (void)offsetp;\n";
+	   print $impl "    (void)t;\n";
+	   print $impl "    (void)little_endian;\n";
+    }

The preferred method is to append _U_ to the unused parameter.  In the case of 'tvb' and the others, this can be done since you know when they won't be used.  'length' is problematic because you don't really know if it will be used or not.  I'm a little uncomfortable with the code as it is--I have this vague memory of some compilers warning about statements like that (something about statements with no effect?).

Is it safe to mark a parameter with _U_ even if it may be used?  Another way would be to simply assign the variable to itself.  I forget what's the preferred method (assuming that we want to keep this dissector clean).

(In reply to comment #11)

> Is it safe to mark a parameter with _U_ even if it may be used?

Yes.  A comment mentioning that it may be used would be helpful though.  We have some generated dissectors (ASN.1?) that just mark everything as unused and use what they need if I remember correctly.

Created attachment 2799 [details]
Patch to add extension support v4: Use _U_ for unreferenced variables/functions, fix infinite loop found by fuzz

Ah, I was unaware that Wireshark had the _U_ define. Thanks! Patch updated.

I've uploaded a half a dozen .pcap files to the Wiki's SampleCaptures page. Fuzz testing over those files turned up an infinite loop in GLX/Render when the length field on the wire is invalid. Fixed in v4 of the patch.

I did double-check the definition of _U_. It only appears to be defined for gcc. The attribute is described as useful for C++ objects where the constructor and destructor are important, but the object is otherwise never used, so it sounds safe to use it on variables/functions that are actually referenced.

v4 also uses "static _U_" for a few functions that were previously non-static. This is a good thing: it cuts down on namespace pollution, and it should (in theory) allow the linker to eliminate a few of the functions (except on Windows, where it could already toss unreferenced functions, due to Windows forcing the equivalent of -fvisibility=hidden).

I'm a little queasy about -Wall -Werror -- some day, gcc will start warning about something else (eg: non-system defines of reserved names (such as those that start with an underscore followed by a capital letter)), and the build will error. But that's beyond the scope of this particular patch.

(In reply to comment #13)
> I've uploaded a half a dozen .pcap files to the Wiki's SampleCaptures page.
> Fuzz testing over those files turned up an infinite loop in GLX/Render when the
> length field on the wire is invalid. Fixed in v4 of the patch.

Excellent.  But I don't think this is a good way to fix it:

+ 	len = VALUE16(tvb, *offsetp);
[...]
+       if (len < 4) { 
+           REPORT_DISSECTOR_BUG("Invalid glRender length");
+       }   
+       len -= 4;

It's not a dissector bug that the packet is malformed.  You should either return or force an exception to be thrown (by trying to read beyond the end of the tvb).

> v4 also uses "static _U_" for a few functions that were previously non-static.

I assume those functions are unused because xcb or whatever defines the structures but doesn't use them?  [Since this is generated code] why not just leave them out of the dissector until they become used?

> I'm a little queasy about -Wall -Werror -- some day, gcc will start warning
> about something else (eg: non-system defines of reserved names (such as those
> that start with an underscore followed by a capital letter)), and the build
> will error. But that's beyond the scope of this particular patch.
 
It should be noted that we don't ship Wireshark with -Werror turned on.  It's just there to keep developers honest between releases.  So far turning it on has been a Good Thing in many ways.


In continuing to review the code, I have a few more comments:

- why abuse value_string?

+static value_string bigreq_reply_funcs[] = {
+    {0, (char *)bigreqEnable_Reply },
+    { 0, NULL }
+};
[...]
+    set_handler("BIG-REQUESTS", dispatch_bigreq, bigreq_errors, bigreq_events, bigreq_event_funcs, bigreq_reply_funcs);

It would be nicer to just define your own structure to hold pointers to functions.

- (Minor, but...) since this is generated code, why not get the indenting right?

+    for (i = 0; i < count; i++) {
+ 	proto_item *item;
+       proto_tree *t;
+    int f_root;

(Not sure if that'll come out in HTML, but the 'int' line is indented 4 spaces but the proto_* are indented with a tab--8 spaces.)

I may have more comments later--there's a lot of code here.

(In reply to comment #14)
> (In reply to comment #13)
> > I've uploaded a half a dozen .pcap files to the Wiki's SampleCaptures page.
> > Fuzz testing over those files turned up an infinite loop in GLX/Render when the
> > length field on the wire is invalid. Fixed in v4 of the patch.
> 
> Excellent.  But I don't think this is a good way to fix it:
...
> +           REPORT_DISSECTOR_BUG("Invalid glRender length");
 
> It's not a dissector bug that the packet is malformed.  You should either
> return or force an exception to be thrown (by trying to read beyond the end of
> the tvb).

I'd like to throw an error, so it shows up in red in wireshark. I need to read past the end of the tvb to do so? Ick.

I'll make this change for v5.

> > v4 also uses "static _U_" for a few functions that were previously non-static.
> 
> I assume those functions are unused because xcb or whatever defines the
> structures but doesn't use them?

Well, xcb does use them - in the core protocol. I wanted to leave the core protocol dissector alone for now.

>  [Since this is generated code] why not just
> leave them out of the dissector until they become used?

Right now, I process the protocol description in a streamy and (mostly) stateless manner. Changing the generator to emit structures lazily would be a rather large change.

> - why abuse value_string?
...
> It would be nicer to just define your own structure to hold pointers to
> functions.

Yes, it would. I thought I was using value_string lookup functions, but it appears not. I'll make this change for v5.

> - (Minor, but...) since this is generated code, why not get the indenting
> right?

I didn't do this initially because this is generated code - nobody should be editing it by hand anyway.

To fix up the indenting I'll have to pass an extra parameter around (since the same function generates code at different indent levels).

I'll make this change for v5.

> I may have more comments later--there's a lot of code here.

Thank you for your review. The code is much better already.

(In reply to comment #15)
> I'd like to throw an error, so it shows up in red in wireshark. I need to read
> past the end of the tvb to do so? Ick.

You can use expert_add_info_format() to add an error message to an item.
Have a look in epan/expert.h for various options.

(In reply to comment #16)
> You can use expert_add_info_format() to add an error message to an item.
> Have a look in epan/expert.h for various options.

Thanks, that's exactly what I was looking for.

Created attachment 2809 [details]
Patch to add extension support v5

v5 of the patch has the following changes from v4:
 - reports malformed GLX/Render subrequest sizes via expert_add_info_format instead of REPORT_DISSECTOR_BUG
 - uses custom structures instead of abusing value_string
 - indents generated code for 'struct' functions properly.
 - implements xcb <union> support, prompted by:
 - omits unused structure dissectors via the brute-force method of blacklisting the ones that are never used.

It would be really nice if this could get merged at some point (it missed 1.2 http://www.mail-archive.com/wireshark-dev@wireshark.org/msg13812.html), since without extension support many X11 traces are so sparsely decoded as to being useless. (think RENDER, GLX, etc.).

(In reply to comment #18)
>  - indents generated code for 'struct' functions properly.

Cool; I hope that wasn't too much of a pain, it was just a minor complaint.

>  - implements xcb <union> support, prompted by:
>  - omits unused structure dissectors via the brute-force method of blacklisting
> the ones that are never used.

Hmmm, I wonder if I misunderstood something.  I saw some functions marked as "static _U_ void" so I thought the script knew what wasn't being used.  But, now that the brute-force code is there...


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 loop will end eventually but it could take a long time (since no exception will be thrown when the offset gets too big for the TVB).  Should there not be a count or should there be an offset increment (like there is elsewhere)?  struct_Event() has the same problem.


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?

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


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

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 }},



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?


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?

Anyway, I think that's about it.  Sorry for all the delay(s).  Looks like you've done good work here.

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

(In reply to comment #21)
> > 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.

Ah, good point. OK.

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

OK.  Maybe a comment would be in order in case someone in the future raises the same question?

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

OK

> > 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?

No, I guess there's nothing clean that can be done about that.  I just figured I'd mention it.

> > 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?

Not a big deal, I just noticed it.  You're the one using the dissector, so whatever is useful for you.


Looking over the changes for V6, it looks like trivial stuff (check_col, the NULL strings in the hf_ entries, and maybe the default-case comment).  I could fix that stuff tonight if you don't beat me to it.

Created attachment 3641 [details]
Patch to add extension support v6

Okay, the opcode line was already special-cased, so it was relatively straight-forward to special-case it a little bit more. QueryExtension-reply-major_opcode isn't already special-cased, so it would be more intrusive to fix. I don't believe it's that critical, since the QueryExtension-request will be nearby in the protocol stream.

Otherwise, v6 does the trivial stuff (check_col, NULL instead of "" in hf_ entries, and the default-case omission comment). Plus it was rebased ontor29841, and uses the latest Mesa and XCB/proto upstreams.

Comment on attachment 3641 [details]
Patch to add extension support v6

Committedrev 29854.

Patch committed with some minor changes (mainly adding more info--including the SVN version and copyright--to the now-checked-in header files).  Thanks for your patience and work.

(BTW, in the course of doing that, I did regenerate the dissector based on the latest version of mesa and xcbproto.)

That made me wonder: would it be possible/would it make sense to include the git version of mesa and xcbproto which were used to generate the dissector in, say, the header of the generated files?  Just so we have an idea what the source used was?

Or put it in the tools directory? (or sub dir).

Created attachment 3645 [details]
Patch to add mesa and xcbproto versions to headers

You're right, something like the attached (to add mesa and xcbproto version to generated/checked-in headers) could be very useful.

(In reply to comment #26)
> Or put it in the tools directory? (or sub dir).

You mean the 2 process-* scripts?  Makes some sense.  Could even add a Makefile target to generate the stuff (which, like the ASN.1 dissectors, doesn't get run as part of the build process).

I'll look into that, maybe this weekend.

Comment on attachment 3645 [details]
Patch to add mesa and xcbproto versions to headers

Committedrev 29865 (with a small change to not use 'which'), thanks!