Home page logo
/

wireshark logo Wireshark mailing list archives

Re: Bug in expert_add_info_format with a NULL pi parameter
From: mmann78 () netscape net
Date: Fri, 18 Jul 2014 20:27:25 -0400 (EDT)


Yes "expert info" APIs (even the deceiving proto_tree_add_expert and proto_tree_add_expert_format) need to be called 
independent of the presence of a tree (just like column APIs).  It can sometimes be difficult to detect that expert API 
calls are "hidden" under an if (tree) call,
 
 
 
-----Original Message-----
From: Peter Wu <peter () lekensteyn nl>
To: 'Developer support list for Wireshark' <wireshark-dev () wireshark org>; Hauke Mehrtens <hauke () hauke-m de>
Sent: Fri, Jul 18, 2014 7:20 pm
Subject: Re: [Wireshark-dev] Bug in expert_add_info_format with a NULL pi parameter


On Saturday 19 July 2014 00:33:26 Peter Wu wrote:
Hi,

While working on refactoring the SSL dissector[1], I noticed that
expert_add_info_format(pinfo, NULL, ...) does not add an expert item to the
tree view. In the case of the SSL dissector, the NULL should be replaced by
the proto item to which the message is related, but with pi = NULL, an item
should still be added somewhere, right? See attached capture. It should add
an expert warning with "Cipher suite length (1) must be a multiple of 2".
Note that in the current implementation, a text label is also added.

This is probably a bug, but the documentation says this:

/** Add an expert info.
 Add an expert info tree to a protocol item, using registered expert info
item,
 but with a formatted message.
 @param pinfo Packet info of the currently processed packet. May be NULL if
        pi is supplied
 @param pi Current protocol item (or NULL)
 @param eiindex The registered expert info item
 @param format Printf-style format string for additional arguments
 */
WS_DLL_PUBLIC void
expert_add_info_format(packet_info *pinfo, proto_item *pi, expert_field
*eiindex,
      const char *format, ...) G_GNUC_PRINTF(4, 5);


I have confirmed (via a gdb breakpoint) that pinfo is never NULL. Any idea
what is going on here?

Upon further investigation, it seems to be a bug in the SSL dissector. With a 
NULL pi, the expert item is not added to the tree view (which is fine). But 
due to a bug, it is also not available in the expert info overview.

By removing the `if (tree || ssl)` from ssl_dissect_hnd_hello_common and 
ssl_dissect_hnd_cli_hello (after applying patch up to change 3021), the expert 
info is displayed again (because the offset is calculated correctly), but only 
when "Restrict to display filter" is enabled. When that checkbox is not 
checked, the expert item still disappears.

In dissect_ssl3_record, dissection of the individual messages is skipped if 
the tree is not available:

  1915         /* PAOLO: if we are doing ssl decryption we must dissect some 
requests type */
  1916         if (ssl_hand_tree || ssl)

By adding '|| 1` here, the expert info is always added, as expected. I can 
imagine the overhead of decryption, but this will break all expert info fields 
in WS operating in the record layer. For now I will remove the `if 
(ssl_hand_tree || tree)` optimization I think.

Peter

Kind regards,
Peter
https://lekensteyn.nl

 [1]: https://code.wireshark.org/review/3021/

-- 
Kind regards,
Peter
https://lekensteyn.nl
___________________________________________________________________________
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

 
___________________________________________________________________________
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 ]