On 12/30/2012 06:57 AM, Fabian Keil wrote: > John Stebbins <[email protected]> wrote: > >> On 12/29/2012 10:55 AM, Fabian Keil wrote: >>> John Stebbins <[email protected]> wrote: >>> >>>> On 12/28/2012 04:15 AM, Fabian Keil wrote: >>>>> John Stebbins <[email protected]> wrote: >>>>> >>>>>> On 11/04/2012 06:33 PM, Fabian Keil wrote: >>>>>>> John Stebbins <[email protected]> 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()? >> 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(). > Thanks for the explanation and the patch review. > >> 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. > My impression is that vlc uses it to generate the title lengths > in the title menu. The crashes in dvdnav_describe_title_chapters() > already happen while opening the DVD, though, which at least makes > debugging easier. > > Unfortunately DVDs that trigger crashes with unpatched libdvdnav > versions usually also have a somewhat useless title menu, as it's > unclear from the lengths what the actual main title is: > http://www.fabiankeil.de/tmp/screenshot-vlc-title-menu-for-ghost-protocol.jpg > > From a user's perspective it would be great to also have a menu > that only shows titles that are reachable through the menu, > but I assume that's not trivial. >
True, it is non-trivial. But this is exactly the reason I created the dvdnav_dup() patch. This patch is in Erik's branch, but hasn't made it to the official repo yet. Creating duplicate dvdnav contexts allows you to do a recursive decent through menus doing virtual button pushes as you go. HandBrake uses this to hunt down a candidate for the main feature on the disc. It's not 100% accurate because my menu parser is a little lame still, but it works very well given it's lameness ;) -- John GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01 83F0 49F1 D7B2 60D4 D0F7
signature.asc
Description: OpenPGP digital signature
_______________________________________________ DVDnav-discuss mailing list [email protected] https://lists.mplayerhq.hu/mailman/listinfo/dvdnav-discuss
