[DVDnav-discuss] [PATCH] handle duplicate language units and pgc's more gracefully

[John Stebbins]

On 12/29/2012 10:55 AM, Fabian Keil wrote:
> John Stebbins <stebbins at jetheaddev.com> wrote:
>
>> On 12/28/2012 04:15 AM, Fabian Keil wrote:
>>> John Stebbins <stebbins at jetheaddev.com> wrote:
>>>
>>>> On 11/04/2012 06:33 PM, Fabian Keil wrote:
>>>>> John Stebbins <stebbins at jetheaddev.com> wrote:
>>>>>> What disc does it crash on?  HandBrake does not use
>>>>>> dvdnav_describe_title_chapters(), so this is a code path we have not
>>>>>> tested well enough it sounds like.  dvdbackup uses this?
>>>>> dvdbackup doesn't use dvdnav_describe_title_chapters() either.
>>>>>
>>>>> I failed to mention that the crashing application is vlc.
>>>>>
>>>>> The DVD is a rip of Monty Python season 3 DVD 1 where unreadable
>>>>> sector regions have been padded with surrounding sectors using a
>>>>> flawed padding strategy, which probably made the "copy" even less
>>>>> standard-compliant than the original.
>>>>>
>>>>> Given the content, it seems very appropriate that the attached patch,
>>>>> which prevents this and other crashes, is somewhat silly ...
>>>>>
>>>>> I made the skip conditions up while going through backtraces until
>>>>> vlc stopped crashing. I haven't properly tested yet how they affect
>>>>> other DVDs.
>>> I haven't noticed any problems with the patches so far.
>>>
>>> I recently had to use another patch (attached) for "Ghost Protocol",
>>> but I haven't verified that the problem actually was due to the pgc
>>> deduplication.
>> This error is not due to the pgc deduplication.  It is caused by an invalid cell_playback_offset value on the disc. 
>> libdvdread does not allocate a cell_playback in this case and applications must check that the value is not null.
> Do you count libdvdnav as application here, or is vlc really expected
> to check for this (and all the other things libdvdread might not allocate)
> before calling dvdnav_describe_title_chapters()?
>
> Fabian
>

I think some variant of your patch should be committed (although one of the comparisons seems to be trivailly false all
the time).  I was just pointing out that some applications also *use* cell_playback directly and need to check it for
null.  So some applications would never provoke this potential crash in dvdnav_describe_title_chapters().

There are actually many other places in libdvdnav where cell_playback is used unchecked.  I'm guessing the reason these
don't provoke obvious frequent crashes is that a title that contains a pgc with cell_playback_offset == 0 is an invalid
title and won't be accessible from the dvd menus.  So that makes me think that an application that provokes the crash
that is fixed by your patch is probably not strictly providing access based on the dvd menus and therefor may be
providing other means to provoke similar crashes.

FYI, this is the comparison that I believe is always false "&pgc->cell_playback[cellnr-1] == NULL".  It takes the
address of a variable which will always be non-null and compares it to null.

-- 
John      GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01  83F0 49F1 D7B2 60D4 D0F7


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: OpenPGP digital signature
URL: <http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/attachments/20121229/bcc45d4e/attachment.asc>