Wireshark-bugs: [Wireshark-bugs] [Bug 2981] Patch to add extension support to the X11 dissector
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=2981
--- Comment #14 from Jeff Morriss <jeff.morriss.ws@xxxxxxxxx> 2009-03-02 20:08:12 PDT ---
(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.
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
