[Guy Harris <gharris@xxxxxxxxxxxx>]

On Sun, Dec 24, 2000 at 12:18:13PM -0800, Guy Harris wrote:
> On Sun, Dec 24, 2000 at 11:59:24AM -0800, Guy Harris wrote:
> > However, 0.8.14 should be getting an exception from the tvbuff code if
> > it tried to do that, so either the tvbuff code isn't doing the right
> > checks, or the routines being used are ones not specified as throwing
> > exceptions.
> 
> The routines being used are ones not specified as throwing exceptions;
> it's using "tvb_strnlen()" to find the length of the option name and
> value strings, and that just returns -1 if there's no 0 byte found
> before the end of the packet, it doesn't throw an exception.
> 
> It's also not checking whether "tvb_strnlen()" returns -1.

If you have Ethereal source, apply the patch attached to this message to
"packet-tftp.c" and recompile; the resulting version *should* throw an
exceptioin, so that it'll correctly report that there's a problem with
the packet in question, rather than just putting random junk into the
protocol tree window.

It won't necessarily throw the correct exception, as it doesn't know
whether the packet really *is* bad or if not all of it was captured.

Arguably, there should be a tvbuff routine that behaves like
"tvb_strnlen()" except that it would throw the appropriate exception if
it doesn't find the terminating 0 byte in the string.

Index: packet-tftp.c
===================================================================
RCS file: /usr/local/cvsroot/ethereal/packet-tftp.c,v
retrieving revision 1.18
diff -c -r1.18 packet-tftp.c
*** packet-tftp.c	2000/11/19 08:54:09	1.18
--- packet-tftp.c	2000/12/24 20:25:55
***************
*** 87,92 ****
--- 87,93 ----
  };
  
  static void tftp_dissect_options(tvbuff_t *tvb, int offset, proto_tree *tree);
