https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=4703
--- Comment #5 from Bill Meier <wmeier@xxxxxxxxxxx> 2010-04-21 19:25:50 EDT ---
Addt'l comments:
1. #include <stdio.h> , <stdlib.h> & <string.h> aren't needed.
2. FT_BOOLEAN, BASE_NONE, TFS(&multi_dst_strings), TRILL_MULTI_DST_MASK,
should be
FT_BOOLEAN, 16, TFS(&multi_dst_strings), TRILL_MULTI_DST_MASK,
so that the bit field is displayed properly.
3. 'offset' in the following will be different depending upon whether 'tree' is
NULL. (See #6 below).
if( !error ) {
next_tvb = tvb_new_subset( tvb, offset + op_len , -1, -1 ) ;
4. Just do a col_set_str(...,COL_PROTOCOL,...) at the beginning iso a
col_clear.
5. Options:
- If there are no options, then "Options:" shouldn't be shown.
- No need to use proto_item_set_len: just replace the -1 in
proto_tree_add_item with op_len.
ti = proto_tree_add_item( trill_tree, hf_trill_options, tvb,
offset, -1, FALSE ) ;
proto_item_set_len( ti, op_len ) ;
6. In general, the convention is to write dissectors assuming that the packet
contains all the required data. If the data is not all available (either
because of a bogus packet or because the capture was done so as to only capture
some initial part of the packet) any fetches 'off the end' will cause an
exception and show an appropriate error message.
IOW: I don't think length checking needs to be done in the various places as is
being done.
I suggest something like the following might be OK:
op_len = tvb_get_bits(.....) ...
if (tree) {
.... /* add_item, etc for all the TRILL fields */
}
next_tvb = tvb_new_subset(tvb, BIT_FIELDS_LEN + 2*NICKNAME_LEN + op_len, ...)
call dissector(...)
If the packet is short, fetching off the end will cause an exception.
7. Finally: Please fuzz-test the dissector (if you haven't already done so).
See section 1.11 of doc/README.developer.
Thanks
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
