Simplify the state machine:

	you're either reading commands, or you're reading message data;

	if you're reading commands, and you see a DATA command, you
	start reading data;

	if you're reading data, and you see an EOM, you start reading
	commands.

Also, *always* fill in the per-frame data you allocate for a frame, and
*always* attach it to the packet.

The old state machine assumed it was done with the SMTP conversation
once it saw an EOM, and the dissector wouldn't fill in the per-frame
data it'd allocated and attach it to the packet if it thought it was
done with the SMTP conversation.  This meant that:

	1) the per-frame data allocated for frames following the EOM
	   (e.g., a QUIT command) would contain random junk for data
	   such as the packet type;

	2) that per-frame data would be re-allocated every time the
	   frame was looked at, as it wouldn't be attached to the frame,
	   so you might well get *different* random junk each time the
	   frame was looked at.

This caused Tethereal and Ethereal to sometimes fail to recognize
commands following the EOM - but it wouldn't *always* fail to do so,
sometimes it'd work and sometimes it wouldn't.

Fix a comment; conversations are *not* removed during filter operations,
and the visited flag is *not* cleared during a filter operation - that's
only true on a *redissection* operation.  In any case, given that frames
can, after the initial sequential scan through the capture, be visited
in any order, and visited repeatedly, it's irrelevant whether
conversations are removed or not - we have to associate with each frame
information telling us how to process it.

svn path=/trunk/; revision=2608

1 files changed, 63 insertions, 42 deletions

diff --git a/packet-smtp.c b/packet-smtp.c
index 7a6092642c..99baa7469b 100644
--- a/packet-smtp.c
+++ b/packet-smtp.c
@@ -1,7 +1,7 @@
 /* packet-smtp.c
  * Routines for SMTP packet disassembly
  *
- * $Id: packet-smtp.c,v 1.7 2000/10/21 05:52:23 guy Exp $
+ * $Id: packet-smtp.c,v 1.8 2000/11/11 07:48:30 guy Exp $
  *
  * Copyright (c) 2000 by Richard Sharpe <rsharpe@ns.aus.com>
  *
@@ -79,10 +79,11 @@ struct smtp_request_key {
   guint32 conversation;
 };
 
+/*
+ * State information stored with a conversation.
+ */
 struct smtp_request_val {
-  guint16 processed;     /* Have we processed this conversation? */
-  guint16 data_seen;     /* Have we seen the data packet */
-  guint16 eom_seen;      /* Have we seen the end of message */
+  gboolean reading_data; /* Reading message data, not commands */
   guint16 crlf_seen;     /* Have we seen a CRLF on the end of a packet */
 };
 
