https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=2935
--- Comment #12 from David Castleford <david.castleford@xxxxxxxxxxxxxxxxxx> 2008-10-09 08:46:36 PDT ---
(In reply to comment #9)
> First: Thanks for your effort so far on this dissector !
>
thanks for your feedback, hope I'm getting there! please see updated diff file
> 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)
>
ok, just followed the examples, removed
> 2. The following don't appear to be used:
> - mikey_tcp_port
> - proto_register(): 'int i = 0;'
>
see new patch, removed a lot of obsolete / unused things
> 3. pvaluedec is defined as a guint16 and is later calculated
> as 100 * <a guint16>.
> This seems like a potential for overflow.
>
corrected, guint32 used now
> 4. The 'if (...==-1)' in proto_register.... is not needed.
>
ok
> 5. The code to build 'param_value' uses strcat and sprintf which are "memory
> unsafe" functions and thus not allowed in Wireshark.
>
removed altogether and used tvb_bytes_to_str, saw in your next comment you
prefer the extra space, be my guest to change if you wish
> 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.
>
done
> 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;
> }
>
corrected, thanks
> ---
>
> 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);
>
don't ask me why, but with the code disabled the ECM_datagram corresponding to
set ca_system_id is not dissected as MIKEY protocol
> 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
>
beyond my grasp, afaik the current code works, please see new diff file and let
me know - if you can remove things and still have it working, fine by me :-)
note that use of ecm_interpretation structure allows a new protocol to be added
to a corresponding ca_system_id (done for a private dissector)
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
