Libdvdread misses hidden files and causes segfaults to calling programs
Package: libdvdread4 Version: 4.1.4-1219-2 and others No error message, but symptoms are usually segfault when reading, for example the Movie DVD Thor. This results from a new anti-copy scheme where the real video_ts.ifo is hidden. Use of the decoy video_ts.ifo results in a unplayable DVD. Discussion is here: http://ubuntuforums.org/showthread.php?p=11257764 Patch is here: diff -ru libdvdread-4.1.3/src/dvd_udf.c libdvdread-4.1.3.fixed/src/dvd_udf.c --- libdvdread-4.1.3/src/dvd_udf.c 2008-09-06 15:55:51.000000000 -0600 +++ libdvdread-4.1.3.fixed/src/dvd_udf.c 2011-09-16 14:07:04.000000000 -0600 @@ -331,21 +331,26 @@ /* This is wrong with regard to endianess */ #define GETN(p, n, target) memcpy(target,&data[p], n) -static int Unicodedecode( uint8_t *data, int len, char *target ) +static int Unicodedecode(uint8_t *data, int len, char *target) { - int p = 1, i = 0; + len--; + data++; + if (data[-1] == 8 ) + memcpy(target, data, len); + else if (data[-1] == 16) { + int i; - if( ( data[ 0 ] == 8 ) || ( data[ 0 ] == 16 ) ) do { - if( data[ 0 ] == 16 ) p++; /* Ignore MSB of unicode16 */ - if( p< len ) { - target[ i++ ] = data[ p++ ]; + for (i = 0; i< len; i++) { + if (data[i*2] == 0) + target[i] = data[i*2+1]; + else + target[i] = 0; } - } while( p< len ); + } + target[len] = '\0'; - target[ i ] = '\0'; return 0; } - static int UDFDescriptor( uint8_t *data, uint16_t *TagID ) { *TagID = GETN2(0);
On 09/18/2011 11:12 AM, doug Springer wrote:
Package: libdvdread4 Version: 4.1.4-1219-2 and others
No error message, but symptoms are usually segfault when reading, for example the Movie DVD Thor. This results from a new anti-copy scheme where the real video_ts.ifo is hidden. Use of the decoy video_ts.ifo results in a unplayable DVD.
This patch has been submitted twice already, but I like my version better. The other 2 unnecessarily truncate the string when something is found in the high byte. This fills that string the same as pre-patch, but returns a code indicating that junk was found in the MSB. -- John GnuPG fingerprint: CADFA1DB594CA50EB79427284617578AADE759A1
On Mon, Sep 19, 2011 at 03:22:55PM -0700, John Stebbins wrote:
On 09/18/2011 11:12 AM, doug Springer wrote:
Package: libdvdread4 Version: 4.1.4-1219-2 and others
No error message, but symptoms are usually segfault when reading, for example the Movie DVD Thor. This results from a new anti-copy scheme where the real video_ts.ifo is hidden. Use of the decoy video_ts.ifo results in a unplayable DVD.
This patch has been submitted twice already, but I like my version better. The other 2 unnecessarily truncate the string when something is found in the high byte. This fills that string the same as pre-patch, but returns a code indicating that junk was found in the MSB.
I that case I don't see the point in making things hard to review by rewriting the whole function. Why not just Index: dvd_udf.c =================================================================== --- dvd_udf.c (revision 1233) +++ dvd_udf.c (working copy) @@ -329,16 +329,17 @@ static int Unicodedecode( uint8_t *data, int len, char *target ) { int p = 1, i = 0; + int err = 0; if( ( data[ 0 ] == 8 ) || ( data[ 0 ] == 16 ) ) do { - if( data[ 0 ] == 16 ) p++; /* Ignore MSB of unicode16 */ + if( data[ 0 ] == 16 ) err |= data[ p++ ]; /* Ignore MSB of unicode16 */ if( p < len ) { target[ i++ ] = data[ p++ ]; } } while( p < len ); target[ i ] = '\0'; - return 0; + return !err; } static int UDFDescriptor( uint8_t *data, uint16_t *TagID )
On 09/20/2011 12:27 AM, Reimar Döffinger wrote:
On Mon, Sep 19, 2011 at 03:22:55PM -0700, John Stebbins wrote:
On 09/18/2011 11:12 AM, doug Springer wrote:
Package: libdvdread4 Version: 4.1.4-1219-2 and others
No error message, but symptoms are usually segfault when reading, for example the Movie DVD Thor. This results from a new anti-copy scheme where the real video_ts.ifo is hidden. Use of the decoy video_ts.ifo results in a unplayable DVD.
This patch has been submitted twice already, but I like my version better. The other 2 unnecessarily truncate the string when something is found in the high byte. This fills that string the same as pre-patch, but returns a code indicating that junk was found in the MSB.
I that case I don't see the point in making things hard to review by rewriting the whole function. Why not just Index: dvd_udf.c =================================================================== --- dvd_udf.c (revision 1233) +++ dvd_udf.c (working copy) @@ -329,16 +329,17 @@ static int Unicodedecode( uint8_t *data, int len, char *target ) { int p = 1, i = 0; + int err = 0;
if( ( data[ 0 ] == 8 ) || ( data[ 0 ] == 16 ) ) do { - if( data[ 0 ] == 16 ) p++; /* Ignore MSB of unicode16 */ + if( data[ 0 ] == 16 ) err |= data[ p++ ]; /* Ignore MSB of unicode16 */ if( p< len ) { target[ i++ ] = data[ p++ ]; } } while( p< len );
target[ i ] = '\0'; - return 0; + return !err; }
static int UDFDescriptor( uint8_t *data, uint16_t *TagID )
In that case, you need to observe the return value in UDFDescriptor, which isn't done currenty: static int UDFDescriptor( uint8_t *data, uint16_t *TagID ) @@ -490,7 +501,9 @@ L_FI = GETN1(19); UDFLongAD(&data[20], FileICB); L_IU = GETN2(36); - if (L_FI) Unicodedecode(&data[38 + L_IU], L_FI, FileName); + if (L_FI) { + if (!Unicodedecode(&data[38 + L_IU], L_FI, FileName)) FileName[0] = '\0'; + } else FileName[0] = '\0'; return 4 * ((38 + L_FI + L_IU + 3) / 4); } -- Cheers, Alex
This patch has been submitted twice already, but I like my version better. The other 2 unnecessarily truncate the string when something is found in the high byte. This fills that string the same as pre-patch, but returns a code indicating that junk was found in the MSB.
I[n] that case I don't see the point in making things hard to review by rewriting the whole function. Why not just Index: dvd_udf.c =================================================================== --- dvd_udf.c (revision 1233) +++ dvd_udf.c (working copy) @@ -329,16 +329,17 @@ static int Unicodedecode( uint8_t *data, int len, char *target ) { int p = 1, i = 0; + int err = 0;
if( ( data[ 0 ] == 8 ) || ( data[ 0 ] == 16 ) ) do { - if( data[ 0 ] == 16 ) p++; /* Ignore MSB of unicode16 */ + if( data[ 0 ] == 16 ) err |= data[ p++ ]; /* Ignore MSB of unicode16 */ if( p< len ) { target[ i++ ] = data[ p++ ]; } } while( p< len );
target[ i ] = '\0'; - return 0; + return !err; }
static int UDFDescriptor( uint8_t *data, uint16_t *TagID )
In that case, you need to observe the return value in UDFDescriptor, which isn't done currenty:
static int UDFDescriptor( uint8_t *data, uint16_t *TagID ) @@ -490,7 +501,9 @@ L_FI = GETN1(19); UDFLongAD(&data[20], FileICB); L_IU = GETN2(36); - if (L_FI) Unicodedecode(&data[38 + L_IU], L_FI, FileName); + if (L_FI) { + if (!Unicodedecode(&data[38 + L_IU], L_FI, FileName)) FileName[0] = '\0'; + } else FileName[0] = '\0'; return 4 * ((38 + L_FI + L_IU + 3) / 4); }
I don't think Reimar was actually submitting a patch but rather making a critique of the Unicodedecode() part of the patch. Which means that it would be handy if another patch were submitted that made the small change that Reimar suggests along w/ the additional conditional that is required to make the right decision on the decoded value. Or annotate the original patch enough that it is much clearer what is going on. If John is up for it this is a good test of his newly minted svn write access. ;) E -- Erik Hovland erik@hovland.org http://hovland.org/
On 20 Sep 2011, at 00:53, Erik Hovland <erik@hovland.org> wrote:
This patch has been submitted twice already, but I like my version better. The other 2 unnecessarily truncate the string when something is found in the high byte. This fills that string the same as pre-patch, but returns a code indicating that junk was found in the MSB.
I[n] that case I don't see the point in making things hard to review by rewriting the whole function. Why not just Index: dvd_udf.c =================================================================== --- dvd_udf.c (revision 1233) +++ dvd_udf.c (working copy) @@ -329,16 +329,17 @@ static int Unicodedecode( uint8_t *data, int len, char *target ) { int p = 1, i = 0; + int err = 0;
if( ( data[ 0 ] == 8 ) || ( data[ 0 ] == 16 ) ) do { - if( data[ 0 ] == 16 ) p++; /* Ignore MSB of unicode16 */ + if( data[ 0 ] == 16 ) err |= data[ p++ ]; /* Ignore MSB of unicode16 */ if( p< len ) { target[ i++ ] = data[ p++ ]; } } while( p< len );
target[ i ] = '\0'; - return 0; + return !err; }
static int UDFDescriptor( uint8_t *data, uint16_t *TagID )
In that case, you need to observe the return value in UDFDescriptor, which isn't done currenty:
static int UDFDescriptor( uint8_t *data, uint16_t *TagID ) @@ -490,7 +501,9 @@ L_FI = GETN1(19); UDFLongAD(&data[20], FileICB); L_IU = GETN2(36); - if (L_FI) Unicodedecode(&data[38 + L_IU], L_FI, FileName); + if (L_FI) { + if (!Unicodedecode(&data[38 + L_IU], L_FI, FileName)) FileName[0] = '\0'; + } else FileName[0] = '\0'; return 4 * ((38 + L_FI + L_IU + 3) / 4); }
I don't think Reimar was actually submitting a patch but rather making a critique of the Unicodedecode() part of the patch.
Exactly, it is also completely untested and not all that carefully verified. And I don't mind a rewrite of the function, it is not very nice as it is currently. However I do not like combining rewrite and bug fixes without a need. And also (no offence intended) the rewritten functions didn't look too great to me. Until we are done with bike-shedding on a rewritten function it might be some time. So my suggestion is a "we are really sure we didn't break anything due to a typo or such" kind of change first.
Ok, then here goes a minimal invasive patch: I still like my first try better, but I understand noone has any desire to review it. On a side note: The linux UDF driver does show the file correctly. Since libdvdread brings its own UDF implementation, it doesn't need to mount the DVD in order to access it. This much I understand. Now a question: would it be possible to use such an approach also for blu-ray? Currently, I have to mount the drive in order to access it with the br:// interface, but if my thoughts are correct, the same method could be applied to it, so one wouldn't have to mount the drive beforehand. On the other hand, I reckon there are much more different file accesses to be done, which are much more easier to go through the normal api instead of the own UDF driver. Again, if libdvdread would have used the linux driver (and would require to mount the device), then this error wouldn't have happened (and I would call it an error, since it's a legal (?) file name that gets truncated and therefore fools the current implementation. -- Cheers, Alex
On 09/19/2011 04:18 PM, Alexander Roalter wrote:
Ok, then here goes a minimal invasive patch:
I still like my first try better, but I understand noone has any desire to review it.
On a side note: The linux UDF driver does show the file correctly. Since libdvdread brings its own UDF implementation, it doesn't need to mount the DVD in order to access it. This much I understand. Now a question: would it be possible to use such an approach also for blu-ray? Currently, I have to mount the drive in order to access it with the br:// interface, but if my thoughts are correct, the same method could be applied to it, so one wouldn't have to mount the drive beforehand. On the other hand, I reckon there are much more different file accesses to be done, which are much more easier to go through the normal api instead of the own UDF driver. Again, if libdvdread would have used the linux driver (and would require to mount the device), then this error wouldn't have happened (and I would call it an error, since it's a legal (?) file name that gets truncated and therefore fools the current implementation.
This last patch looks good to me. But note that I have not yet had a chance to test this fix. Still waiting on delivery of my Thor disc. Regarding UDF for bd access. The version of UDF used for DVD is older than the one used for BD. So at the least, the code needs to be updated. However, personally I do not like the tightly bound implementation dvdread uses. I would prefer a separate libudf that an application can link in (or not link in as it chooses). The relevant functions (read, write, readdir, etc...) in libbluray would have to be substitutable by some registration mechanism. And this has been done already (see bd_register_file and bd_register_dir). So it would be up to the application to add the udf library and create the linkage to it through the file and directory methods. -- John GnuPG fingerprint: CADFA1DB594CA50EB79427284617578AADE759A1
On a side note: The linux UDF driver does show the file correctly. Since libdvdread brings its own UDF implementation, it doesn't need to mount the DVD in order to access it. This much I understand. Now a question: would it be possible to use such an approach also for blu-ray? Currently, I have to mount the drive in order to access it with the br:// interface, but if my thoughts are correct, the same method could be applied to it, so one wouldn't have to mount the drive beforehand. On the other hand, I reckon there are much more different file accesses to be done, which are much more easier to go through the normal api instead of the own UDF driver.
I wrote the required patches to read BD UDF filesystems for libdvdread. One could either have them accepted here, or simply rename a version of libdvdread to libbdread, and take out the DVD specific calls. I'm a little disappointed in Hollywood for not using the VOB gaps to their advantage, as libdvdread still does not follow chains correctly (another patch submitted) Oh well :) Lund -- Jorgen Lundman | <lundman@lundman.net> Unix Administrator | +81 (0)3 -5456-2687 ext 1017 (work) Shibuya-ku, Tokyo | +81 (0)90-5578-8500 (cell) Japan | +81 (0)3 -3375-1767 (home)
Ok, then here goes a minimal invasive patch:
Would it be possible to seed this patch on at least the Ubuntu forum to try to get some testing w/ it? It would be helpful if some people reported success w/ it. Thanks E -- Erik Hovland erik@hovland.org http://hovland.org/
On Wed, Sep 21, 2011 at 08:34:53AM -0700, Erik Hovland wrote:
Ok, then here goes a minimal invasive patch:
Would it be possible to seed this patch on at least the Ubuntu forum to try to get some testing w/ it? It would be helpful if some people reported success w/ it.
Go right ahead :) Diego
Am 26.09.2011 10:43, schrieb Diego Biurrun:
On Wed, Sep 21, 2011 at 08:34:53AM -0700, Erik Hovland wrote:
Ok, then here goes a minimal invasive patch:
Would it be possible to seed this patch on at least the Ubuntu forum to try to get some testing w/ it? It would be helpful if some people reported success w/ it.
Go right ahead :)
To my knowledge, two discs are currently affected by this: Thor (european and US) and Rango. So much for testing (apart from regression testing against normal discs). -- cheers, Alex
On Mon, Sep 26, 2011 at 01:45:13PM +0200, Alexander Roalter wrote :
Am 26.09.2011 10:43, schrieb Diego Biurrun:
On Wed, Sep 21, 2011 at 08:34:53AM -0700, Erik Hovland wrote:
Ok, then here goes a minimal invasive patch:
Would it be possible to seed this patch on at least the Ubuntu forum to try to get some testing w/ it? It would be helpful if some people reported success w/ it.
Go right ahead :)
To my knowledge, two discs are currently affected by this: Thor (european and US) and Rango. So much for testing (apart from regression testing against normal discs).
Confirmed from many VLC users. Best Regards, -- Jean-Baptiste Kempf http://www.jbkempf.com/ - +33 672 704 734 Sent from my Electronic Device
On 09/19/2011 04:18 PM, Alexander Roalter wrote:
Ok, then here goes a minimal invasive patch:
I still like my first try better, but I understand noone has any desire to review it.
I finally received my Thor DVD. I have verified that this patch works with this disc. -- John GnuPG fingerprint: CADFA1DB594CA50EB79427284617578AADE759A1
On Monday, 26 September 2011 at 22:39, John Stebbins wrote:
On 09/19/2011 04:18 PM, Alexander Roalter wrote:
Ok, then here goes a minimal invasive patch:
I still like my first try better, but I understand noone has any desire to review it.
I finally received my Thor DVD. I have verified that this patch works with this disc.
Patch applied, then. Regards, Dominik -- Fedora http://fedoraproject.org/wiki/User:Rathann RPMFusion http://rpmfusion.org | MPlayer http://mplayerhq.hu "Faith manages." -- Delenn to Lennier in Babylon 5:"Confessions and Lamentations"
I find it comical that the release of Thor is what is causing this turbulence. Could we be more nerdy. On Mon, Sep 19, 2011 at 3:27 PM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
On Mon, Sep 19, 2011 at 03:22:55PM -0700, John Stebbins wrote:
On 09/18/2011 11:12 AM, doug Springer wrote:
Package: libdvdread4 Version: 4.1.4-1219-2 and others
No error message, but symptoms are usually segfault when reading, for example the Movie DVD Thor. This results from a new anti-copy scheme where the real video_ts.ifo is hidden. Use of the decoy video_ts.ifo results in a unplayable DVD.
This patch has been submitted twice already, but I like my version better. The other 2 unnecessarily truncate the string when something is found in the high byte. This fills that string the same as pre-patch, but returns a code indicating that junk was found in the MSB.
I[n] that case I don't see the point in making things hard to review by rewriting the whole function.
I agree that both patches that chose to rewrite the function are hard to review. But the function looks like a hack to begin w/. I am not opposed to a rewrite if we can agree that it is the best option. Note that some more annotation on the rewrite patches would help us review it faster.
Why not just Index: dvd_udf.c =================================================================== --- dvd_udf.c (revision 1233) +++ dvd_udf.c (working copy) @@ -329,16 +329,17 @@ static int Unicodedecode( uint8_t *data, int len, char *target ) { int p = 1, i = 0; + int err = 0;
if( ( data[ 0 ] == 8 ) || ( data[ 0 ] == 16 ) ) do { - if( data[ 0 ] == 16 ) p++; /* Ignore MSB of unicode16 */ + if( data[ 0 ] == 16 ) err |= data[ p++ ]; /* Ignore MSB of unicode16 */
We have to drop the comment if we are going to stop ignoring the MSB. But I appreciate that a 4 line patch is much easier to review then a rewrite. E -- Erik Hovland erik@hovland.org http://hovland.org/
participants (9)
-
Alexander Roalter -
Diego Biurrun -
Dominik 'Rathann' Mierzejewski -
doug Springer -
Erik Hovland -
Jean-Baptiste Kempf -
John Stebbins -
Jorgen Lundman -
Reimar Döffinger