Home page logo

wireshark logo Wireshark mailing list archives

Re: [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 ...
From: "Maynard, Chris" <Christopher.Maynard () GTECH COM>
Date: Mon, 9 Dec 2013 20:44:36 -0500

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 () wireshark org [mailto:wireshark-dev-bounces () wireshark org] 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?


On Mon, Dec 9, 2013 at 5:23 PM,  <cmaynard () wireshark org> wrote:

User: cmaynard
Date: 2013/12/09 10:23 PM

 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.
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe

  By Date           By Thread  

Current thread:
[ Nmap | Sec Tools | Mailing Lists | Site News | About/Contact | Advertising | Privacy ]