diff mbox

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

Message ID 42754C90.7050501@gmx.de
State New
Headers

Commit Message

Reinhard Nissl May 1, 2005, 9:39 p.m. UTC 
  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.

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.

Bye.

Comments

Rainer Zocholl May 2, 2005, 8:57 p.m. UTC | #1 
rnissl@gmx.de(Reinhard Nissl)  01.05.05 23:39


>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.

>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.


Nice bug, nice analysis.

but that leds to a general question:
Why are "foreign"-based parameters to memcpy not range checked?
Nobody should trust any parameters he got from "outside",
we learned in grammar school.

Such "out of range" writings might cause very funny, 
hard to debug errors. Mostly not so "lulely" crashes the
box so reproducible.

Are there maybe more place where "memcpy" and similar critical
("buffer overflow sensitive") function are used with maybe "tainted"
unchecked values, which assumes that all previous calculations are 
done right and all parameters "mets" the specs?



Rainer
Reinhard Nissl May 3, 2005, 9:35 p.m. UTC | #2 
Hi,

Rainer Zocholl wrote:

[snip]

>>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.
> 
> Nice bug, nice analysis.
> 
> but that leds to a general question:
> Why are "foreign"-based parameters to memcpy not range checked?
> Nobody should trust any parameters he got from "outside",
> we learned in grammar school.
> 
> Such "out of range" writings might cause very funny, 
> hard to debug errors. Mostly not so "lulely" crashes the
> box so reproducible.
> 
> Are there maybe more place where "memcpy" and similar critical
> ("buffer overflow sensitive") function are used with maybe "tainted"
> unchecked values, which assumes that all previous calculations are 
> done right and all parameters "mets" the specs?

Well, as I wrote, it took the whole afternoon and evening to find the 
bug. I've read the code over and over and didn't find any case, where it 
would result in a negative length for memcpy(), even if the processed 
data would not match the specs. It was simply a programming error that 
dropped part of the data and resulted in a crash later.

So I don't think that each and every memcpy() should be protected in 
general. In this case, the code properly takes care of finding a valid 
length for memcpy(), so it should be sufficient to verify that the code 
operates properly, maybe by adding an assert() statement.

Anything else wouldn't make sense in that case (just my opinion).

Bye.
diff mbox

Patch

--- ../vdr-1.3.23-orig/device.c	2005-02-27 14:55:15.000000000 +0100
+++ device.c	2005-05-01 22:59:18.869069399 +0200
@@ -27,6 +27,7 @@  private:
   uint32_t tag;
   int length;
   int size;
+  bool hasFragment;
   bool Realloc(int Size);
 public:
   cPesAssembler(void);
@@ -39,6 +40,7 @@  public:
   void Put(uchar c);
   void Put(const uchar *Data, int Length);
   bool IsPes(void);
+  bool HasFragment(void);
   };
 
 cPesAssembler::cPesAssembler(void)
@@ -57,6 +59,12 @@  void cPesAssembler::Reset(void)
 {
   tag = 0xFFFFFFFF;
   length = 0;
+  hasFragment = false;
+}
+
+bool cPesAssembler::HasFragment(void)
+{
+  return hasFragment || length > 0;
 }
 
 bool cPesAssembler::Realloc(int Size)
@@ -67,6 +75,7 @@  bool cPesAssembler::Realloc(int Size)
      if (!data) {
         esyslog("ERROR: can't allocate memory for PES assembler");
         length = 0;
+        hasFragment = false;
         size = 0;
         return false;
         }
@@ -77,6 +86,7 @@  bool cPesAssembler::Realloc(int Size)
 void cPesAssembler::Put(uchar c)
 {
   if (!length) {
+     hasFragment = true;
      tag = (tag << 8) | c;
      if ((tag & 0xFFFFFF00) == 0x00000100) {
         if (Realloc(4)) {
@@ -985,7 +997,7 @@  int cDevice::PlayPes(const uchar *Data, 
      return 0;
      }
   int Result = 0;
-  if (pesAssembler->Length()) {
+  if (pesAssembler->HasFragment()) {
      // Make sure we have a complete PES header:
      while (pesAssembler->Length() < 6 && Length > 0) {
            pesAssembler->Put(*Data++);