Ethereal-dev: Re: [Ethereal-dev] Armagetronad dissector update

Note: This archive is from the project's previous web site, ethereal.com. This list is no longer active.

From: Guillaume Chazarain <guichaz@xxxxxxxx>
Date: Wed, 29 Mar 2006 16:40:03 +0200
Gerald Combs wrote:
Guillaume Chazarain wrote:
> Jaap Keuter wrote:
>> +               guint16 *ptr, *end = (guint16*) &data[data_len];
>> +               for (ptr = (guint16*) data; ptr != end; ptr++) {
>>
>> This will never end when data_len is odd.
> guint16 data_len = tvb_get_ntohs(tvb, offset + 4) * 2;
> This can't happen ;-)

What happens if "tvb_get_ntohs(tvb, offset + 4)" returns 0, 32768, or 32769?
Good point, because of the overflow, a valid packet (unlikely with such a high length) would have been rejected. Other than that, I don't see anything bad happening.
So I changed the guint16 to gint.

Thanks for the review.

Index: epan/dissectors/packet-armagetronad.c
===================================================================
--- epan/dissectors/packet-armagetronad.c	(révision 17755)
+++ epan/dissectors/packet-armagetronad.c	(copie de travail)
@@ -55,7 +55,7 @@
 #define ACK 1
 
 /*
- * CVS as of 05/05/2005
+ * armagetronad-0.2.8.1
  * The list in src/network/Makefile.in does not seem up to date,
  * so the numbers and names were retrieved at runtime using the
  * 'nDescriptor* descriptors[MAXDESCRIPTORS]' array
@@ -87,14 +87,19 @@
 	{51, "big_server"},
 	{52, "small_request"},
 	{53, "big_request"},
+	{54, "big_server_master"},
+	{55, "big_request_master"},
 	{60, "transfer config"},
 	{200, "Chat"},
 	{201, "ePlayerNetID"},
 	{202, "player_removed_from_game"},
+	{203, "Chat Client"},
 	{210, "eTimer"},
 	{220, "eTeam"},
 	{230, "vote cast"},
-	{231, "Chat"},
+	{231, "Kick vote"},
+	{232, "Server controlled vote"},
+	{233, "Server controlled vote expired"},
 	{300, "gNetPlayerWall"},
 	{310, "game"},
 	{311, "client_gamestate"},
@@ -102,7 +107,7 @@
 	{321, "destinaton"},
 	{330, "gAIPlayer"},
 	{331, "gAITeam"},
-	{340, "winzone"},
+	{340, "zone"},
 	{0, NULL}
 };
 
@@ -112,7 +117,7 @@
 
 	/* For each message in the frame */
 	while (tvb_length_remaining(tvb, offset) > 2) {
-		guint16 data_len = tvb_get_ntohs(tvb, offset + 4) * 2;
+		gint data_len = tvb_get_ntohs(tvb, offset + 4) * 2;
 
 #if 0
 		/*
@@ -141,15 +146,16 @@
 add_message_data(tvbuff_t * tvb, gint offset, guint16 data_len,
 		 proto_tree * tree)
 {
-	guint16 *data = NULL;
+	guchar *data = NULL;
 
 	if (tree) {
-		data = (guint16*)tvb_memcpy(tvb, ep_alloc(data_len), offset, data_len);
+		data = tvb_memcpy(tvb, ep_alloc(data_len + 1), offset, data_len);
+		data[data_len] = '\0';
 	}
 
 	if (data) {
-		guint16 *ptr, *end = &data[data_len / 2];
-		for (ptr = data; ptr != end; ptr++) {
+		guint16 *ptr, *end = (guint16*) &data[data_len];
+		for (ptr = (guint16*) data; ptr != end; ptr++) {
 			/*
 			 * There must be a better way to tell
 			 * Ethereal not to stop on null bytes
@@ -168,8 +174,6 @@
 
 		proto_tree_add_string(tree, hf_armagetronad_data, tvb, offset,
 				      data_len, (gchar *) data);
-
-		data = NULL;
 	} else
 		proto_tree_add_item(tree, hf_armagetronad_data, tvb, offset,
 				    data_len, FALSE);
@@ -178,7 +182,7 @@
 static gint add_message(tvbuff_t * tvb, gint offset, proto_tree * tree,
 			GString * info)
 {
-	guint16 descriptor_id, message_id, data_len;
+	gint descriptor_id, message_id, data_len;
 	proto_item *msg;
 	proto_tree *msg_tree;
 	const gchar *descriptor;