Wireshark · Ethereal-dev: Re: Patch in reassemble.c (was Re: [Ethereal-dev] Crash in ethereal 0.10.8, somewhat reproducible)

[Peter Johansson <Peter.Johansson@xxxxxxxxxxxx>]

Guy Harris wrote:

> Peter Johansson wrote:
>
> > Pilz Rene wrote:
> >
> > > Peter Johansson wrote:
> > >
> > > > ronnie sahlberg wrote:
> > > >
> > > > > ok   jag avvaktar ett tag.
> > > > >
> > > > > en idee,   det kan eventuellt vara fel i bittorrent dissectorn
> > > > > eftersom det inte verkar vara nagra storre fel pa hur andra
> > > > > dissectorer anvander reassemble.
> > > > >
> > > > > kan du kolla att bittorrent dissectorn ENDAST anropar reassembly OMM
> > > > > packetet verkligen ar fragmenterat.  DVS om hela PDUn redan finns i
> > > > > paketet och darfor ingen reassembly behovs,  da skall inte reassembly
> > > > > rutinerna anropas.
> > > > >
> > > > >
> > > > > reassembly rutinerna ar lite risiga,   de var nar jag skrev
> > > > > reassemble.c  mest ett snabbt hack  men jag fick aldrig tid att 
> > > > > skriva
> > > > > om koden battre :-(
> > > > >
> > > > >
> > > > > On Sat, 22 Jan 2005 15:41:22 +0100, Peter Johansson
> > > > > <Peter.Johansson@xxxxxxxxxxxx> wrote:
> > > > >  
> > > > >
> > > > > > ronnie sahlberg wrote:
> > > > > >
> > > > > >  
> > > > > >
> > > > > > > Hej,
> > > > > > >
> > > > > > > Har du en exempel capturefil du kan skicka till mig sa kan jag 
> > > > > > > titta
> > > > > > > pa vad som ar fel.
> > > > > > >
> > > > > > >
> > > > > > >     
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > Jag har två capturefiler, vardera om 20MB (jag körde capture till
> > > > > > roterande filer). Om först laddar den ena och sedan den andra kan 
> > > > > > man
> > > > > > reproducera krashen.
> > > > > > Jag håller på och felsöker och tror jag hittat en workaround för
> > > > > > krashen, tyvärr har jag ännu inte hittat den egentliga orsaken till
> > > > > > problemet.
> > > > > > Den info jag samlat på mig är:
> > > > > > 1. Kraschen är ett faktum (förr eller senare) endast om den trafik
> > > > > > ethereal avkodar innehåller Bittorrent data, det kan alltså vara ett
> > > > > > problem med packet-bittorrent.c (har inte hunnit börja kika på den
> > > > > > filen), se också punkt 4.
> > > > > > 2. Kraschen i reassemble.c som ju sker i ett anrop till memcpy sker
> > > > > > eftersom source (fd_i->data) inte verkar vara allokerat minne,
> > > > > > fd_i->flags anger bla att biten FD_NOT_MALLOCED är satt.
> > > > > > 3. Kraschen inträffar endast när ethereal avkodar insamlat data, dvs
> > > > > > bara om man gör capture med "Update list of packets in realtime" 
> > > > > > påslaget.
> > > > > > 4. Kraschen inträffar endast om Bittorrent avkodaren är med i listan
> > > > > > över "enabled protocols", därför tror jag att orsaken till kraschen
> > > > > > egentligen ligger i packet-bittorent.c.
> > > > > >
> > > > > > Min workaround (som troligtvis ska permanentas även om felet 
> > > > > > troligtvis
> > > > > > ligger i packet-bittorrent.c) är att skriva om ett fåtal rader i
> > > > > > reassemble.c (se den bifogade bilden), det garanterar att man 
> > > > > > inte kan
> > > > > > krascha där för att man försöker hantera data som ej är 
> > > > > > allokerat. Så
> > > > > > vitt jag har förstått bör konsekvensen bli att ethereal kanske inte
> > > > > > avkodar just denna PDU på korrekt sätt. Man bör kanske därför 
> > > > > > lägga in
> > > > > > någon kod som kan tala om för användaren att så är fallet (kanske en
> > > > > > error popup där framenumret är angivet, etc). Därutöver ska 
> > > > > > naturligtvis
> > > > > > den egentliga orsaken till kraschen grävas fram.
> > > > > >
> > > > > > Vidare tror jag att reassemble.c bör skrivas om såsom den bifogade
> > > > > > bilden visar. Dvs man ska aldrig anropa memcpy om src->flags eller
> > > > > > destination->flags har biten FD_NOT_MALLOCED satt.
> > > > > > Tyvärr rättar ju inte det orsaken till problemet, det ser bara 
> > > > > > till att
> > > > > > kraschen inte sker. Anledningen till att jag tidigare skrev att 
> > > > > > jag inte
> > > > > > visste vad jag skulle göra med problemet var att jag då inte kunde
> > > > > > reproducera det på ett kontrollerat sätt, det kan jag nu.
> > > > > > Är du fortfarande intresserad av filerna eller vill du avvakta?
> > > > > >
> > > > > > Mvh Peter
> > > > > >   
> > > > I have not forgot about this, I have just been a little bit more 
> > > > busy than usual. I finally tracked down the problem though.
> > > > My conclusion is this:
> > > > Ethereal crashes in reassemble.c because reassemble.c copies data 
> > > > to a memory area that is not yet allocated (fd_i->flags has the 
> > > > FD_NOT_MALLOCED bit set). I have a solution to this (ensuring that 
> > > > a crash does not occur) which I will post once I have done some 
> > > > cleaning up.
> > > >
> > > > The crash in reassemble.c occurs only as the result of a faulty 
> > > > protocol dissector. In this case it is packet-bittorrent.c that is 
> > > > the reason for the crash.
> > > > The Bittorrent dissector registers only a heur-dissector (which 
> > > > should be fine). But once the heur test function detects that this 
> > > > TCP stream is in fact Bittorrent data, it creates a conversation, 
> > > > making sure that all future data in the same TCP stream is decoded 
> > > > by the Bittorrent protocol dissector without the use of the heur 
> > > > test function (this too should be fine I guess).
> > > > The heur test function is capable of telling the calling framework 
> > > > whether the PDU was in fact decoded by this dissector or not by 
> > > > returning TRUE or FALSE
> > > > packet-bittorrent.c. The function that dissects Bittorrent data 
> > > > based on the fact that it belongs to a conversation does not have 
> > > > the opportunity of telling the calling framework that it in fact 
> > > > cannot decode the supplied PDU if necessary. And this is necessary 
> > > > in the rare event that packet-tcp has marked the current PDU with 
> > > > "[TCP previous segment lost]". In this case some data is missing 
> > > > but the Bittorrent dissector still assumes that the first 4 bytes 
> > > > of the PDU denotes the length of the PDU to be dissected. The 
> > > > problem now is that since data was lost, the length is read using a 
> > > > random offset into the original Bittorrent packet (since some data 
> > > > was lost).
> > > > My guess is that this could happen for any dissector that is called 
> > > > since the data belongs to a conversation created by the specific 
> > > > dissector when data has been lost.
> > > >
> > > > Should packet-tcp perhaps not call higher level dissector when the 
> > > > PDU is marked with "[TCP previous segment lost]" or at least not 
> > > > perform the try_conversation_dissector(...) call?
> > > > What would be the better way of ensuring that this does not happen 
> > > > with any of the already existing dissectors?
> > > > Should perhaps the API at hand for dissectors be changed so that 
> > > > when decoding PDU data, the dissector would be able to return TRUE 
> > > > or FALSE in a similar way to the heur functions? This way, any 
> > > > dissector would be able to tell the lower layer dissector that 
> > > > although it should have handled this PDU, it could not.
> > > >
> > > > What is your opinion?
> > > >
> > > > / Peter
> > > >
> > > > _______________________________________________
> > > > Ethereal-dev mailing list
> > > > Ethereal-dev@ethereal.
> > > > http://www.ethereal.com/mailman/listinfo/ethereal-dev
>
> Unfortunately:
>
>     1) this completely breaks the feature wherein a TCP dissector, 
> handed a reassembled chunk of data, can indicate that it needs at 
> least N more bytes of data to be added to the reassembled chunk, so 
> that the reassembly has to be continued
>
> and
>
>     2) I don't understand Swedish so I can't easily tell what the 
> technical discussion above says.
>
> That feature is used by several dissectors, such as the HTTP dissector 
> which, when it's reassembling the entity headers of an HTTP request or 
> response, keeps requesting more data until it sees the blank line at 
> the end of the entity headers, at which point it says the reassembly 
> is complete.
>
> That feature is now broken because:
>
>     The "If it was already defragmented and this new fragment goes 
> beyond data limits" loop at the top of "fragment_add_work()" "undoes" 
> the reassembly by pointing fragments that no longer have data, because 
> it was copied to the reassembled chunk and then freed, at the target 
> of the copy in the reassembled chunk, and sets the FD_NOT_MALLOCED 
> flag on those fragments.
>
>     The "we have received an entire packet, defragment it and free all 
> fragments" code in "fragment_add_work()" saves the pointer to the old 
> reassembled chunk, allocates a new chunk to hold the reassembled data, 
> and then falls into the "add all data fragments" loop.
>
>     The "add all data fragments" loop in "fragment_add_work()" then 
> used to copy *all* the fragments, regardless of whether 
> FD_NOT_MALLOCED was set on the fragment or not, into the 
> newly-allocated chunk.  It now copies only the chunks with 
> FD_NOT_MALLOCED set, and reports the others as being "Reassemble error"s.
>
> This means that, in the reassemblies after the first reassembly, some 
> of the data in the reassembled chunk is whatever just happened to be 
> there at the time of the allocation.
>
> The old code *did* work correctly for some captures I have with HTTP 
> traffic in them - FD_NOT_MALLOCED doesn't mean "fd_i->data isn't 
> valid", it means "it's not the address of a mallocated chunk, it's an 
> address *in* a mallocated chunk".
>
> What are the details of the cases where the old code *didn't* work?
>
> It might be that the correct fix is to, in the "we have received an 
> entire packet..." code, set "fd_head->data" to 
> "g_realloc(fd_head->data, max)", which means that the data that was 
> already copied there during previous reassemblies will still be 
> there.  However, we *still* need to get rid of the printout of the 
> "Reassemble error" message, because it's bogus to print that message 
> every time we, for example, reassemble HTTP entity headers - which 
> means we should really figure out why we're doing that in cases where 
> it *is* an error, and figure out where to fix that.
>
> "tcp_dissect_pdus()" uses the "continue reassembly" feature - it first 
> tries reassembling the fixed-length portion of the PDU, so that the 
> "get the length" routine has enough of that portion to find out how 
> large the packet is, and then tries reassembling the entire packet, so 
> if the 4-byte header of a presumed BitTorrent packet is split across 
> TCP segments, that code path would be used.
>
> One place where there's *definitely* a risk of problems is a presumed 
> BitTorrent packet where the presumed length field is greater than 
> 2^32-5, so that when 4 is added to it we overflow and get a value 
> *less* than 4.  However, going back to at least 0.10.8, if the 
> get_pdu_len routine called by "tcp_dissect_pdus()" returns a value 
> less than the length of the fixed-length portion of the PDU, that's 
> assumed to be an overflow, so it just shows a "Malformed packet" error 
> and quits.
This is a repost since my first attempt (a week ago) did not seem to 
reach the list.

