Wireshark-bugs: [Wireshark-bugs] [Bug 3922] TN5250 Dissector
Date: Thu, 17 Sep 2009 18:46:01 -0700 (PDT)

Jeff Morriss <[email protected]> changed:

           What    |Removed                     |Added
                 CC|                            |[email protected]

--- Comment #2 from Jeff Morriss <[email protected]>  2009-09-17 18:45:58 PDT ---
First: Holy bitmasks, Batman!  I'm bet you're glad proto_tree_add_bitmask()
exists... :-)

Second, checkhf.pl found a few hf_ entries that aren't used (among many false
positives).  How I found the real problems was to look for entries checkhf
found that only had 2 references in the file:

$ for hf in $(tools/checkhf.pl epan/dissectors/packet-tn5250.c |awk '{print
$4;}'); do printf "$hf "; grep -c $hf epan/dissectors/packet-tn5250.c;
done|grep 2$
hf_tn5250_res_twobytes 2
hf_tn5250_wssf_cc_flag1_7 2
hf_tn5250_character_code 2
hf_tn5250_stop_address 2
hf_tn5250_dpt_id 2
hf_tn5250_wdsf_class 2
hf_tn5250_null 2
hf_tn5250_wdsf_type 2

Third, I saw a comment saying something about checking for memory leaks.  Well,
I found two:

+    if (tn5250_info == NULL) {
+      /* No.  Attach that information to the conversation, and add
+      * it to the list of information structures.
+      */
+      tn5250_info = g_malloc(sizeof(tn5250_conv_info_t));
+      COPY_ADDRESS(&(tn5250_info->outbound_addr),&(pinfo->dst));

The g_malloc()'d memory is never freed (e.g., when the file is closed) and
neither is the address memory (COPY_ADDRESS allocates memory too).  Look at
other dissectors who use the conversation API on how/when to do this.

Fourth, the reliance on tvb_offset_exists() and tvb_length_remaining() mean
that this dissector won't throw exceptions if the packets are cut short because
a snaplen was used when capturing.  I suppose a tvb_reported_offset_exists()
could be written, but another way would be to simply check if
tvb_reported_length_remaining() was 0.

Lastly (since you provided a patch to Makfile.common): the header needs to be
in DISSECTOR_INCLUDES.  (Not that that this thing really needs a header file
other than to split up the 7kloc.)

Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.