I don't think it can be handled in one place (but I could be wrong). A significant number of dissectors don't use the data parameter at all, so it's perfectly acceptable for it to be NULL. And while most dissectors that do expect data either can't deal with a NULL parameter or don't make any effort to deal with that case, there were some dissectors that did, such as packet-btl2cap.c (see http://anonsvn.wireshark.org/viewvc?revision=53784&view=revision).
I started looking at this to try to avoid more bugs/crashes like https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=9482. In this case, the crash was the result of fuzz testing, where the Fibre Channel dissector would not normally have been called in the first place, so I don't think there's really anything else that could be done except for adding the extra checks.
- Chris
-----Original Message-----
From: wireshark-dev-bounces@xxxxxxxxxxxxx [mailto:wireshark-dev-bounces@xxxxxxxxxxxxx] On Behalf Of Evan Huus
Sent: Monday, December 09, 2013 6:19 PM
To: Wireshark Developer List
Subject: Re: [Wireshark-dev] [Wireshark-commits] rev 53895: /trunk/ /trunk/asn1/disp/: packet-disp-template.c /trunk/epan/dissectors/: packet-disp.c packet-dop.c packet-dsp.c packet-hci_usb.c packet-p1.c packet-pw-atm.c packet-rfid-pn532-hci.c ...
Should all of these null checks be handled in one place (like call_dissector_through_handle or something)?
Are there specific dissectors where it's valid for data to be NULL?
Even if there are, is it simply less work at this point to pass them a pointer to an empty struct or some such thing?
Evan
On Mon, Dec 9, 2013 at 5:23 PM, <cmaynard@xxxxxxxxxxxxx> wrote:
> http://anonsvn.wireshark.org/viewvc/viewvc.cgi?view=rev&revision=53895
>
> User: cmaynard
> Date: 2013/12/09 10:23 PM
>
> Log:
> Reject the packet if data is NULL without doing anything else.
>
> Note: We *might* want to do _something_ but that _something_ should be well-defined and consistent across all dissectors. Previously, some dissectors called proto_tree_add_text() to add some error message text to the tree, while others called DISSECTOR_ASSERT().
>
> Directory: /trunk/asn1/disp/
> Changes Path Action
> +4 -10 packet-disp-template.c Modified
>
> Directory: /trunk/epan/dissectors/
> Changes Path Action
> +7 -13 packet-disp.c Modified
> +8 -14 packet-dop.c Modified
> +8 -14 packet-dsp.c Modified
> +6 -3 packet-hci_usb.c Modified
> +8 -14 packet-p1.c Modified
> +12 -4 packet-pw-atm.c Modified
> +6 -3 packet-rfid-pn532-hci.c Modified
> +6 -3 packet-rfid-pn532.c Modified
> +7 -12 packet-ros.c Modified
> +7 -13 packet-rtse.c Modified
>
>
> (6 files not shown)
CONFIDENTIALITY NOTICE: The information contained in this email message is intended only for use of the intended recipient. If the reader of this message is not the intended recipient, you are hereby notified that any dissemination, distribution or copying of this communication is strictly prohibited. If you have received this communication in error, please immediately delete it from your system and notify the sender by replying to this email. Thank you.