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.
--
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@xxxxxx
