vdr-1.3.23: BUG: cPesAssembler dropped data resulting in delayed crash of VDR

Message ID 427A34A1.3010403@cadsoft.de
State New
Headers

Commit Message

Klaus Schmidinger May 5, 2005, 2:58 p.m. UTC
  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
  

Comments

Reinhard Nissl May 5, 2005, 6:58 p.m. UTC | #1
Hi,

Klaus Schmidinger wrote:

>> 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?!

Isn't it "automagically" used when a device (like cXineDevice) is 
operating in transfer mode ;-)

>> 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:
> 
> --- 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--;
>          }
> 
> Can you please verify if this does the same as your patch?

I've checked the patch and it looks good. Then I've applied the patch 
and checked the binary by playing the above mentioned recording: 
everything's fine!

Bye.
  
Klaus Schmidinger May 5, 2005, 8:07 p.m. UTC | #2
Reinhard Nissl wrote:
> Hi,
> 
> Klaus Schmidinger wrote:
> 
>>> 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?!
> 
> 
> Isn't it "automagically" used when a device (like cXineDevice) is 
> operating in transfer mode ;-)

Sure - but it sounded as if you were using it _explicitly_.

> ...
>> Can you please verify if this does the same as your patch?
> 
> 
> I've checked the patch and it looks good. Then I've applied the patch 
> and checked the binary by playing the above mentioned recording: 
> everything's fine!

Thanks.

Klaus
  
Burkhardt Petermann May 6, 2005, 5:16 p.m. UTC | #3
Hi,
my problem: after playing dts-cds with the mp3-plugin (0.9.10 or 0.9.12
under vdr 1.3.22) there will not any sound in the live-modus.
Only switching to a channel with more than one sound track und change there
to DD the sound will appear again.
Is there an easy way to solve this problem ?
Greeting
  Burkhardt
  
Darren Salt May 6, 2005, 5:44 p.m. UTC | #4
I demand that Burkhardt Petermann may or may not have written...

> my problem: after playing dts-cds with the mp3-plugin [...]

What does this have to do with cPesAssembler bugginess?
  
Stefan Huelswitt May 6, 2005, 5:58 p.m. UTC | #5
On 06 May 2005 "Burkhardt Petermann" <bpetermann@xyware.de> wrote:

> my problem: after playing dts-cds with the mp3-plugin (0.9.10 or 0.9.12
> under vdr 1.3.22) there will not any sound in the live-modus.
> Only switching to a channel with more than one sound track und change there
> to DD the sound will appear again.
> Is there an easy way to solve this problem ?

Well, it depends :-)

In past days the mp3 plugin used some special tricks to work
around this problem. AFAIK the new 261d firm takes care of this,
so the code has been removed.

So either you're not using a current firmware or the 261d
firmware cannot handle dts sound correctly.

You can give the option BROKEN_PCM=1 while compiling the plugin.
This reenables the pcm work around. May be this helps.

Regards.
  
Burkhardt Petermann May 6, 2005, 6:13 p.m. UTC | #6
Hi,

> Well, it depends :-)
> 
> In past days the mp3 plugin used some special tricks to work
> around this problem. AFAIK the new 261d firm takes care of this,
> so the code has been removed.
> 
> So either you're not using a current firmware or the 261d
> firmware cannot handle dts sound correctly.
> 
> You can give the option BROKEN_PCM=1 while compiling the plugin.
> This reenables the pcm work around. May be this helps.
> 
> Regards.
> 
thank you, but I'm using the firmware 261d ...
Greetings
  Burkhardt
  
Burkhardt Petermann May 6, 2005, 6:17 p.m. UTC | #7
Hi,
forgotten: also I've tried to set BROKEN_PCM and this is not a solution for
me.
My dvddriver is the cvs-snapshot form 2005-03-10 ...
Greetings
  Burkhardt

> -----Original Message-----
> From: vdr-bounces@linuxtv.org [mailto:vdr-bounces@linuxtv.org]On Behalf
> Of Burkhardt Petermann
> Sent: Friday, May 06, 2005 8:13 PM
> To: Klaus Schmidinger's VDR
> Subject: RE: [vdr] mp3-plugin and dts-CDs
>
>
> Hi,
>
> > Well, it depends :-)
> >
> > In past days the mp3 plugin used some special tricks to work
> > around this problem. AFAIK the new 261d firm takes care of this,
> > so the code has been removed.
> >
> > So either you're not using a current firmware or the 261d
> > firmware cannot handle dts sound correctly.
> >
> > You can give the option BROKEN_PCM=1 while compiling the plugin.
> > This reenables the pcm work around. May be this helps.
> >
> > Regards.
> >
> thank you, but I'm using the firmware 261d ...
> Greetings
>   Burkhardt
>
> _______________________________________________
> vdr mailing list
> vdr@linuxtv.org
> http://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
>
  
Stefan Huelswitt May 6, 2005, 6:55 p.m. UTC | #8
On 06 May 2005 "Burkhardt Petermann" <bpetermann@xyware.de> wrote:

> forgotten: also I've tried to set BROKEN_PCM and this is not a solution for
> me.
> My dvddriver is the cvs-snapshot form 2005-03-10 ...

