Ethereal-dev: Re: Patch in reassemble.c (was Re: [Ethereal-dev] Crash in ethereal 0.10.8, some

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

Date Index Thread Index Other Months All Mailing Lists
Date Prev Date Next Thread Prev Thread Next
From: Guy Harris <gharris@xxxxxxxxx>
Date: Wed, 13 Jul 2005 00:14:05 -0700
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.