[FFmpeg-devel] [PATCH] fix possible crash if vpre before vcodec

[Reimar Döffinger]

On Fri, Feb 27, 2009 at 01:33:02PM +0000, Robert Swain wrote:
> 2009/2/27 Reimar D?ffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de>:
> > On Fri, Feb 27, 2009 at 12:36:27PM +0000, Robert Swain wrote:
> >> 2009/2/27 Michael Niedermayer <michaelni at gmx.at>:
> >> > On Fri, Feb 27, 2009 at 11:25:26AM +0100, Reimar D?ffinger wrote:
> >> >> I recently tried the following command
> >> >> ffmpeg -i baldursgate-logo.mve -vpre hq -vcodec libx264 -b 500k -an out.mp4
> >> >> and wondered why it would not work.
> >> >> strace showed the following:
> >> >> open("/home/reimar/.ffmpeg/hq.ffpreset", O_RDONLY) = -1 ENOENT (No such file or directory)
> >> >> open("/home/reimar/.ffmpeg/(null)-hq.ffpreset", O_RDONLY) = -1 ENOENT (No such file or directory)
> >> >> open("/usr/local/share/ffmpeg/hq.ffpreset", O_RDONLY) = -1 ENOENT (No such file or directory)
> >> >> open("/usr/local/share/ffmpeg/(null)-hq.ffpreset", O_RDONLY) = -1 ENOENT (No such file or directory)
> >> >> obviously ffmpeg tries to open the file already during option parsing (this could probably be considered
> >> >> the real bug). When -vcodec is set only afterwards it passes NULL to
> >> >> snprintf %s argument which may (and will) crash with some
> >> >> implementations.
> >> >> I propose a patch that at least avoids the crash, but e.g. this:
> >> >> -vcodec blub -vpre hq -vcodec libx264
> >> >> will still have the counter-intuitive behaviour of reading
> >> >> blub-hq.ffpreset but encoding with libx264.
> >> >
> >> > would it not make more sense to warn the user that the codec is not set
> >> > before -*pre instead of silently skiping searching for the file?
> >>
> >> I agree. I prefer command lines to error out on options that don't
> >> work as intended than to continue on. But a warning at least is
> >> absolutely necessary.
> >>
> >> Also, it should be documented that the preset options should come
> >> after the codec name.
> >
> > Well, the warning still would not work right, as the example
> > -vcodec blub -vpre hq -vcodec libx264
> > above shows, that's why I didn't consider it particularly useful.
> 
> Maybe the non-explicit codec idea wasn't so good after all. Maybe we
> should suggest people use -vpre libx264-hq?

I think it was a good idea, just the implementation is not optimal.
I don't know FFmpeg command line parsing well, but in MPlayer I'd just
store the -?pre string somewhere and open the file after parsing all the
other options...