Wireshark · Wireshark-bugs: [Wireshark-bugs] [Bug 2935] New simulcrypt protocol dissector

[bugzilla-daemon@xxxxxxxxxxxxx]

https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=2935





--- Comment #9 from Bill Meier <wmeier@xxxxxxxxxxx>  2008-10-07 11:15:20 PDT ---
First: Thanks for your effort so far on this dissector !

Some additional comments:

1.   I don't believe the 'if (..._tree)' for the various subtrees is req'd
     since proto_item_add_subtree will always return a non-null value 
     (as long as the parent is non-null)

2.  The following don't appear to be used:
   - mikey_tcp_port
   - proto_register(): 'int i = 0;'

3. pvaluedec is defined as a guint16 and is later calculated 
   as 100 * <a guint16>.
   This seems like a potential for overflow.

4. The 'if (...==-1)' in proto_register.... is not needed.

5. The code to build 'param_value' uses strcat and sprintf which are "memory
unsafe" functions and thus not allowed in Wireshark.

I suggest instead displaying the "param value" fields by using
FT_BYTES and BASE_HEX in the relevant hf[] entries. If you would rather have
the hex strings shown with spaces between each byte, then I guess you'll 
need to build param_value as now but instead maybe 
using g_strlcat & g_snprintf.

See the glib reference for info on g_strlcat &etc 
(http://library.gnome.org/devel/glib/2.16/glib-String-Utility-Functions.html)

(There may be a simpler way which someone else can suggest).

Note: for future reference I suggest reviewing the doc/README.developer
document (if you haven't already done so) which discusses such things as not
using strcat & etc.

6. With respect to proto_reg_handoff:

   As you note in comment #7, the code involving tcp_port and global_tcp port
   is similar to that used in other dissectors. It needs one additional line 
   to be OK:

  if (!initialized) {
    simulcrypt_handle = create_dissector_handle(dissect_simulcrypt, ...
    dissector_add("tcp.port", global_simulcrypt_tcp_port, ...
>>  tcp_port = global_simulcrypt_tcp_port;
    initialized = TRUE;
  }

---

   The code which seems unneeded is:
               
if(!strcmp(tab_ecm_inter[i].protocol_name,CA_SYSTEM_ID_MIKEY_PROTO))
  {
    tab_ecm_inter[i].ca_system_id=ca_system_id_mikey;
    tab_ecm_inter[i].ecmg_port=-1;
  }
datagram_handle=find_dissector(tab_ecm_inter[i].protocol_name);
dissector_delete("tcp.port", tcp_port, datagram_handle);

Let try to explain:
a. As far as I can tell, The simulcrypt dissector never actually registers 
   a handle for tab_ecm_inter[i].protocol_name on tcp_port and thus 
   a dissector_delete is not required. 
   (In actuality, altho the Wirehark documentation may not 
   be clear, all that dissector delete really does is to deregister *whatever*
   handle is currently registered on the specified tcp port; 
   the 'handle' argument is currently ignored).
   So: the find_dissector() ... delete_dissector lines are not required.

b. ecmg_port is being used elsewhere to apparently determine when a
   sub-dissector should be called (find_dissector & etc).

   I would note that I think it's the case that find_dissector need 
   be called only once during proto_reg_handoff for each of the protocols in
   tab_ecm_inter[] (with the handle being stored in another variable 
   in tab_ecm_inter.

   I'm not sure when ecm_port needs to be initialized to -1; maybe for each new
   capture via an init function registered using 'register_init_routine' ? 
   (The registered init function will be called each time a capture file 
   dissection is done).

   Please let me know if I'm missing something


-- 
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.