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.
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
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.
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 >=4It'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
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 >=4It'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++; }@@ -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.
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
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
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?
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.
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
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
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.
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.
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
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.
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
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
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
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
-
Burkhardt Petermann -
Darren Salt -
Dr. Werner Fink -
Klaus Schmidinger -
Rainer Zocholl -
Reinhard Nissl -
s.huelswitt＠gmx.de -
tinconn＠virgilio.it