https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=4118
--- Comment #4 from Bill Meier <[email protected]> 2009-10-13 14:42:45 PDT --- Thanks for your effort on the dissector. The following comments based upon a quick "first pass" review... - The dissector needs a $Id$ near the top (See doc/README.developer); - All fcns other than proto_register & proto_reg_handoff should be static; - Not req'd: void proto_register_packetbb(); void proto_reg_handoff_packetbb(); /* Causes Compiler warning on Windows */ /* due to missing (void). */ /* No matter since the forward ref */ /* is not needed. */ - In dissect_pbb_tlvblock: indexEnd = addrCount; gives Compiler warning on Windows: conversion from 'gint16' to 'guint8', possible loss of data - proto_register & proto_reg_handoff should be moved to the end of the file. - C++ comments should be changed to C style; - hf[]: "blurb" fields with "" should be replaced by NULL; - Please use tfs_true_false (see epan/tfs.h) rather than your own version of same. - if (proto_packetbb == -1) not req'd; - proto_reg_handoff: Code looks OK but it would be appreciated if you could use the "standard idiom" for handling port-delete/port-add. see packet-agentx.c for an example. (Also: packetbb_handle can/should be local to proto_reg-handoff...) - check_col is no longer req'd around col_set_str and col_clear - COL_INFO never gets filled with anything ....: Is this as intended ??? - In general: I think The "too many octets"/"not enough octets" type situations may be better handled by generating a "Malformed" Expert message & etc: However: I need to think a bit about what might be the best way to do this in this dissector. So: Let's leave this for a 2nd review. (or Maybe someone else will have a suggestion). - Don't use tvb->length & etc. The convention is to call one of the tvb_... fcns; In most cases: tvb_reported_length(...): Using tvb_reported_length means that the case of a "capture with truncated frames" will be handled properly with the correct message displayed if the dissector tries to access a "valid" protocol field which happens to not be present because the capture limited the length of the each frame captured. - ti = proto_tree_add_item(tree, proto_packetbb, tvb, 0, tvb->length, FALSE); use -1 for length ("from offset to end") not tvb->length - dissect_pbb_addressblock I'm a bit nervous about all the tvb_get_ptr/memcpy stuff: However: let's leave this for a 2nd review. Why is address[255] static ? if (maxoffset - offset < head_length): what if head_length==0: Will this work ?? - Has this dissector been fuzz-tested with the test .pcap file ?? -- Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the assignee for the bug. ___________________________________________________________________________ Sent via: Wireshark-bugs mailing list <[email protected]> Archives: http://www.wireshark.org/lists/wireshark-bugs Unsubscribe: https://wireshark.org/mailman/options/wireshark-bugs mailto:[email protected]?subject=unsubscribe
