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.