+ static gint tftp_strnlen(tvbuff_t *tvb, gint offset);
  
  static void
  dissect_tftp(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
***************
*** 100,105 ****
--- 101,108 ----
  
  	CHECK_DISPLAY_AS_DATA(proto_tftp, tvb, pinfo, tree);
  
+ 	pinfo->current_proto = "TFTP";
+ 
  	/*
  	 * The first TFTP packet goes to the TFTP port; the second one
  	 * comes from some *other* port, but goes back to the same
***************
*** 149,176 ****
  	    
  	  switch (opcode) {
  	  case RRQ:
! 	    i1 = tvb_strnlen(tvb, offset, -1);
  	    proto_tree_add_item(tftp_tree, hf_tftp_source_file,
! 			    tvb, offset, i1 + 1, FALSE);
! 	    offset += i1 + 1;
  
! 	    i1 = tvb_strnlen(tvb, offset, -1);
  	    ti = proto_tree_add_item(tftp_tree, hf_tftp_transfer_type,
! 			    tvb, offset, i1 + 1, FALSE);
! 	    offset += i1 + 1;
  
  	    tftp_dissect_options(tvb, offset, tftp_tree);
  	    break;
  	  case WRQ:
! 	    i1 = tvb_strnlen(tvb, offset, -1);
  	    proto_tree_add_item(tftp_tree, hf_tftp_destination_file,
! 			    tvb, offset, i1 + 1, FALSE);
! 	    offset += i1 + 1;
  
! 	    i1 = tvb_strnlen(tvb, offset, -1);
  	    ti = proto_tree_add_item(tftp_tree, hf_tftp_transfer_type,
! 			    tvb, offset, i1 + 1, FALSE);
! 	    offset += i1 + 1;
  
  	    tftp_dissect_options(tvb, offset, tftp_tree);
  	    break;
--- 152,179 ----
  	    
  	  switch (opcode) {
  	  case RRQ:
! 	    i1 = tftp_strnlen(tvb, offset);
  	    proto_tree_add_item(tftp_tree, hf_tftp_source_file,
! 			    tvb, offset, i1, FALSE);
! 	    offset += i1;
  
! 	    i1 = tftp_strnlen(tvb, offset);
  	    ti = proto_tree_add_item(tftp_tree, hf_tftp_transfer_type,
! 			    tvb, offset, i1, FALSE);
! 	    offset += i1;
  
  	    tftp_dissect_options(tvb, offset, tftp_tree);
  	    break;
  	  case WRQ:
! 	    i1 = tftp_strnlen(tvb, offset);
  	    proto_tree_add_item(tftp_tree, hf_tftp_destination_file,
! 			    tvb, offset, i1, FALSE);
! 	    offset += i1;
  
! 	    i1 = tftp_strnlen(tvb, offset);
  	    ti = proto_tree_add_item(tftp_tree, hf_tftp_transfer_type,
! 			    tvb, offset, i1, FALSE);
! 	    offset += i1;
  
  	    tftp_dissect_options(tvb, offset, tftp_tree);
  	    break;
***************
*** 191,199 ****
  			    FALSE);
  	    offset += 2;
  
! 	    i1 = tvb_strnlen(tvb, offset, -1);
  	    proto_tree_add_item(tftp_tree, hf_tftp_error_string, tvb, offset,
! 	        i1 + 1, FALSE);
  	    break;
  	  case OACK:
  	    tftp_dissect_options(tvb, offset, tftp_tree);
--- 194,202 ----
  			    FALSE);
  	    offset += 2;
  
! 	    i1 = tftp_strnlen(tvb, offset);
  	    proto_tree_add_item(tftp_tree, hf_tftp_error_string, tvb, offset,
! 	        i1, FALSE);
  	    break;
  	  case OACK:
  	    tftp_dissect_options(tvb, offset, tftp_tree);
***************
*** 210,226 ****
  static void
  tftp_dissect_options(tvbuff_t *tvb, int offset, proto_tree *tree)
  {
! 	int i1, i2;
  
  	while (tvb_offset_exists(tvb, offset)) {
! 	  i1 = tvb_strnlen(tvb, offset, -1);	/* length of option */
! 	  i2 = tvb_strnlen(tvb, offset+i1+1, -1);	/* length of value */
! 	  proto_tree_add_text(tree, tvb, offset, i1+i2+2,
  	          "Option: %.*s = %.*s",
! 		  i1, tvb_get_ptr(tvb, offset, i1),
! 		  i2, tvb_get_ptr(tvb, offset+i1+1, i2));
! 	  offset += i1 + i2 + 2;
  	}
  }
  
  void
--- 213,258 ----
  static void
  tftp_dissect_options(tvbuff_t *tvb, int offset, proto_tree *tree)
  {
! 	int option_len, value_len;
! 	int value_offset;
  
  	while (tvb_offset_exists(tvb, offset)) {
! 	  option_len = tftp_strnlen(tvb, offset);	/* length of option */
! 	  value_offset = offset + option_len;
! 	  value_len = tftp_strnlen(tvb, value_offset);	/* length of value */
! 	  proto_tree_add_text(tree, tvb, offset, option_len+value_len,
  	          "Option: %.*s = %.*s",
! 		  option_len - 1, tvb_get_ptr(tvb, offset, option_len - 1),
! 		  value_len - 1, tvb_get_ptr(tvb, value_offset, value_len - 1));
! 	  offset += option_len + value_len;
! 	}
! }
! 
! /*
!  * Find the length of a null-terminated string in a TFTP packet; if we
!  * don't find the '\0', throw an exception, as it means that either
!  * we didn't capture enough of the frame, or the frame is malformed.
!  *
!  * XXX - we'd like to know *which* exception to throw....
!  *
!  * XXX - this should probably be a standard tvbuff accessor.
!  *
!  * Add 1 to the resulting length, so that it includes the '\0'.
!  */
! static gint
! tftp_strnlen(tvbuff_t *tvb, gint offset)
! {
! 	gint len;
! 
! 	len = tvb_strnlen(tvb, offset, -1);
! 	if (len == -1) {
! 	    /*
! 	     * No '\0' found before the end of the tvbuff; throw an
! 	     * exception.
! 	     */
! 	    THROW(BoundsError);
  	}
+ 	return len + 1;
  }
  
  void