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

[Ulion]

2007/12/11, Andrew Calkin <calkina at geexbox.org>:
> 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.

No, %02d still may take 5 bytes when the integer need to be displayed
indeed need 5 bytes.

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

Yes, with your response, at least I rechecked the code to make it more safer.

-- 
Ulion