From patchwork Thu May 5 14:58:41 2005 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Klaus Schmidinger X-Patchwork-Id: 11865 Received: from tiger.cadsoft.de ([217.7.101.210]) by www.linuxtv.org with esmtp (Exim 4.34) id 1DThor-0003oe-P4 for vdr@linuxtv.org; Thu, 05 May 2005 16:59:17 +0200 Received: from raven.cadsoft.de (raven.cadsoft.de [217.7.101.211]) by tiger.cadsoft.de (8.12.7/8.12.7) with ESMTP id j45EwiMC027424 for ; Thu, 5 May 2005 16:59:01 +0200 Received: from [192.168.100.10] (hawk.cadsoft.de [192.168.100.10]) by raven.cadsoft.de (8.12.7/8.12.7) with ESMTP id j45EwhXY014559 for ; Thu, 5 May 2005 16:58:44 +0200 Message-ID: <427A34A1.3010403@cadsoft.de> Date: Thu, 05 May 2005 16:58:41 +0200 From: Klaus Schmidinger Organization: CadSoft Computer GmbH User-Agent: Mozilla Thunderbird 1.0.2 (X11/20050317) X-Accept-Language: en MIME-Version: 1.0 To: vdr@linuxtv.org Subject: Re: [vdr] vdr-1.3.23: BUG: cPesAssembler dropped data resulting in delayed crash of VDR References: <42754C90.7050501@gmx.de> In-Reply-To: <42754C90.7050501@gmx.de> X-BeenThere: vdr@linuxtv.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Klaus Schmidinger's VDR List-Id: Klaus Schmidinger's VDR List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 05 May 2005 14:59:18 -0000 Status: O X-Status: X-Keywords: X-UID: 1943 Reinhard Nissl wrote: > Hi, > > while recording the radio channel "Bayern 1" on Astra 19.2E from 0900 to > 1000 this morning, I've discoverd that VDR crashed at about 0932. > > After debugging the whole afternoon I've recently found the bug that > caused this crash. It's cPesAssembler that drops some data in certain > circumstances. The crash has nothing todo with recording this channel, > but with the transfer mode I'm using with vdr-xine, as it makes use of > cPesAssembler. Just curious: how can you use cPesAssembler? It's local to device.c?! > BTW: To reproduce this crash just tune to the mentioned channel in > transfer mode and wait for about 33 minutes. > > The bug has to do with cPesAssembler's length member, as it conforms to > this equation: > > length == 0 || length >=4 > > It's therefore impossible for cPesAssembler to store any fragments of > less than 4 bytes. But the problem is, that these bytes have modified > it's shift register "tag" which leads to wrong synchronisation on the > next fragment. > > For this channel, it seems to happen that cPesAssembler sees data blocks > that end with the sequence 00 00 01. These 3 bytes are stored in > cPesAssembler's "tag" member. But when the next data block arrives, > these bytes are ignored, as "length" is still 0, and further data is > dropped, as it needs to sychronize on the next start code (00 00 01). > > When the next fragment is to be stored, it results in "data" beeing > something like that > > 00 00 01 00 00 01 c0 ... > > As a result, the expected length evaluates to 6 bytes, but "length" > containes already more than that (in the above case it was 79). This > finally leads to "Rest" beeing -73 which then causes a crash in memcpy. > > The attached patch fixes this issue by introducing a new variable > "hasFragment". It helps to recognize that 1 to 3 bytes are already > stored in cPesAssembler. Feel free to drop this new variable and use > "length" instead. Where length is checked for beeing 0, it should be > checked for beeing < 4 and where "hasFragment" is set to true, "length" > should be set to any value != 0 and < 4. Thanks for debugging this pretty subtle problem. I'd rather go with using only 'length' for this and would suggest the following patch instead: Can you please verify if this does the same as your patch? Klaus --- device.c 2005/02/27 13:55:15 1.99 +++ device.c 2005/05/05 14:48:01 @@ -34,7 +34,7 @@ int ExpectedLength(void) { return PacketSize(data); } static int PacketSize(const uchar *data); int Length(void) { return length; } - const uchar *Data(void) { return data; } + const uchar *Data(void) { return data; } // only valid if Length() >= 4 void Reset(void); void Put(uchar c); void Put(const uchar *Data, int Length); @@ -76,7 +76,7 @@ void cPesAssembler::Put(uchar c) { - if (!length) { + if (length < 4) { tag = (tag << 8) | c; if ((tag & 0xFFFFFF00) == 0x00000100) { if (Realloc(4)) { @@ -84,6 +84,8 @@ length = 4; } } + else if (length < 3) + length++; } else if (Realloc(length + 1)) data[length++] = c; @@ -91,7 +93,7 @@ void cPesAssembler::Put(const uchar *Data, int Length) { - while (!length && Length > 0) { + while (length < 4 && Length > 0) { Put(*Data++); Length--; }