1) My apologies, my intention was of course not to break the reassembly 
code...although I don't quite understand what you imply. I thought I had 
verified that packet reassembly still works after the patch. Am I just 
not looking at enough layers of dissectors on top of packet-tcp?

2) My end-part of my conversation with Ronnie Sahlberg translated from 
swedish is attached in the Swedish2English.txt file.

Since the reassembly function is now broken, I guess that the best thing 
to do right now is to back out of my earlier proposed change to 
reassembly.c. This will however make ethereal crash again in memcpy 
called from reasseble.c most certainly when decoding Bittorrent data. 
However the problem is quite much bigger than that (as I wrote earlier). 
Any dissector may cause a crash in reassemble.c because the dissector is 
not decoding the data length correctly. I asked before if there were any 
ideas on how to handle this matter, but since this seems to have been 
forgotten I will try again.

Should perhaps the API at hand for dissectors be changed so that when 
decoding PDU data, the dissector would be able to return TRUE or FALSE 
in a similar way to the heur functions? This way, any dissector would be 
able to tell the lower layer dissector that although it should have 
handled this PDU, it could not (or just simply refuses to even though 
the dissector is in fact registered to handle data from this stream)?

While I applied my proposal for a change in reassemble.c (that led to 
this bug you have just pointed out), I also did some rework in 
packet-bittorrent.c that I applied as a path. Perhaps someone could take 
a look at that (see 
http://www.ethereal.com/lists/ethereal-dev/200504/msg00307.html). 
Unfortunately other changes has been made to packet-bittorrent.c since 
then. Therefore I also reapply a patch (rework) for packet-bittorrent.c.
I guess I will have to hunt further into the Bittorrent dissector once 
more to see if it can detect malicious data (actually that a PDU has 
been lost) and still not cause a crash in reassemle.c.

I understand (now) that my interpretation of FD_NOT_MALLOCED was not 
correct, the fact still remains though that reassemble.c passed invalid 
(unallocated data) to memcpy. I only noticed this when also the 
FD_NOT_MALLOCED bit was set, hence my misinterpretation. I guess that 
Ronnie Sahlbergs proposal for a new memory allocation API could be used 
to detect that a memory are is not valid and should not be passed on to 
for instance memcpy.
I honestly do not know what the best solution would be for this problem 
as it is reassemble.c that is vulnerable to dissectors that cannot 
handle their data correctly.

/ Peter

I have two capturefiles, of 20MB each (I ran capture to rotating files). If I first load one and then the other the crash is reproducible.
I am investigating and believe to have found a workaround for the krash, unfortunately I have not yet discovered the real cause of the problem.
The information that I have gathered is:
1. The crash is a fact (sooner or later) only if the traffic ethereal decodes containts Bittorrent data, hence it may be a problem with packet-bittorrent.c (I have not yet had time to look at that file), see also item number 4.
2. The crash in reassemle.c that ocurrs in a call to memcpy is a fact since the source (fd_i->data) does not seem to be allocated memory, fd_i->flags tells (among other things) that the bit FD_NOT_MALLOCAED is set.
3. The crash only ocurrs when ethereal decodes gathered data, that is only if performing a capture with "Update list of packets in realtime" enabled.
4. The crash ocurrs only if the Bittorrnet dissector is enabled in the list of "enabled protocols", this is the reason why I believe that the cause of the crash is in fact in packet-bittorrent.c.

My workaround (which probably should be applied even if the fault is most certainly in packet-bittorrent.c) is to rewrite a few lines in reassemble.c (see the attached file), this guarantees that a crash will not ocurr there when trying to handle data that for which no memory has been allocated. As far as I know, the consequence should be that ethereal might not decode this specific PDU correctly. Therefore code should perhaps be added to tell the user that this is the case (perhaps an error popup where the framenumber is displayed, etc). Apart from this should of course the real reason for the crash be investigated.

Furthermore I believe that reassemble.c should be rewritten as the attached image shows. That is, a call should never be made to memcpy if src->flags or destination->flags has the bit FD_NOT_MALLOCED set.

Unfortunately this does not correct the source of the problem, it only ensures that the crash will not happen. The reason why I earlier wrote that I did not know what to do with this problem was that I could not reproduce the problem before in a controlled manner, which I can now.

Index: I:/ethereal-win32-libs/epan/dissectors/packet-bittorrent.c
===================================================================
--- I:/ethereal-win32-libs/epan/dissectors/packet-bittorrent.c	(working copy)
+++ I:/ethereal-win32-libs/epan/dissectors/packet-bittorrent.c	(revision 14913)
@@ -37,346 +37,252 @@
 #include <epan/conversation.h>
 #include <epan/packet.h>
 #include <epan/strutil.h>
-#include <epan/gdebug.h>
 
 #include "packet-tcp.h"
 
 /*
  * See
  *
- * http://bittorrent.com/protocol.html
- * http://wiki.theory.org/BitTorrentSpecification
- * http://bitconjurer.org/BitTorrent/protocol.html
+ *	http://bittorrent.com/protocol.html
  */
 
-#define BITTORRENT_MESSAGE_CHOKE          0
-#define BITTORRENT_MESSAGE_UNCHOKE        1
-#define BITTORRENT_MESSAGE_INTERESTED     2
-#define BITTORRENT_MESSAGE_NOT_INTERESTED 3
-#define BITTORRENT_MESSAGE_HAVE           4
-#define BITTORRENT_MESSAGE_BITFIELD       5
-#define BITTORRENT_MESSAGE_REQUEST        6
-#define BITTORRENT_MESSAGE_PIECE          7
-#define BITTORRENT_MESSAGE_CANCEL         8
+#define BITTORRENT_MESSAGE_CHOKE			0
+#define BITTORRENT_MESSAGE_UNCHOKE			1
+#define BITTORRENT_MESSAGE_INTERESTED		2
+#define BITTORRENT_MESSAGE_NOT_INTERESTED	3
+#define BITTORRENT_MESSAGE_HAVE				4
+#define BITTORRENT_MESSAGE_BITFIELD			5
+#define BITTORRENT_MESSAGE_REQUEST			6
+#define BITTORRENT_MESSAGE_PIECE			7
+#define BITTORRENT_MESSAGE_CANCEL			8
 
-#define BITTORRENT_HEADER_LENGTH          4
-
 static const value_string bittorrent_messages[] = {
-   { BITTORRENT_MESSAGE_CHOKE, "Choke" },
-   { BITTORRENT_MESSAGE_UNCHOKE, "Unchoke" },
-   { BITTORRENT_MESSAGE_INTERESTED, "Interested" },
-   { BITTORRENT_MESSAGE_NOT_INTERESTED, "Not Interested" },
-   { BITTORRENT_MESSAGE_HAVE, "Have" },
-   { BITTORRENT_MESSAGE_BITFIELD, "Bitfield" },
-   { BITTORRENT_MESSAGE_REQUEST, "Request" },
-   { BITTORRENT_MESSAGE_PIECE, "Piece" },
-   { BITTORRENT_MESSAGE_CANCEL, "Cancel" },
-   { 0, NULL }
+	{ BITTORRENT_MESSAGE_CHOKE, "Choke" },
+	{ BITTORRENT_MESSAGE_UNCHOKE, "Unchoke" },
+	{ BITTORRENT_MESSAGE_INTERESTED, "Interested" },
+	{ BITTORRENT_MESSAGE_NOT_INTERESTED, "Not Interested" },
+	{ BITTORRENT_MESSAGE_HAVE, "Have" },
+	{ BITTORRENT_MESSAGE_BITFIELD, "Bitfield" },
+	{ BITTORRENT_MESSAGE_REQUEST, "Request" },
+	{ BITTORRENT_MESSAGE_PIECE, "Piece" },
+	{ BITTORRENT_MESSAGE_CANCEL, "Cancel" },
+	{ 0, NULL }
 };
 
-static dissector_handle_t dissector_handle;
 static int proto_bittorrent = -1;
 
 static gint hf_bittorrent_field_length  = -1;
 static gint hf_bittorrent_prot_name_len = -1;
 static gint hf_bittorrent_prot_name     = -1;
-static gint hf_bittorrent_reserved      = -1;
-static gint hf_bittorrent_sha1_hash     = -1;
-static gint hf_bittorrent_peer_id       = -1;
-static gint hf_bittorrent_msg_len       = -1;
-static gint hf_bittorrent_msg_type      = -1;
-static gint hf_bittorrent_bitfield_data = -1;
-static gint hf_bittorrent_piece_index   = -1;
-static gint hf_bittorrent_piece_begin   = -1;
-static gint hf_bittorrent_piece_length  = -1;
-static gint hf_bittorrent_piece_data    = -1;
+static gint hf_bittorrent_reserved 		= -1;
+static gint hf_bittorrent_sha1_hash		= -1;
+static gint hf_bittorrent_peer_id 		= -1;
+static gint hf_bittorrent_msg_len		= -1;
+static gint hf_bittorrent_msg_type		= -1;
+static gint hf_bittorrent_bitfield_data	= -1;
+static gint hf_bittorrent_piece_index	= -1;
+static gint hf_bittorrent_piece_begin	= -1;
+static gint hf_bittorrent_piece_length	= -1;
+static gint hf_bittorrent_piece_data	= -1;
 
 static gint ett_bittorrent = -1;
 static gint ett_bittorrent_msg = -1;
-static gint ett_peer_id = -1;
 
 static gboolean bittorrent_desegment = TRUE;
-static gboolean decode_client_information = FALSE;
 
-struct client_information {
-   char id[4];
-   char name[25];
-};
+static dissector_handle_t bittorrent_handle;
 
-static struct client_information peer_id[] = {
-   {"-AZ", "Azureus"},
-   {"-BB", "BitBuddy"},
-   {"-CT", "CTorrent"},
-   {"-MT", "MoonlightTorrent"},
-   {"-LT", "libtorrent"},
-   {"-BX", "Bittorrent X"},
-   {"-TS", "Torrentstorm"},
-   {"-TN", "TorrnetDotNET"},
-   {"-SS", "SwarmScope"},
-   {"-XT", "XanTorrent"},
-   {"-BS", "BTSlave"},
-   {"-ZT", "ZipTorrent"},
-   {"S",   "Shadow's client"},
-   {"U",   "UPnP NAT Bit Torrent"},
-   {"T",   "BitTornado"},
-   {"A",   "ABC"},
-   {NULL,  NULL}
-};
-
 static guint get_bittorrent_pdu_length(tvbuff_t *tvb, int offset)
 {
-   guint8 type;
-   guint32 length;
-
-   if (tvb_get_guint8(tvb, offset) == 19 &&
-       tvb_memeql(tvb, offset + 1, "BitTorrent protocol", 19) == 0) {
-      /* Return the length of a Handshake message */
-      return 1 + /* pstrlen */
-         19 +    /* pstr */
-         8 +     /* reserved */
-         20 +    /* SHA1 hash of the info key */
-         20;     /* peer id */
-   } else {
-      /* Try to validate the length of the message indicated by the header. */
-      if(tvb_bytes_exist(tvb, offset, 5)) {
-         type = tvb_get_guint8(tvb, offset + BITTORRENT_HEADER_LENGTH);
-         if(type <= BITTORRENT_MESSAGE_CANCEL) {
-         /* This seems to be a valid BitTorrent header with a known
-            type identifier */
-            return BITTORRENT_HEADER_LENGTH + tvb_get_ntohl(tvb, offset);
-         } else {
-         /* The type is not known, this message cannot be decoded
-            properly by this dissector. */
-            DISSECTOR_ASSERT(!"Reassemble error?");
-            return 0;
-         }
-      } else {
-      /* Only the BitTorrent header exists, it should read 0,
-         otherwise it is not a valid BitTorrent message */
-         length = tvb_get_ntohl(tvb, offset);
-         if(length == 0) {
-            return BITTORRENT_HEADER_LENGTH;
-         } else {
-         /* This is not something that can be decoded properly by
-            this dissector. */
-            DISSECTOR_ASSERT(!"Reassemble error?");
-            return 0;
-         }
-      }
-   }
+	if (tvb_get_guint8(tvb, offset) == 19) {
+		return 20 + 20 + 20 + 8;
+	} else {
+		guint32 length = tvb_get_ntohl(tvb, offset);
+		return 4 + length;
+	}
 }
 
 static void dissect_bittorrent_message (tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
 {
-   int offset = 0;
-   proto_tree *mtree;
-   guint8 type;
-   guint32 length = tvb_get_ntohl(tvb, offset);
-   proto_item *ti = proto_tree_add_text(tree, tvb, offset, length + BITTORRENT_HEADER_LENGTH, "BitTorrent Message");
-   
-   mtree = proto_item_add_subtree(ti, ett_bittorrent_msg);
+	int offset = 0;
+	proto_tree *mtree;
+	guint8 type;
+	guint32 length = tvb_get_ntohl(tvb, offset);
+	proto_item *ti = proto_tree_add_text(tree, tvb, offset, length, "BitTorrent Message");
 
-   proto_tree_add_item(mtree, hf_bittorrent_msg_len, tvb, offset, BITTORRENT_HEADER_LENGTH, FALSE); offset+=BITTORRENT_HEADER_LENGTH;
+	mtree = proto_item_add_subtree(ti, ett_bittorrent_msg);
+								  
+	proto_tree_add_item(mtree, hf_bittorrent_msg_len, tvb, offset, 4, FALSE); offset+=4;
 
-   /* Keepalive message */
-   if (length == 0) {
-      if (check_col(pinfo->cinfo, COL_INFO)) {
-         col_set_str(pinfo->cinfo, COL_INFO, "BitTorrent KeepAlive message");
-      }
-      return;
-   }
-   
-   type = tvb_get_guint8(tvb, offset);
-   proto_tree_add_item(mtree, hf_bittorrent_msg_type, tvb, offset, 1, FALSE); offset+=1;
-   
-   if (check_col(pinfo->cinfo, COL_INFO)) {
-      char *val = match_strval(type, bittorrent_messages);
-      if (val != NULL) {
-         col_set_str(pinfo->cinfo, COL_INFO, val);
-      }
-   }
+	/* Keepalive message */
+	if (length == 0) {
+	    if (check_col(pinfo->cinfo, COL_INFO)) {
+		    col_set_str(pinfo->cinfo, COL_INFO, "BitTorrent KeepAlive message");
+		}
+		return;
+	}
+	
+	type = tvb_get_guint8(tvb, offset);
+	proto_tree_add_item(mtree, hf_bittorrent_msg_type, tvb, offset, 1, FALSE); offset+=1;
 
-    switch (type) 
-    {
-    case BITTORRENT_MESSAGE_CHOKE:
-    case BITTORRENT_MESSAGE_UNCHOKE:
-    case BITTORRENT_MESSAGE_INTERESTED:
-    case BITTORRENT_MESSAGE_NOT_INTERESTED:
-       /* No payload */
-       break;
-       
-    case BITTORRENT_MESSAGE_REQUEST:
-    case BITTORRENT_MESSAGE_CANCEL:
-       proto_tree_add_item(mtree, hf_bittorrent_piece_index, tvb, offset, 4, FALSE); offset += 4;
-       proto_tree_add_item(mtree, hf_bittorrent_piece_begin, tvb, offset, 4, FALSE); offset += 4;
-       proto_tree_add_item(mtree, hf_bittorrent_piece_length, tvb, offset, 4, FALSE);
-       break;
-       
-    case BITTORRENT_MESSAGE_HAVE:
-       proto_tree_add_item(mtree, hf_bittorrent_piece_index, tvb, offset, 4, FALSE);
-       break;
-       
-    case BITTORRENT_MESSAGE_BITFIELD:
-       proto_tree_add_item(mtree, hf_bittorrent_bitfield_data, tvb, offset, tvb_length_remaining(tvb, offset), FALSE); 
-       break;
-       
-    case BITTORRENT_MESSAGE_PIECE:
-       proto_tree_add_item(mtree, hf_bittorrent_piece_index, tvb, offset, 4, FALSE); offset += 4;
-       proto_tree_add_item(mtree, hf_bittorrent_piece_begin, tvb, offset, 4, FALSE); offset += 4;
-       proto_tree_add_item(mtree, hf_bittorrent_piece_data, tvb, offset, tvb_length_remaining(tvb, offset), FALSE);
-       break;
-       
-    default:
-       DISSECTOR_ASSERT(!"Not Bittorrent data after all");
-    }
+	if (check_col(pinfo->cinfo, COL_INFO)) {
+		const char *val = match_strval(type, bittorrent_messages);
+		if (val != NULL) {
+		    col_set_str(pinfo->cinfo, COL_INFO, val);
+		}
+	}
+
+	switch (type) 
+	{
+	case BITTORRENT_MESSAGE_CHOKE:
+	case BITTORRENT_MESSAGE_UNCHOKE:
+	case BITTORRENT_MESSAGE_INTERESTED:
+	case BITTORRENT_MESSAGE_NOT_INTERESTED:
+		/* No payload */
+		break;
+		
+
+	case BITTORRENT_MESSAGE_REQUEST:
+	case BITTORRENT_MESSAGE_CANCEL:
+		proto_tree_add_item(mtree, hf_bittorrent_piece_index, tvb, offset, 4, FALSE); offset += 4;
+		proto_tree_add_item(mtree, hf_bittorrent_piece_begin, tvb, offset, 4, FALSE); offset += 4;
+		proto_tree_add_item(mtree, hf_bittorrent_piece_length, tvb, offset, 4, FALSE);
+		break;
+		
+	case BITTORRENT_MESSAGE_HAVE:
+		proto_tree_add_item(mtree, hf_bittorrent_piece_index, tvb, offset, 4, FALSE);
+		break;
+		
+	case BITTORRENT_MESSAGE_BITFIELD:
+		proto_tree_add_item(mtree, hf_bittorrent_bitfield_data, tvb, offset, tvb_length_remaining(tvb, offset), FALSE); 
+		break;
+		
+	case BITTORRENT_MESSAGE_PIECE:
+		proto_tree_add_item(mtree, hf_bittorrent_piece_index, tvb, offset, 4, FALSE); offset += 4;
+		proto_tree_add_item(mtree, hf_bittorrent_piece_begin, tvb, offset, 4, FALSE); offset += 4;
+		proto_tree_add_item(mtree, hf_bittorrent_piece_data, tvb, offset, tvb_length_remaining(tvb, offset), FALSE);
+		break;
+	}
 }
 
 static void dissect_bittorrent_welcome (tvbuff_t *tvb, packet_info *pinfo _U_, proto_tree *tree)
 {
-   int offset = 0;
-   int i;
-   const char *version;
-   
-   if (check_col(pinfo->cinfo, COL_INFO)) {
-      col_set_str(pinfo->cinfo, COL_INFO, "BitTorrent Handshake");
-   }
-   
-   /* It has already been verified that 20 bytes exist prior to the call of this function */
-   proto_tree_add_item(tree, hf_bittorrent_prot_name_len, tvb, offset, 1, FALSE); offset+=1;
-   proto_tree_add_item(tree, hf_bittorrent_prot_name, tvb, offset, 19, FALSE); offset += 19;
-   if(tvb_bytes_exist(tvb, offset, 8)) {
-      proto_tree_add_item(tree, hf_bittorrent_reserved, tvb, offset, 8, FALSE); offset += 8;
-   } else {
-      proto_tree_add_text(tree, tvb, offset, 20, "The reserved field is missing");
-   }
-   
-   if(tvb_bytes_exist(tvb, offset, 20)) {
-      proto_tree_add_item(tree, hf_bittorrent_sha1_hash, tvb, offset, 20, FALSE); offset += 20;
-   } else {
-      proto_tree_add_text(tree, tvb, offset, 20, "SHA1 hash key field is missing");
+	int offset = 0;
+
+	if (check_col(pinfo->cinfo, COL_INFO)) {
+	    col_set_str(pinfo->cinfo, COL_INFO, "BitTorrent Handshake");
+	}
+
+	proto_tree_add_item(tree, hf_bittorrent_prot_name_len, tvb, offset, 1, FALSE); offset+=1;
+	proto_tree_add_item(tree, hf_bittorrent_prot_name, tvb, offset, 19, FALSE); offset += 19;
+	proto_tree_add_item(tree, hf_bittorrent_reserved, tvb, offset, 8, FALSE); offset += 8;
+	proto_tree_add_item(tree, hf_bittorrent_sha1_hash, tvb, offset, 20, FALSE); offset += 20;
+	proto_tree_add_item(tree, hf_bittorrent_peer_id, tvb, offset, 20, FALSE); offset += 20;
 }
-   if(tvb_bytes_exist(tvb, offset, 20)) {
-      proto_tree_add_item(tree, hf_bittorrent_peer_id, tvb, offset, 20, FALSE);
-      if(decode_client_information) {
-         for(i = 0; peer_id[i].id[0] != '\0'; ++i)
-         {
-            if(tvb_memeql(tvb, offset, peer_id[i].id, strlen(peer_id[i].id)) == 0) {
-            /*	The version number is 4 numeric characters for the
-            client ids beginning with '-' and 3 characters for the
-               rest. */
-               version = tvb_get_ptr(tvb, offset + strlen(peer_id[i].id), (i <= 11 ? 5 : 4));
-               ((char*)version)[(i <= 11 ? 4 : 3)] = '\0';
-               proto_tree_add_text(tree, tvb, offset, 20, "Client is %s v%s",
-                  peer_id[i].name, version);
-               break;
-            }
-         }
-      }
-      offset += 20;
-   } else {
-      proto_tree_add_text(tree, tvb, offset, 20, "The peer_id field is missing");
-   }
-}
 
 static void dissect_bittorrent_tcp_pdu (tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
 {
-   proto_item *ti;
-   
-   if (check_col(pinfo->cinfo, COL_PROTOCOL)) {
-      col_set_str(pinfo->cinfo, COL_PROTOCOL, "BitTorrent");
-   }
-   
-   if (check_col(pinfo->cinfo, COL_INFO)) {
-      col_set_str(pinfo->cinfo, COL_INFO, "BitTorrent Peer-To-Peer connection");
-   }
-   
-   ti = proto_tree_add_text(tree, tvb, 0, -1, "BitTorrent");
-   
-   tree = proto_item_add_subtree(ti, ett_bittorrent);
-   
-   if (tvb_get_guint8(tvb, 0) == 19 &&
-      tvb_memeql(tvb, 1, "BitTorrent protocol", 19) == 0) {
-      dissect_bittorrent_welcome(tvb, pinfo, tree);
-   } else {
-      dissect_bittorrent_message(tvb, pinfo, tree);
-   }
+	proto_item *ti;
+	
+	if (check_col(pinfo->cinfo, COL_PROTOCOL)) {
+		col_set_str(pinfo->cinfo, COL_PROTOCOL, "BitTorrent");
+	}
+
+	if (check_col(pinfo->cinfo, COL_INFO)) {
+	    col_set_str(pinfo->cinfo, COL_INFO, "BitTorrent Peer-To-Peer connection");
+	}
+
+	ti = proto_tree_add_text(tree, tvb, 0, -1, "BitTorrent");
+
+	tree = proto_item_add_subtree(ti, ett_bittorrent);
+
+	if (tvb_get_guint8(tvb, 0) == 19) {
+		dissect_bittorrent_welcome(tvb, pinfo, tree);
+	} else {
+		dissect_bittorrent_message(tvb, pinfo, tree);
+	}
 }
 
 static void dissect_bittorrent (tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
 {
-   tcp_dissect_pdus(tvb, pinfo, tree, bittorrent_desegment, BITTORRENT_HEADER_LENGTH,
-                    get_bittorrent_pdu_length, dissect_bittorrent_tcp_pdu);
+	tcp_dissect_pdus(tvb, pinfo, tree, bittorrent_desegment, 4, get_bittorrent_pdu_length, dissect_bittorrent_tcp_pdu);
 }
 
+static const guint8 bittorrent_magic[20] = {
+	19,
+	'B', 'i', 't', 'T', 'o', 'r', 'r', 'e', 'n', 't',
+	' ', 'p', 'r', 'o', 't', 'o', 'c', 'o', 'l'
+};
+
 static gboolean test_bittorrent_packet (tvbuff_t *tvb, packet_info *pinfo,
-                                        proto_tree *tree)
+										     proto_tree *tree)
 {
-   conversation_t *conversation;
+	conversation_t *conversation;
 
-   if (tvb_bytes_exist(tvb, 0, 20) &&
-       tvb_get_guint8(tvb, 0) == 19 &&
-       tvb_memeql(tvb, 1, "BitTorrent protocol", 19) == 0) {
-      conversation = conversation_new (pinfo->fd->num, &pinfo->src, &pinfo->dst, pinfo->ptype, pinfo->srcport, pinfo->destport, 0);
+	if (tvb_memeql(tvb, 0, bittorrent_magic, sizeof bittorrent_magic) == -1) {
+		return FALSE;
+	}
 
-      conversation_set_dissector(conversation, dissector_handle);
+	conversation = conversation_new (pinfo->fd->num, &pinfo->src, &pinfo->dst, pinfo->ptype, pinfo->srcport, pinfo->destport, 0);
 
-      dissect_bittorrent(tvb, pinfo, tree);
+	conversation_set_dissector(conversation, bittorrent_handle);
+	
+	dissect_bittorrent(tvb, pinfo, tree);
 
-      return TRUE;
+	return TRUE;
 }
 
-   return FALSE;
-}
 
+
 void
 proto_register_bittorrent(void)
 {
-   static hf_register_info hf[] = {
-      { &hf_bittorrent_field_length, 
-      { "Field Length", "bittorrent.length", FT_UINT32, BASE_DEC, NULL, 0x0, "", HFILL }
-      },
-      { &hf_bittorrent_prot_name_len,
-      { "Protocol Name Length", "bittorrent.protocol.name.length", FT_UINT8, BASE_DEC, NULL, 0x0, "", HFILL }
-      },
-      { &hf_bittorrent_prot_name,
-      { "Protocol Name", "bittorrent.protocol.name", FT_STRING, BASE_DEC, NULL, 0x0, "", HFILL }
-      },
-      { &hf_bittorrent_reserved,
-      { "Reserved Extension Bytes", "bittorrent.reserved", FT_BYTES, BASE_DEC, NULL, 0x0, "", HFILL }
-      },
-      { &hf_bittorrent_sha1_hash,
-      { "SHA1 Hash of info dictionary", "bittorrent.info_hash", FT_BYTES, BASE_DEC, NULL, 0x0, "", HFILL }
-      },
-      { &hf_bittorrent_peer_id,
-      { "Peer ID", "bittorrent.peer_id", FT_BYTES, BASE_DEC, NULL, 0x0, "", HFILL }
-      },
-      { &hf_bittorrent_msg_len,
-      { "Message Length", "bittorrent.msg.length", FT_UINT32, BASE_DEC, NULL, 0x0, "", HFILL }
-      }, 
-      { &hf_bittorrent_msg_type,
-      { "Message Type", "bittorrent.msg.type", FT_UINT8, BASE_DEC, VALS(bittorrent_messages), 0x0, "", HFILL }
-      },
-      { &hf_bittorrent_bitfield_data,
-      { "Bitfield data", "bittorrent.msg.bitfield", FT_BYTES, BASE_HEX, NULL, 0x0, "", HFILL }
-      },
-      { &hf_bittorrent_piece_index,
-      { "Piece index", "bittorrent.piece.index", FT_UINT32, BASE_HEX, NULL, 0x0, "", HFILL }
-      },
-      { &hf_bittorrent_piece_begin,
-      { "Begin offset of piece", "bittorrent.piece.begin", FT_UINT32, BASE_HEX, NULL, 0x0, "", HFILL }
-      },
-      { &hf_bittorrent_piece_data, 
-      { "Data in a piece", "bittorrent.piece.data", FT_BYTES, BASE_HEX, NULL, 0x0, "", HFILL }
-      },
-      { &hf_bittorrent_piece_length,
-         { "Piece Length", "bittorrent.piece.length", FT_UINT32, BASE_HEX, NULL, 0x0, "", HFILL }
-      }
+
+  static hf_register_info hf[] = {
+	  { &hf_bittorrent_field_length, 
+		  { "Field Length", "bittorrent.length", FT_UINT32, BASE_DEC, NULL, 0x0, "", HFILL },
+	  },
+	  { &hf_bittorrent_prot_name_len,
+		  { "Protocol Name Length", "bittorrent.protocol.name.length", FT_UINT8, BASE_DEC, NULL, 0x0, "", HFILL },
+	  },
+	  { &hf_bittorrent_prot_name,
+		  { "Protocol Name", "bittorrent.protocol.name", FT_STRING, BASE_DEC, NULL, 0x0, "", HFILL },
+	  },
+	  { &hf_bittorrent_reserved,
+		  { "Reserved Extension Bytes", "bittorrent.reserved", FT_BYTES, BASE_DEC, NULL, 0x0, "", HFILL },
+	  },
+	  { &hf_bittorrent_sha1_hash,
+		  { "SHA1 Hash of info dictionary", "bittorrent.info_hash", FT_BYTES, BASE_DEC, NULL, 0x0, "", HFILL },
+	  },
+	  { &hf_bittorrent_peer_id,
+		  { "Peer ID", "bittorrent.peer_id", FT_BYTES, BASE_DEC, NULL, 0x0, "", HFILL },
+	  },
+	  { &hf_bittorrent_msg_len,
+		  { "Message Length", "bittorrent.msg.length", FT_UINT32, BASE_HEX, NULL, 0x0, "", HFILL },
+	  }, 
+	  { &hf_bittorrent_msg_type,
+		  { "Message Type", "bittorrent.msg.type", FT_UINT8, BASE_HEX, VALS(bittorrent_messages), 0x0, "", HFILL },
+	  },
+	  { &hf_bittorrent_bitfield_data,
+		  { "Bitfield data", "bittorrent.msg.bitfield", FT_BYTES, BASE_HEX, NULL, 0x0, "", HFILL },
+	  },
+	  { &hf_bittorrent_piece_index,
+		  { "Piece index", "bittorrent.piece.index", FT_UINT32, BASE_HEX, NULL, 0x0, "", HFILL },
+	  },
+	  { &hf_bittorrent_piece_begin,
+		  { "Begin offset of piece", "bittorrent.piece.begin", FT_UINT32, BASE_HEX, NULL, 0x0, "", HFILL },
+	  },
+	  { &hf_bittorrent_piece_data, 
+		  { "Data in a piece", "bittorrent.piece.data", FT_BYTES, BASE_HEX, NULL, 0x0, "", HFILL },
+	  },
+	  { &hf_bittorrent_piece_length,
+		  { "Piece Length", "bittorrent.piece.length", FT_UINT32, BASE_HEX, NULL, 0x0, "", HFILL },
+	  },
   };
 
   static gint *ett[] = {
     &ett_bittorrent,
-    &ett_bittorrent_msg,
-    &ett_peer_id
+	&ett_bittorrent_msg,
   };
 
   module_t *bittorrent_module;
@@ -391,19 +297,13 @@
     "Whether the BitTorrent dissector should reassemble messages spanning multiple TCP segments."
     " To use this option, you must also enable \"Allow subdissectors to reassemble TCP streams\" in the TCP protocol settings.",
     &bittorrent_desegment);
-  prefs_register_bool_preference(bittorrent_module, "decode_client",
-     "Decode the peer_id of the handshake messages",
-     "Enabling this will tell which BitTorrent client that produced the handshake message",
-     &decode_client_information);
 }
 
 
 void
 proto_reg_handoff_bittorrent(void)
 {
-   register_dissector("bittorrent", dissect_bittorrent, proto_bittorrent);
-   heur_dissector_add("tcp", test_bittorrent_packet, proto_bittorrent);
-
-   dissector_handle = find_dissector("bittorrent");
-   DISSECTOR_ASSERT(dissector_handle);
+	register_dissector("bittorrent", dissect_bittorrent, proto_bittorrent);
+	bittorrent_handle = find_dissector("bittorrent");
+  	heur_dissector_add("tcp", test_bittorrent_packet, proto_bittorrent);
 }