I cannot test this myself as I don't have a dts cd.
Can you upload a sample somewhere?

Regards.
  
Burkhardt Petermann May 6, 2005, 7:41 p.m. UTC | #9
Hi Stefan,
you can download some samples from http://www.sr.se/multikanal/index.stm and
burn them to a cd.
Greetings
  Burkhardt
> On 06 May 2005 "Burkhardt Petermann" <bpetermann@xyware.de> wrote:
>
> > forgotten: also I've tried to set BROKEN_PCM and this is not a
> solution for
> > me.
> > My dvddriver is the cvs-snapshot form 2005-03-10 ...
>
> I cannot test this myself as I don't have a dts cd.
> Can you upload a sample somewhere?
>
> Regards.
>
  
tinconn@virgilio.it May 6, 2005, 7:43 p.m. UTC | #10
Hi
i like to know if this slowing of vdr develop means that vdr-1.4 is
near, or a simply pause for holidays and deserved rest...

thanks for your work...
kikko77
  
Stefan Huelswitt May 7, 2005, 6:24 a.m. UTC | #11
On 06 May 2005 "Burkhardt Petermann" <bpetermann@xyware.de> wrote:

> you can download some samples from http://www.sr.se/multikanal/index.stm and
> burn them to a cd.

I downloaded some samples from there and I cannot reproduce your
problem. The sounds returns everytime after playback. I played
the samples from harddisc but this shouldn't matter.

Using dvb-kernel (for kernel 2.4) cvs from 3.12.2004 and firmware
261d (probably 1.0rc5).

>> > My dvddriver is the cvs-snapshot form 2005-03-10 ...

Are you sure about the firmware version?

Regards.
  
Burkhardt Petermann May 7, 2005, 8:45 a.m. UTC | #12
Hi,
my output from driver loading:
DVB: AV7111(0) - firm f0240009, rtsl b0250018, vid 71010068, app 8000261d
I've testet nearly all DVB-drivers (also with kernel 2.4) and the problem
will not disappear.
Greetings
  Burkhardt


> I downloaded some samples from there and I cannot reproduce your
> problem. The sounds returns everytime after playback. I played
> the samples from harddisc but this shouldn't matter.
>
> Using dvb-kernel (for kernel 2.4) cvs from 3.12.2004 and firmware
> 261d (probably 1.0rc5).
>
> >> > My dvddriver is the cvs-snapshot form 2005-03-10 ...
>
> Are you sure about the firmware version?
>
> Regards.
>
> --
> Stefan Huelswitt
> s.huelswitt@gmx.de  | http://www.muempf.de/
>
> _______________________________________________
> vdr mailing list
> vdr@linuxtv.org
> http://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
>
  
Klaus Schmidinger May 7, 2005, 8:57 a.m. UTC | #13
tinconn@virgilio.it wrote:
> Hi
> i like to know if this slowing of vdr develop means that vdr-1.4 is
> near, or a simply pause for holidays and deserved rest...

Pretty much both. I'm working towards a stable 1.4, but didn't have
time to spare in the past few weeks.

Greetings
Klaus
  
Burkhardt Petermann May 7, 2005, 9:53 a.m. UTC | #14
Hi,

now I found the reason:
It's the extern decoder (creative DDTS-100) which has a problem with the new
firmware and vdr 1.3.2x (with vdr 1.2.6 and AC3overDVB-Patch there isn't the
problem).
If I change the decoder-input-channel und go then back --> sound ...
Next time I'll test my greater AV-Preamp (Tag AV30R) ...
Greetings
  Burkhardt

> -----Original Message-----
> From: vdr-bounces@linuxtv.org [mailto:vdr-bounces@linuxtv.org]On Behalf
> Of Burkhardt Petermann
> Sent: Saturday, May 07, 2005 10:46 AM
> To: Klaus Schmidinger's VDR
> Subject: RE: [vdr] mp3-plugin and dts-CDs
>
>
> Hi,
> my output from driver loading:
> DVB: AV7111(0) - firm f0240009, rtsl b0250018, vid 71010068, app 8000261d
> I've testet nearly all DVB-drivers (also with kernel 2.4) and the problem
> will not disappear.
> Greetings
>   Burkhardt
  
Dr. Werner Fink May 9, 2005, 10:16 a.m. UTC | #15
On Fri, May 06, 2005 at 05:58:42PM +0000, Stefan Huelswitt wrote:
> On 06 May 2005 "Burkhardt Petermann" <bpetermann@xyware.de> wrote:
> 
> > my problem: after playing dts-cds with the mp3-plugin (0.9.10 or 0.9.12
> > under vdr 1.3.22) there will not any sound in the live-modus.
> > Only switching to a channel with more than one sound track und change there
> > to DD the sound will appear again.
> > Is there an easy way to solve this problem ?
> 
> Well, it depends :-)
> 
> In past days the mp3 plugin used some special tricks to work
> around this problem. AFAIK the new 261d firm takes care of this,
> so the code has been removed.
> 
> So either you're not using a current firmware or the 261d
> firmware cannot handle dts sound correctly.

All DTS samples I have around can be handled by 261d.

     Werner
  

Patch

--- 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--;
          }