[MPlayer-dev-eng] [PATCH] Chapter support improvement

[Andrew Calkin]

2007/12/11, Ulion <ulion2002 at gmail.com>:
> 2007/12/11, Andrew Calkin <calkina at geexbox.org>:
> > 2007/12/11, Ulion <ulion2002 at gmail.com>:
> > >
> > > By now I keep the property named 'chapter', it make me feel better.
> > > About what you said in the menu directly use chapter property, using
> > > property is a little complex than directly use the seek_chapter
> > > command, so I keep to use the command.
> > >
> > > Here's the updated patches:
> > > demuxer: add a functiond demuxer_chapter_time to get chapter start and
> > > end time if available. And a little chapter name format adjust.
> > > property & command: property chapter use same cmd id with command
> > > seek_chapter and handle all requests to command seek_chapter, old
> > > command handler for MP_CMD_SEEK_CHAPTER removed.
> > > libmenu: add a keybinding entry for chapsel in menu.conf. support
> > > display start time info in chapter menu and little custom format
> > > supported.
> >
> > I had a quick look through:
> >
> > osdmenu_chapter3.diff:
> >
> > in libmenu/menu_chapsel.c, is:
> >
> > +#include "stream/stream.h"
> >
> > required anymore? It doesn't seem you use it, so probably legacy from
> > when stream was accessed directly.
>
> It is needed for compiling, there's some struct in demuxer.h need it.

OK

> > Minor nit, in fill_menu(), you ensure that start_time >=0, so
> > timestr[9] should suffice, right? Perhaps a warning if hour > 99, as
> > it would indicate a bizarre file where the chapter information it
> > returns for the menu is bogus.
>
> Take it easy, it's just a local stack variable, indeed now I make it
> size 13 to make me feel safer with any start_time within 2147483648.

Sorry, I don't think I made my point clear. I was not worried about
how much memory was used, but rather that because for the hour
variable you always use %02d to format it, it will never use more than
9 chars, even if you increase the buffer size to 13, right? I just
could not see the relation between "%02d:%02d:%02d" and a buffer
length of 13.

> > In read_cmd(), even though cmdbuf[26] is quite large still no overrun
> > checking is performed. Perhaps snprintf(cmdbuf, 25...) and
> > cmdbuf[25]='\0'; to ensure that a bogus file chapter index doesn't
> > cause a buffer overrun. I didn't trace all the way through the code,
> > so I'm not sure if it's an issue or not.
>
> I don't see there's such needs, an cid is greater than 0, and max be
> the chapter_num, even if chapter_num is a large integer that takes
> maximum 10 bytes to be displayed, the lefted buffer can still hode a
> '\0' at the end. Or am I wrong?

You're probably right, I was just being devil's advocate. Malicious
files may exist, so I tend to favour a certain amount of paranoia when
I'm coding. Buffer overrun's can sometimes slip through, so I'll wait
for someone more experienced to comment- I just pointed it out.

//Andrew