Wireshark · Ethereal-dev: Re: [Ethereal-dev] IP defragment

[Guy Harris <guy@xxxxxxxxxx>]

> please consider for cvs

The code that does

	ipfk->srcip = *((guint32 *)pinfo->src.data);
	ipfk->dstip = *((guint32 *)pinfo->dst.data);

may crash on platforms with strict memory alignment requirements (and,
in fact, *did* crash on my SPARC/Solaris/GCC platform at work on at
least one capture file); do

	memcpy(&ipfk->srcip, pinfo->src.data, sizeof ipfk->srcip);
	memcpy(&ipfk->dstip, pinfo->dst.data, sizeof ipfk->dstip);

instead.

Note also that "g_malloc()" is defined to abort the application if you
run out of memory:

	http://developer.gnome.org/doc/API/glib/glib-memory-allocation.html

"Note: If any call to allocate memory fails, the application is
terminated. This also means that there is no need to check if the call
succeeded."

so it shouldn't check for "g_malloc()" returning NULL - it never returns
NULL.

You also need to save and restore some of the members of "pi" before
calling a subdissector, so that old-style (non-tvbuffified) dissectors
work correctly.

In addition, given that the reassembly code checks the IP checksum, you
have to compute it *regardless* of whether the "tree" argument is null
or not.

I've attached a patch to the "packet-ip.c" you sent out, which includes
those fixes.

Another thing you'll need to do is to arrange that if not all the data
in a fragment is present - i.e., if "tvb_reported_length(tvb)" is
creater than "tvb_length(tvb)" - you don't add it to the fragment hash
table.  This can happen if the capture was done with a snapshot length
less than the MTU of the network; Ethereal/Tethereal, snoop, and
Microsoft Network Monitor default to a large or "infinite" snapshot
length, so, at least with those programs, it'll happen only if the user
explicitly specifies a smaller snapshot length, but the default snapshot
length in tcpdump is 68 bytes, so it'll happen with tcpdump *unless* the
user specifies a larger snapshot length.

> * multiple-tails, overlap-conflict and too-long-fragmnet will also set
> "ip.fragments.error" which is easier to use in a
> display-filter.

Perhaps what we need here is either

	1) a generic "error" flag that can be set on a frame, with an
	   expression such as "error" checking it

or

	2) a way of marking a protocol tree item as an error indication,
	   which could not only be filtered for with a special
	   expression such as "error" but could perhaps also be made a
	   different color (defaulting, perhaps, to the standard color,
	   but allowing the user to specify a different color).

*** /tmp/packet-ip.c	Mon Apr 16 16:48:25 2001
--- ./packet-ip.c	Mon Apr 16 16:46:30 2001
***************
*** 448,459 ****
  
  
  	/* create key to search hash with */
! 	if ( (ipfk=g_malloc(sizeof(ip_fragment_key))) == NULL ){
! 		fprintf(stderr,"packet-ip.c:ip_fragment_add() g_malloc() failed\n");
! 		exit(10);
! 	}
! 	ipfk->srcip = *((guint32 *)pinfo->src.data);
! 	ipfk->dstip = *((guint32 *)pinfo->dst.data);
  	ipfk->id    = id;
  
  	ipfd_head=g_hash_table_lookup(ip_fragment_table, ipfk);
--- 448,456 ----
  
  
  	/* create key to search hash with */
! 	ipfk=g_malloc(sizeof(ip_fragment_key));
! 	memcpy(&ipfk->srcip, pinfo->src.data, sizeof ipfk->srcip);
! 	memcpy(&ipfk->dstip, pinfo->dst.data, sizeof ipfk->dstip);
  	ipfk->id    = id;
  
  	ipfd_head=g_hash_table_lookup(ip_fragment_table, ipfk);
***************
*** 1128,1133 ****
--- 1125,1132 ----
    guint16    ipsum;
    ip_fragment_data *ipfd_head;
    tvbuff_t   *next_tvb;
+   packet_info save_pi;
+   gboolean   must_restore_pi = FALSE;
  
    if (check_col(pinfo->fd, COL_PROTOCOL))
      col_set_str(pinfo->fd, COL_PROTOCOL, "IP");
***************
*** 1188,1193 ****
--- 1187,1197 ----
      return;
    }
  
+   /*
+    * Compute the checksum of the IP header.
+    */
+   ipsum = ip_checksum(tvb_get_ptr(tvb, offset, hlen), hlen);
+ 
    if (tree) {
      proto_tree_add_uint(ip_tree, hf_ip_version, tvb, offset, 1, hi_nibble(iph.ip_v_hl));
      proto_tree_add_uint_format(ip_tree, hf_ip_hdr_len, tvb, offset, 1, hlen,
***************
*** 1231,1237 ****
      proto_tree_add_uint_format(ip_tree, hf_ip_proto, tvb, offset +  9, 1, iph.ip_p,
  	"Protocol: %s (0x%02x)", ipprotostr(iph.ip_p), iph.ip_p);
  
-     ipsum = ip_checksum(tvb_get_ptr(tvb, offset, hlen), hlen);
      if (ipsum == 0) {
  	proto_tree_add_uint_format(ip_tree, hf_ip_checksum, tvb, offset + 10, 2, iph.ip_sum,
                "Header checksum: 0x%04x (correct)", iph.ip_sum);
--- 1235,1240 ----
***************
*** 1345,1350 ****
--- 1348,1361 ----
  
        /* It's not fragmented. */
        pinfo->fragmented = FALSE;
+ 
+       /* Save the current value of "pi", and adjust certain fields to
+          reflect the new tvbuff. */
+       save_pi = pi;
+       pi.compat_top_tvb = next_tvb;
+       pi.len = tvb_reported_length(next_tvb);
+       pi.captured_len = tvb_length(next_tvb);
+       must_restore_pi = TRUE;
      } else {
        /* We don't have the complete reassembled payload. */
        next_tvb = NULL;
***************
*** 1381,1386 ****
--- 1392,1400 ----
        col_add_fstr(pinfo->fd, COL_INFO, "Fragmented IP protocol (proto=%s 0x%02x, off=%u)",
  	ipprotostr(iph.ip_p), iph.ip_p, (iph.ip_off & IP_OFFSET) * 8);
      dissect_data(tvb, offset, pinfo, tree);
+ 
+     /* As we haven't reassembled anything, we haven't changed "pi", so
+        we don't have to restore it. */
      return;
    }
  
***************
*** 1397,1402 ****
--- 1411,1419 ----
        col_add_fstr(pinfo->fd, COL_INFO, "%s (0x%02x)", ipprotostr(iph.ip_p), iph.ip_p);
      dissect_data(next_tvb, 0, pinfo, tree);
    }
+ 
+   if (must_restore_pi)
+     pi = save_pi;
  }