Patch to add extension support to the X11 dissector

This issue was migrated from bug 2981 in our old bug tracker.

Original bug information:

Reporter: Peter Harris
Status: RESOLVED FIXED
Product: Wireshark
Component: GTK+ UI
OS: All
Platform: All
Version: Git

Attachments:

extensions.patch.bz2: Patch to add extension support to the X11 dissector
README.X11: How to rebuild X11 extension dissectors
extensions.patch2.bz2: Patch to add extension support, updated to r26786
extensions.patch3.bz2: Patch to add extension support v3: minor bug fix, updated to Wireshark r27564, Mesa mesa_7_1_rc4-8575-g17c7852 and xcb/proto 1.4-1-g8e5a650
gtk.pcap.bz2: Small capture from a GTK app
extensions.patch4.bz2: Patch to add extension support v4: Use U for unreferenced variables/functions, fix infinite loop found by fuzz
extensions.patch5.bz2: Patch to add extension support v5
extensions.patch6.bz2: Patch to add extension support v6
git-version.patch: Patch to add mesa and xcbproto versions to headers

To upload designs, you'll need to enable LFS and have an admin enable hashed storage. More information

Child items 0

No child items are currently assigned. Use child items to break down this issue into smaller parts.

This patch adds extension support to the X11 dissector. I've removed the perl script from the make file, since the new one depends on perl 5.10, xcbproto (at least git as of today), and mesa (at least the mesa/src/mesa/glapi directory). It seemed easier to just add the generated header files to svn directly. If you would rather see this patch the other way around (keep running the generation scripts at 'make' time, and include mesa/glapi and xcbproto in the repository instead of the generated .h files), please let me know.

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

Created attachment 2809 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.

Patch to add extension support to the X11 dissector

Activity