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

[Fabian Keil]

John Stebbins <stebbins at jetheaddev.com> wrote:

> 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()?

> 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.

> 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.

(gdb) p &pgc->cell_playback[cellnr-1]
$2 = (cell_playback_t *) 0x0
(gdb) p pgc->cell_playback
$3 = (cell_playback_t *) 0x0
(gdb) p cellnr
$4 = 1

I agree that the check is useless, though, and the adjusted
patch (attached) indeed still works as expected.

I believe it was the first (insufficient) fix attempt and I only
added the more reasonable pgc->cell_playback check later on without
realising that it's sufficient.

After all the main goal was being able to watch the movie ...

Fabian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: libdvdnav-4.2.0-Let-dvdnav_describe_title_chapters-skip-PGCs-with-mi.diff
Type: text/x-patch
Size: 833 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/attachments/20121230/5d77fea1/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/attachments/20121230/5d77fea1/attachment.asc>