Wireshark-bugs: [Wireshark-bugs] [Bug 3443] A new dissector for the DTN Bundle Protocol
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=3443
--- Comment #6 from Jaap Keuter <jaap.keuter@xxxxxxxxx> 2009-05-11 22:40:20 PDT ---
Quick review.
For all details see doc/README.developer doc/README.malloc etc.
Never need to #include <gmodule.h>
Please remove
What with the FILTER_ON_EID? Does it need to stay in the code?
What with the OLD_CONVERGENCE? Does it need to stay in the code?
What with the DMCV99? Does it need to stay in the code?
What with the USING_3RD_OPTION? Does it need to stay in the code?
Never a need to put in: G_MODULE_EXPORT const gchar version[] = "0.0";
Please remove.
To avoid namespace collisions header field filter names all must have the
format: "PROTOABBREV.FIELDABBREV". Yours are missing the PROTOABBREV part.
When FIELDDESCR is the same as FIELDNAME you can use NULL i.s.o. ""
Flags are usually shown as FT_BOOLEAN fields.
+ {&hf_contact_hdr_flags_ack_req,
+ {"Bundle Acks Requested", "contact_hdr.flags.ackreq",
+ FT_UINT8, BASE_DEC, NULL, TCP_CONV_BUNDLE_ACK_FLAG, "", HFILL}
would become
+ {&hf_contact_hdr_flags_ack_req,
+ {"Bundle Acks Requested", "bundle.contact_hdr.flags.ackreq",
+ FT_BOOLEAN, 8, NULL, TCP_CONV_BUNDLE_ACK_FLAG, NULL, HFILL}
plugin_register() and plugin_reg_handoff() need not included.
Please remove
This kind of code makes me very nervous. Example:
+ sdnv_ptr = (guint8 *) tvb_get_ptr(tvb, offset, 2);
+ control_flags = eval_sdnv(sdnv_ptr, &sdnv_length);
There is *no* guarantee that eval_sdnv() won't parse past the end of the TVB,
even though it's checked for the length you *expect* it to have (2 in this
case). A malformed byte stream may still cause you to read past the end of the
buffer. Always try to use the TVB access functions!
The proper way to implement this is to pass the TVB and the offset into
eval_sdnv() and use tvb_get_guint8(tvb, offset) to get the bytes *safely* out
of the buffer.
+ sptr = (char *) malloc(eid_length + 1);
+ snprintf(sptr, eid_length + 1, "%s", eid_ptr); /*Adds the NULL*/
+ proto_item_set_text(eid_item, "Local EID: %s", sptr);
+ free(sptr);
Don't mix memory handlers, use the appropriate versions for these please:
malloc -> ep_alloc
snprintf -> g_snprintf
free -> dropped (garbage collected)
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
