Re: [FFmpeg-devel] [PATCH 1/2] lavu: add syntax for loading AV_OPT_TYPE_BINARY from files

From: Niklas Haas <ffmpeg@haasn.xyz>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH 1/2] lavu: add syntax for loading AV_OPT_TYPE_BINARY from files
Date: Tue, 8 Mar 2022 13:53:31 +0100
Message-ID: <20220308135331.GC28717@haasn.xyz> (raw)
In-Reply-To: <164674011536.19727.522116367172581993@lain.red.khirnov.net>

On Tue, 08 Mar 2022 12:48:35 +0100 Anton Khirnov <anton@khirnov.net> wrote:
> Quoting Niklas Haas (2022-03-04 16:03:06)
> > From: Niklas Haas <git@haasn.dev>
> > 
> > I arbitrarily decided to use the syntax 'opt=@filename' to match e.g.
> > `curl -Ffield=@filename`, and also because @ is not a valid hex
> > character, nor does it conflict with any other common shell or ffmpeg
> > syntax.
> > 
> > This is arguably somewhat clunky because it does not round-trip with
> > av_opt_get - you will get back a hex interpretation of the loaded file,
> > rather than the filename it was loaded from. It also implies a (perhaps
> > unnecessary) memcpy from mapped file memory into a allocated memory.
> > This is unfortunately necessary because there's no way for us to know
> > whether av_free or av_file_unmap is needed to clean up previous option
> > values.
> > 
> > The motivating use case was the introduction of several new binary
> > options for vf_libplacebo, but other filters that currently rely on
> > manual file-path loading could benefit from it as well.
> 
> Sorry, I think having an arbitrary file loader in the options parser
> will be an endless security nightmare.
> 
> The alternative I had in mind was having ffmpeg.c itself do the file
> loading. This will be require some modification of the options parsing
> code in cmdutils.c and also extending the mechanisms we use to pass
> options to filters.
> 
> I can try to make a POC in a few days.

I think that is the better approach, too. I went for this approach
because it was the easiest to implement, not because it makes the most
sense.

I do think that it's fine for ffmpeg.c to do this, but springing it upon
unsuspecting API users *in general* is, well, in retrospect, not
something I want to have my name attached to.

I think that I would drop the second half of this commit (keeping only
the introduction of `av_set_string_from_filepath`), and then make
ffmpeg.c use it for cmdlist arguments starting with '@' as a separate
commit.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".