| Message ID | 20161205125251.28683-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | a5f27a9c3aa973c543bd8bbf2a78363700bbc03e |
| Headers | show |
On Mon, Dec 05, 2016 at 01:52:50PM +0100, Michael Niedermayer wrote: > This should make it less ambigous that these are URLs > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > doc/ffmpeg.texi | 18 +++++++++--------- > doc/ffplay.texi | 6 +++--- > doc/ffprobe.texi | 10 +++++----- > ffmpeg_opt.c | 4 ++-- > 4 files changed, 19 insertions(+), 19 deletions(-) applied [...]
On Mon, 5 Dec 2016 13:52:50 +0100, Michael Niedermayer wrote: > This should make it less ambigous that these are URLs > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > doc/ffmpeg.texi | 18 +++++++++--------- > doc/ffplay.texi | 6 +++--- > doc/ffprobe.texi | 10 +++++----- > ffmpeg_opt.c | 4 ++-- > 4 files changed, 19 insertions(+), 19 deletions(-) Although this is a trivial patch, approximately 7 hours between sending a patch and applying without feedback isn't enough time. At least 24 hours would be preferrable. This patch seems unnecessary and I prefer the previous syntax. "URL" has a strong networking connotation. This change may confuse some users: a few may ask if the tools work with "files" in addition to URLs. If we want to go full neckbeard then URI may be more generic, but no normal human would know what that means. Lastly, and unimportantly, "url" should have been capitalized in the descriptive texts.
> Although this is a trivial patch, approximately 7 hours between sending > a patch and applying without feedback isn't enough time. At least 24 > hours would be preferrable. > > This patch seems unnecessary and I prefer the previous syntax. "URL" > has a strong networking connotation. This change may confuse some > users: a few may ask if the tools work with "files" in addition to URLs. > > If we want to go full neckbeard then URI may be more generic, but no > normal human would know what that means. > > Lastly, and unimportantly, "url" should have been capitalized in > the descriptive texts. You forget another very important point: What FFmpeg accepts are not URLs, especially with the file protocol, but URL-like sblurbs that do not respect any standard. Regards,
On Thu, Dec 08, 2016 at 11:13:16AM -0900, Lou Logan wrote: > On Mon, 5 Dec 2016 13:52:50 +0100, Michael Niedermayer wrote: > > > This should make it less ambigous that these are URLs > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > doc/ffmpeg.texi | 18 +++++++++--------- > > doc/ffplay.texi | 6 +++--- > > doc/ffprobe.texi | 10 +++++----- > > ffmpeg_opt.c | 4 ++-- > > 4 files changed, 19 insertions(+), 19 deletions(-) > > Although this is a trivial patch, approximately 7 hours between sending > a patch and applying without feedback isn't enough time. At least 24 > hours would be preferrable. there were open and fully public security bugs, the use of untrusted filenames which look like urls aka files as in "http://someserver.com" would allow potential remote code execution i applied this quickly as it seemed important to me to clarify that the command line arguments are not just normal file names in addition to fixing the bug which depended on such files can you help me clarify and improve this further ? I suspect you can reword this quicker yourself than with me messing around further The really important point is that one cannot saftely put a random untrusted string or filename in place of these arguments. untrusted filenames needs "file:" prefix at least Thanks and sorry for havning applied this so quickly [...]
On Mon, 5 Dec 2016 21:10:00 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Mon, Dec 05, 2016 at 01:52:50PM +0100, Michael Niedermayer wrote: > > This should make it less ambigous that these are URLs > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > doc/ffmpeg.texi | 18 +++++++++--------- > > doc/ffplay.texi | 6 +++--- > > doc/ffprobe.texi | 10 +++++----- > > ffmpeg_opt.c | 4 ++-- > > 4 files changed, 19 insertions(+), 19 deletions(-) > > applied > > > [...] I guess my remarks mean nothing.
On Fri, 9 Dec 2016 03:48:39 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Thu, Dec 08, 2016 at 11:13:16AM -0900, Lou Logan wrote: > > On Mon, 5 Dec 2016 13:52:50 +0100, Michael Niedermayer wrote: > > > > > This should make it less ambigous that these are URLs > > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > doc/ffmpeg.texi | 18 +++++++++--------- > > > doc/ffplay.texi | 6 +++--- > > > doc/ffprobe.texi | 10 +++++----- > > > ffmpeg_opt.c | 4 ++-- > > > 4 files changed, 19 insertions(+), 19 deletions(-) > > > > Although this is a trivial patch, approximately 7 hours between sending > > a patch and applying without feedback isn't enough time. At least 24 > > hours would be preferrable. > > there were open and fully public security bugs, the use of untrusted > filenames which look like urls aka files as in > "http://someserver.com" > would allow potential remote code execution I guess "security bugs" now justify any kind of change? It's clear that a user has to prefix filenames with file: or so, since it will interpret various strings as not-files (like as an option or an URL). Thus it's not a security bug, just an user error. > i applied this quickly as it seemed important to me to clarify that > the command line arguments are not just normal file names > in addition to fixing the bug which depended on such files > > can you help me clarify and improve this further ? > I suspect you can reword this quicker yourself than with me messing > around further > > The really important point is that one cannot saftely put a random > untrusted string or filename in place of these arguments. > untrusted filenames needs "file:" prefix at least > > Thanks and sorry for havning applied this so quickly > > [...] >
On Fri, Dec 09, 2016 at 09:55:52AM +0100, wm4 wrote: > On Fri, 9 Dec 2016 03:48:39 +0100 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > On Thu, Dec 08, 2016 at 11:13:16AM -0900, Lou Logan wrote: > > > On Mon, 5 Dec 2016 13:52:50 +0100, Michael Niedermayer wrote: > > > > > > > This should make it less ambigous that these are URLs > > > > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > doc/ffmpeg.texi | 18 +++++++++--------- > > > > doc/ffplay.texi | 6 +++--- > > > > doc/ffprobe.texi | 10 +++++----- > > > > ffmpeg_opt.c | 4 ++-- > > > > 4 files changed, 19 insertions(+), 19 deletions(-) > > > > > > Although this is a trivial patch, approximately 7 hours between sending > > > a patch and applying without feedback isn't enough time. At least 24 > > > hours would be preferrable. > > > > there were open and fully public security bugs, the use of untrusted > > filenames which look like urls aka files as in > > "http://someserver.com" > > would allow potential remote code execution > > I guess "security bugs" now justify any kind of change? what exactly is this comment supposed to mean ? > > It's clear that a user has to prefix filenames with file: or so, since > it will interpret various strings as not-files (like as an option or an > URL). Thus it's not a security bug, just an user error. There are really just 2 options, either its safe to use arbitrary filenames in the arguments, or it has to be documented that these are not arbitrary filenames, aka its not safe to put arbitrary things in there. And it certainly was not clear enough as tickets are being opened on the assumtation that this was safe and that by security researchers [...]
On Fri, Dec 09, 2016 at 09:53:00AM +0100, wm4 wrote: > On Mon, 5 Dec 2016 21:10:00 +0100 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > On Mon, Dec 05, 2016 at 01:52:50PM +0100, Michael Niedermayer wrote: > > > This should make it less ambigous that these are URLs > > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > doc/ffmpeg.texi | 18 +++++++++--------- > > > doc/ffplay.texi | 6 +++--- > > > doc/ffprobe.texi | 10 +++++----- > > > ffmpeg_opt.c | 4 ++-- > > > 4 files changed, 19 insertions(+), 19 deletions(-) > > > > applied > > > > > > [...] > > I guess my remarks mean nothing. You had no remarks about this patch. You did comment on a different patch in this patchset, which has not been applied. And i didnt reply to your comment there yet due to lack of time and i hoped others would comment too. [...]
On Fri, 9 Dec 2016 14:45:24 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Fri, Dec 09, 2016 at 09:53:00AM +0100, wm4 wrote: > > On Mon, 5 Dec 2016 21:10:00 +0100 > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > On Mon, Dec 05, 2016 at 01:52:50PM +0100, Michael Niedermayer wrote: > > > > This should make it less ambigous that these are URLs > > > > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > doc/ffmpeg.texi | 18 +++++++++--------- > > > > doc/ffplay.texi | 6 +++--- > > > > doc/ffprobe.texi | 10 +++++----- > > > > ffmpeg_opt.c | 4 ++-- > > > > 4 files changed, 19 insertions(+), 19 deletions(-) > > > > > > applied > > > > > > > > > [...] > > > > I guess my remarks mean nothing. > > You had no remarks about this patch. You did comment on a different > patch in this patchset, which has not been applied. > > And i didnt reply to your comment there yet due to lack of time and > i hoped others would comment too. > > > [...] My error. Apologies!
On Fri, 9 Dec 2016 14:33:08 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Fri, Dec 09, 2016 at 09:55:52AM +0100, wm4 wrote: > > On Fri, 9 Dec 2016 03:48:39 +0100 > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > On Thu, Dec 08, 2016 at 11:13:16AM -0900, Lou Logan wrote: > > > > On Mon, 5 Dec 2016 13:52:50 +0100, Michael Niedermayer wrote: > > > > > > > > > This should make it less ambigous that these are URLs > > > > > > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > --- > > > > > doc/ffmpeg.texi | 18 +++++++++--------- > > > > > doc/ffplay.texi | 6 +++--- > > > > > doc/ffprobe.texi | 10 +++++----- > > > > > ffmpeg_opt.c | 4 ++-- > > > > > 4 files changed, 19 insertions(+), 19 deletions(-) > > > > > > > > Although this is a trivial patch, approximately 7 hours between sending > > > > a patch and applying without feedback isn't enough time. At least 24 > > > > hours would be preferrable. > > > > > > there were open and fully public security bugs, the use of untrusted > > > filenames which look like urls aka files as in > > > "http://someserver.com" > > > would allow potential remote code execution > > > > > I guess "security bugs" now justify any kind of change? > > what exactly is this comment supposed to mean ? I'm just saying that security fixes doesn't mean that quality control should be lacking. Maybe rather the opposite. > > > > > It's clear that a user has to prefix filenames with file: or so, since > > it will interpret various strings as not-files (like as an option or an > > URL). Thus it's not a security bug, just an user error. > > There are really just 2 options, either its safe to use arbitrary > filenames in the arguments, or it has to be documented that these are > not arbitrary filenames, aka its not safe to put arbitrary things in > there. Yes. If you allow "plain" local fileanes, it's inherently ambiguous. Maybe we could go as far as disallowing such filenames by default, and requiring that the filename starts with "/", "./" or "file:". I also claimed that a filename can be misinterpreted as option - but that's probably not the case: filenames for input always are passed to "-i" or similar options. Only output filenames are implicit. Anyway, I might have assumed that this discussion is about patch 2/2, not 1/2. > And it certainly was not clear enough as tickets are being opened on > the assumtation that this was safe and that by security researchers > > > > [...]
diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi index 4b159fd659..b56bdbe261 100644 --- a/doc/ffmpeg.texi +++ b/doc/ffmpeg.texi @@ -12,7 +12,7 @@ @chapter Synopsis -ffmpeg [@var{global_options}] @{[@var{input_file_options}] -i @file{input_file}@} ... @{[@var{output_file_options}] @file{output_file}@} ... +ffmpeg [@var{global_options}] @{[@var{input_file_options}] -i @file{input_url}@} ... @{[@var{output_file_options}] @file{output_url}@} ... @chapter Description @c man begin DESCRIPTION @@ -24,10 +24,10 @@ rates and resize video on the fly with a high quality polyphase filter. @command{ffmpeg} reads from an arbitrary number of input "files" (which can be regular files, pipes, network streams, grabbing devices, etc.), specified by the @code{-i} option, and writes to an arbitrary number of output "files", which are -specified by a plain output filename. Anything found on the command line which -cannot be interpreted as an option is considered to be an output filename. +specified by a plain output url. Anything found on the command line which +cannot be interpreted as an option is considered to be an output url. -Each input or output file can, in principle, contain any number of streams of +Each input or output url can, in principle, contain any number of streams of different types (video/audio/subtitle/attachment/data). The allowed number and/or types of streams may be limited by the container format. Selecting which streams from which inputs will go into which output is either done automatically @@ -243,8 +243,8 @@ Force input or output file format. The format is normally auto detected for inpu files and guessed from the file extension for output files, so this option is not needed in most cases. -@item -i @var{filename} (@emph{input}) -input file name +@item -i @var{url} (@emph{input}) +input file url @item -y (@emph{global}) Overwrite output files without asking. @@ -281,7 +281,7 @@ libx264, and the 138th audio, which will be encoded with libvorbis. When used as an input option (before @code{-i}), limit the @var{duration} of data read from the input file. -When used as an output option (before an output filename), stop writing the +When used as an output option (before an output url), stop writing the output after its duration reaches @var{duration}. @var{duration} must be a time duration specification, @@ -310,7 +310,7 @@ extra segment between the seek point and @var{position} will be decoded and discarded. When doing stream copy or when @option{-noaccurate_seek} is used, it will be preserved. -When used as an output option (before an output filename), decodes but discards +When used as an output option (before an output url), decodes but discards input until the timestamps reach @var{position}. @var{position} must be a time duration specification, @@ -1168,7 +1168,7 @@ may be reassigned to a different value. For example, to set the stream 0 PID to 33 and the stream 1 PID to 36 for an output mpegts file: @example -ffmpeg -i infile -streamid 0:33 -streamid 1:36 out.ts +ffmpeg -i inurl -streamid 0:33 -streamid 1:36 out.ts @end example @item -bsf[:@var{stream_specifier}] @var{bitstream_filters} (@emph{output,per-stream}) diff --git a/doc/ffplay.texi b/doc/ffplay.texi index 4bc3ced39a..073b457256 100644 --- a/doc/ffplay.texi +++ b/doc/ffplay.texi @@ -12,7 +12,7 @@ @chapter Synopsis -ffplay [@var{options}] [@file{input_file}] +ffplay [@var{options}] [@file{input_url}] @chapter Description @c man begin DESCRIPTION @@ -106,8 +106,8 @@ the input audio. Use the option "-filters" to show all the available filters (including sources and sinks). -@item -i @var{input_file} -Read @var{input_file}. +@item -i @var{input_url} +Read @var{input_url}. @end table @section Advanced options diff --git a/doc/ffprobe.texi b/doc/ffprobe.texi index 1069ae32de..7514b2df73 100644 --- a/doc/ffprobe.texi +++ b/doc/ffprobe.texi @@ -12,7 +12,7 @@ @chapter Synopsis -ffprobe [@var{options}] [@file{input_file}] +ffprobe [@var{options}] [@file{input_url}] @chapter Description @c man begin DESCRIPTION @@ -24,8 +24,8 @@ For example it can be used to check the format of the container used by a multimedia stream and the format and type of each media stream contained in it. -If a filename is specified in input, ffprobe will try to open and -probe the file content. If the file cannot be opened or recognized as +If a url is specified in input, ffprobe will try to open and +probe the url content. If the url cannot be opened or recognized as a multimedia file, a positive exit code is returned. ffprobe may be employed both as a standalone application or in @@ -332,8 +332,8 @@ with name "PIXEL_FORMAT". Force bitexact output, useful to produce output which is not dependent on the specific build. -@item -i @var{input_file} -Read @var{input_file}. +@item -i @var{input_url} +Read @var{input_url}. @end table @c man end diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c index e470dbeb46..6862456c27 100644 --- a/ffmpeg_opt.c +++ b/ffmpeg_opt.c @@ -3195,8 +3195,8 @@ enum OptGroup { }; static const OptionGroupDef groups[] = { - [GROUP_OUTFILE] = { "output file", NULL, OPT_OUTPUT }, - [GROUP_INFILE] = { "input file", "i", OPT_INPUT }, + [GROUP_OUTFILE] = { "output url", NULL, OPT_OUTPUT }, + [GROUP_INFILE] = { "input url", "i", OPT_INPUT }, }; static int open_files(OptionGroupList *l, const char *inout,
This should make it less ambigous that these are URLs Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- doc/ffmpeg.texi | 18 +++++++++--------- doc/ffplay.texi | 6 +++--- doc/ffprobe.texi | 10 +++++----- ffmpeg_opt.c | 4 ++-- 4 files changed, 19 insertions(+), 19 deletions(-)