@@ -199,6 +200,7 @@ dissect_smtp(const u_char *pd, int offset, frame_data *fd,
     conversation_t          *conversation;
     struct smtp_request_key request_key, *new_request_key;
     struct smtp_request_val *request_val;
+    gboolean                eom_seen = FALSE;
 
 #if 0
     CHECK_DISPLAY_AS_DATA(proto_smtp, tvb, pinfo, tree);
@@ -206,14 +208,23 @@ dissect_smtp(const u_char *pd, int offset, frame_data *fd,
     OLD_CHECK_DISPLAY_AS_DATA(proto_smtp, pd, offset, fd, tree);
 #endif
 
-    /* If we have per frame data, use that, else, we must be on the first 
-     * pass, so we figure it out on the first pass.
-     * 
-     * Since we can't stash info away in a conversation (as they are 
-     * removed during a filter operation, and we can't rely on the visited
-     * flag, as that is set to 0 during a filter, we must save per-frame
-     * data for each frame. However, we only need it for requests. Responses
+    /* As there is no guarantee that we will only see frames in the
+     * the SMTP conversation once, and that we will see them in
+     * order - in Ethereal, the user could randomly click on frames
+     * in the conversation in any order in which they choose - we
+     * have to store information with each frame indicating whether
+     * it contains commands or data or an EOM indication.
+     *
+     * XXX - what about frames that contain *both*?  TCP is a
+     * byte-stream protocol, and there are no guarantees that
+     * TCP segment boundaries will correspond to SMTP commands
+     * or EOM indications.
+     *
+     * We only need that for the client->server stream; responses
      * are easy to manage.
+     *
+     * If we have per frame data, use that, else, we must be on the first 
+     * pass, so we figure it out on the first pass.
      */
 
     /* Find out what conversation this packet is part of ... but only
@@ -252,9 +263,7 @@ dissect_smtp(const u_char *pd, int offset, frame_data *fd,
 	new_request_key->conversation = conversation->index;
 
 	request_val = g_mem_chunk_alloc(smtp_request_vals);
-	request_val->processed = 0;
-	request_val->data_seen = 0;
-	request_val->eom_seen = 0;
+	request_val->reading_data = FALSE;
 	request_val->crlf_seen = 0;
 
 	g_hash_table_insert(smtp_request_hash, new_request_key, request_val);
@@ -268,7 +277,7 @@ dissect_smtp(const u_char *pd, int offset, frame_data *fd,
        * two passes through here ...
        */
 
-      if (request_val->data_seen && !request_val->processed) {
+      if (request_val->reading_data) {
 
 	/*
 	 * The order of these is important ... We want to avoid
@@ -279,7 +288,7 @@ dissect_smtp(const u_char *pd, int offset, frame_data *fd,
 	if ((request_val->crlf_seen && strncmp(pd + offset, ".\r\n", 3) == 0) ||
 	    (strncmp(pd + offset, "\r\n.\r\n", 5) == 0)) {
 
-	  request_val->eom_seen = 1;
+	  eom_seen = TRUE;
 
 	}
 
@@ -304,40 +313,52 @@ dissect_smtp(const u_char *pd, int offset, frame_data *fd,
 
 	frame_data = g_mem_chunk_alloc(smtp_packet_infos);
 
-	if (!request_val->processed) { 
-	  if (strncmp(pd + offset, "DATA", 4)==0) {
-
-	  request_val->data_seen = 1;
-	  frame_data->pdu_type = SMTP_PDU_CMD;
-	  p_add_proto_data(pinfo->fd, proto_smtp, frame_data);
-
-	  } else if ((!request_val->eom_seen) &&
-		     (request_val->data_seen)) {
-	 
-	    /* Now, create the frame data for this frame ... */
-
+	if (request_val->reading_data) {
+	  /*
+	   * This is message data.
+	   */
+	  if (eom_seen) { /* Seen the EOM */
+	    /*
+	     * EOM.
+	     * Everything that comes after it is commands.
+	     *
+	     * XXX - what if the EOM isn't at the beginning of
+	     * the TCP segment?  It can occur anywhere....
+	     */
+	    frame_data->pdu_type = SMTP_PDU_EOM;
+	    request_val->reading_data = FALSE;
+	  } else {
+	    /*
+	     * Message data with no EOM.
+	     */
 	    frame_data->pdu_type = SMTP_PDU_MESSAGE;
-	    p_add_proto_data(pinfo->fd, proto_smtp, frame_data);
-
-	  } else if (request_val->eom_seen) { /* Seen the EOM */
-
-	    /* Now, we clear the eom_seen and data_seen bits */
-
-	    request_val->eom_seen = request_val->data_seen = 0;
-	    request_val->processed = 1;   /* We have seen all the packets */
-
-	    /* And add the packet data */
+	  }
+	} else {
+	  /*
+	   * This is commands.
+	   */
+	  if (strncmp(pd + offset, "DATA", 4)==0) {
 
-	    frame_data->pdu_type = SMTP_PDU_EOM;
-	    p_add_proto_data(pinfo->fd, proto_smtp, frame_data);
+	    /*
+	     * DATA command.
+	     * This is a command, but everything that comes after it,
+	     * until an EOM, is data.
+	     */
+	    frame_data->pdu_type = SMTP_PDU_CMD;
+	    request_val->reading_data = TRUE;
 
 	  } else {
 
+	    /*
+	     * Regular command.
+	     */
 	    frame_data->pdu_type = SMTP_PDU_CMD;
-	    p_add_proto_data(pinfo->fd, proto_smtp, frame_data);
-
 	  }
+
 	}
+
+	p_add_proto_data(pinfo->fd, proto_smtp, frame_data);
+
       }
     }