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:
--- 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?
Klaus
