[FFmpeg-devel] 答复: [PATCH] fftools/ffmpeg_ffplay_ffprobe_cmdutils: add -mask_url to replace the protocol address in the command with the asterisk (*)
>On Mon, Dec 26, 2022 at 01:07:51PM +, Wujian(Chin) wrote: >> The issue has been modified. Please review again, thank you! >> >> Signed-off-by: wujian_nanjing >> --- >> doc/fftools-common-opts.texi | 11 +++ >> fftools/cmdutils.c | 77 >> ++-- >> fftools/cmdutils.h | 25 ++ >> fftools/ffmpeg.c | 10 +++--- >> fftools/ffplay.c | 9 -- >> fftools/ffprobe.c| 10 +++--- >> 6 files changed, 128 insertions(+), 14 deletions(-) >ffmpeg -h >segfaults with this patch >==32366== Invalid read of size 8 I have reproduced the problem. The reason is that the last element of argv_copy does not end with null. Otherwise, this problem will occur. Thanks. >==32366==at 0x30836B: split_commandline (in ffmpeg/ffmpeg_g) >==32366==by 0x3039CD: ffmpeg_parse_options (in ffmpeg/ffmpeg_g) >==32366==by 0x2ED201: main (in ffmpeg/ffmpeg_g) >==32366== Address 0x2ced5290 is 0 bytes after a block of size 16 alloc'd >==32366==at 0x4C33E76: memalign (in >/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) >==32366==by 0x4C33F91: posix_memalign (in >/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) >==32366==by 0x116B322: av_malloc (in ffmpeg/ffmpeg_g) >==32366==by 0x116B4D8: av_mallocz (in ffmpeg/ffmpeg_g) >==32366==by 0x306469: copy_argv (in ffmpeg/ffmpeg_g) >==32366==by 0x306537: handle_arg_param (in ffmpeg/ffmpeg_g) >==32366==by 0x2ED1F5: main (in ffmpeg/ffmpeg_g) >==32366== > [...] >-- >Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB >Opposition brings concord. Out of discord comes the fairest harmony. >-- Heraclitus ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] 答复: [PATCH] fftools/ffmpeg_ffplay_ffprobe_cmdutils: add -mask_url to replace the protocol address in the command with the asterisk (*)
>ffmpeg -h
>segfaults with this patch
My environment test is OK. Details are as follows:
SZV1000266228:/usr1/wujian/build # ffmpeg -h
ffmpeg version N-109445-gcc46f5b Copyright (c) 2000-2022 the FFmpeg developers
built with gcc 7 (GCC)
configuration: --samples=/usr1/wujian/ffmpeg_master/fate-suite
libavutil 57. 43.100 / 57. 43.100
libavcodec 59. 55.103 / 59. 55.103
libavformat59. 34.102 / 59. 34.102
libavdevice59. 8.101 / 59. 8.101
libavfilter 8. 53.100 / 8. 53.100
libswscale 6. 8.112 / 6. 8.112
libswresample 4. 9.100 / 4. 9.100
Hyper fast Audio and Video encoder
usage: ffmpeg [options] [[infile options] -i infile]... {[outfile options]
outfile}...
Getting help:
-h -- print basic options
-h long -- print more options
-h full -- print all options (including all format and codec specific
options, very long)
-h type=name -- print all options for the named
decoder/encoder/demuxer/muxer/filter/bsf/protocol
See man ffmpeg for detailed description of the options.
Print help / information / capabilities:
-L show license
-h topicshow help
-? topicshow help
-help topic show help
--help topicshow help
-versionshow version
-buildconf show build configuration
-formatsshow available formats
-muxers show available muxers
-demuxers show available demuxers
-devicesshow available devices
-codecs show available codecs
-decoders show available decoders
-encoders show available encoders
-bsfs show available bit stream filters
-protocols show available protocols
-filtersshow available filters
-pix_fmts show available pixel formats
-layoutsshow standard channel layouts
-sample_fmtsshow available audio sample formats
-dispositions show available stream dispositions
-colors show available color names
-sources device list sources of the input device
-sinks device list sinks of the output device
-hwaccels show available HW acceleration methods
Global options (affect whole program instead of just one file):
-loglevel loglevel set logging level
-v loglevel set logging level
-report generate a report
-max_alloc bytesset maximum size of a single allocated block
-y overwrite output files
-n never overwrite output files
-ignore_unknown Ignore unknown stream types
-filter_threads number of non-complex filter threads
-filter_complex_threads number of threads for -filter_complex
-stats print progress report during encoding
-max_error_rate maximum error rate ratio of decoding errors (0.0: no errors,
1.0: 100% errors) above which ffmpeg returns an error instead of success.
Per-file main options:
-f fmt force format
-c codeccodec name
-codec codeccodec name
-pre preset preset name
-map_metadata outfile[,metadata]:infile[,metadata] set metadata information of
outfile from infile
-t duration record or transcode "duration" seconds of audio/video
-to time_stop record or transcode stop time
-fs limit_size set the limit file size in bytes
-ss time_offset the start time offset
-sseof time_off set the start time offset relative to EOF
-seek_timestamp enable/disable seeking by timestamp with -ss
-timestamp time set the recording timestamp ('now' to set the current time)
-metadata string=string add metadata
-program title=string:st=number... add program with specified streams
-target typespecify target file type ("vcd", "svcd", "dvd", "dv" or
"dv50" with optional prefixes "pal-", "ntsc-" or "film-")
-apad audio pad
-frames number set the number of frames to output
-filter filter_graph set stream filtergraph
-filter_script filename read stream filtergraph description from a file
-reinit_filter reinit filtergraph on input parameter changes
-discarddiscard
-dispositiondisposition
Video options:
-vframes number set the number of video frames to output
-r rate set frame rate (Hz value, fraction or abbreviation)
-fpsmax rateset max frame rate (Hz value, fraction or abbreviation)
-s size set frame size (WxH or abbreviation)
-aspect aspect set aspect ratio (4:3, 16:9 or 1., 1.)
-display_rotation angle set pure counter-clockwise rotation in degrees for
stream(s)
-display_hflip set display horizontal flip for stream(s) (overrides any
display rotation if it is not set)
-display_vflip set display vertical flip for stream(s) (overrides any
display rotation if it is not set)
-vn disable video
-vcodec codec force video codec ('copy' to copy stream)
-timecode hh:mm:ss[:;.]ff set initial TimeCode value.
-pass n
Re: [FFmpeg-devel] 答复: [PATCH] fftools/ffmpeg_ffplay_ffprobe_cmdutils: add -mask_url to replace the protocol address in the command with the asterisk (*)
Wujian(Chin) (12022-12-20): > I think that it's more concise to use code this way. Concision is not the goal here, maintainability is. Please do not use gotos. > I think that it would be better to replace the entire url, so that the code > implementation is simple. Then replace the whole command line, it is even simpler. Also, this way you miss credentials passed through options. > >> +argv2 = av_mallocz(argc * sizeof(char *)); > > >sizeof(*argv2) Youhoud? > This option needs to replace the URL. It is more appropriate to judge > mask_url and copy argv in this place. Otherwise, do you have any > other suggestions? Use the normal options parsing system. Regards, -- Nicolas George signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] 答复: [PATCH] fftools/ffmpeg_ffplay_ffprobe_cmdutils: add -mask_url to replace the protocol address in the command with the asterisk (*)
>Marvin Scholz (12022-12-19):
> I agree, but then the docs should probably mention that to not give a
>> false sense of absolute security here. And maybe note that it might
>Indeed, documentation is necessary.
Is it appropriate to describe this document? Please give some suggestions.
Thank you , everyone.
fftools-common-opts.texi
@item -mask_url -i @var{url} (@emph{output})
If the protocol address contains the user name and password, the ps -ef
command exposes plaintext. The -mask_url parameter option is added to
replace the protocol address in the command line with the asterisk (*).
Because other users can run the ps -ef command to view sensitive
information such as the user name and password in the protocol address,
which is insecure.
@example
ffmpeg -mask_url -i rtsp://username:password-ip:port/stream/test
@end example
>> be a better option to pass the password via stdin or hide the process
>> from other users to completely avoid leaking the password.
>Unfortunately, as far as I know, we do not have any mechanism of the kind.
>That would be useful.
>Regards,
--
> Nicolas George
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with
subject "unsubscribe".
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] 答复: [PATCH] fftools/ffmpeg_ffplay_ffprobe_cmdutils: add -mask_url to replace the protocol address in the command with the asterisk (*)
>> @@ -215,13 +249,13 @@ static void prepare_app_arguments(int *argc_ptr, char
>> ***argv_ptr)
>> if (win32_argv_utf8) {
>> *argc_ptr = win32_argc;
>> *argv_ptr = win32_argv_utf8;
>> -return;
>> +goto end;
> We only use goto for error processing.
I think that it's more concise to use code this way.
>> +int i, j;
>> +for (i = 1; i < argc; i++) {
>> +char *match = strstr(argv[i], "://");
>> +if (match) {
>> +int total = strlen(argv[i]);
>> +for (j = 0; j < total; j++) {
>> +argv[i][j] = '*';
>> +}
>Masking the whole URL seems too much. Logins and passwords are introduced by
>the @ character.
I think that it would be better to replace the entire url, so that the code
implementation is simple.
>> +char **argv2;
>> +argv2 = av_mallocz(argc * sizeof(char *));
>sizeof(*argv2)
>
>> +maskFlag = 0;
>> +if (argc > 1 && !strcmp(argv[1], "-mask_url")) {
>> +argv[1] = argv[0];
>> +maskFlag = 1;
>> +argc--;
>> +argv++;
>> +}
>This option is not special nor important enough to warrant a special treatment
>like that.
This option needs to replace the URL. It is more appropriate to judge mask_url
and copy argv in this place. Otherwise, do you have any other suggestions?
Thank you for your issue. Nicolas George
-邮件原件-
发件人: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] 代表 Nicolas George
发送时间: 2022年12月19日 21:30
收件人: FFmpeg development discussions and patches
抄送: wangqinghua (I)
主题: Re: [FFmpeg-devel] [PATCH] fftools/ffmpeg_ffplay_ffprobe_cmdutils: add
-mask_url to replace the protocol address in the command with the asterisk (*)
Wujian(Chin) (12022-12-19):
> I have modified the issues. Please review it again. Thank you.
>
> If the protocol address contains the user name and password, The ps -ef
> command exposes plaintext.
Spurious comma or capital.
> The -mask_url parameter option is added to replace the protocol address in
> the command line with the asterisk (*).
> Because other users can run the ps -ef command to view sensitive
> information such as the user name and password in the protocol address, which
> is insecure.
Please wrap to 60-72 characters.
>
> Signed-off-by: wujian_nanjing
> ---
> doc/ffmpeg.texi| 9 +
> doc/ffplay.texi| 8
> doc/ffprobe.texi | 9 +
> fftools/cmdutils.c | 47
> +++
> fftools/cmdutils.h | 15 +++
> fftools/ffmpeg.c | 16 +---
> fftools/ffplay.c | 15 +--
> fftools/ffprobe.c | 18 ++
> 8 files changed, 124 insertions(+), 13 deletions(-)
>
> diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi index 0367930..1f6cb33
> 100644
> --- a/doc/ffmpeg.texi
> +++ b/doc/ffmpeg.texi
> --- a/doc/ffplay.texi
> +++ b/doc/ffplay.texi
> --- a/doc/ffprobe.texi
> +++ b/doc/ffprobe.texi
The place for common options is doc/fftools-common-opts.texi.
> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c index
> a1de621..c35d7e1 100644
> --- a/fftools/cmdutils.c
> +++ b/fftools/cmdutils.c
> @@ -61,6 +61,40 @@ AVDictionary *format_opts, *codec_opts;
>
> int hide_banner = 0;
>
> +void param_masking(int argc, char **argv) {
Functions name in ...ing do not seem idiomatic to me.
The style for the brace is off.
> +int i, j;
> +for (i = 1; i < argc; i++) {
> +char *match = strstr(argv[i], "://");
> +if (match) {
> +int total = strlen(argv[i]);
> +for (j = 0; j < total; j++) {
> +argv[i][j] = '*';
> +}
Masking the whole URL seems too much. Logins and passwords are introduced by
the @ character.
> +}
> +}
> +}
> +
> +char **copy_argv(int argc, char **argv) {
The brace is off here too.
> +char **argv2;
> +argv2 = av_mallocz(argc * sizeof(char *));
sizeof(*argv2)
> +if (!argv2)
> +exit_program(1);
Error message.
> +
> +for (int i = 0; i < argc; i++) {
> +int length = strlen(argv[i]) + 1;
> +argv2[i] = av_mallocz(length * sizeof(char *));
> +if (!argv2[i])
> +exit_program(1);
> +memcpy(argv2[i], argv[i], length - 1);
> +}
> +return argv2;
> +}
> +
> +void free_pp(int argc, char **argv) {
The brace is off too. This function is called only from ffprobe, looks wrong.
> +for (int i = 0; i < argc; i++)
> +av_free(argv[i]);
> +av_free(argv);
> +}
> void uninit_opts(void)
> {
> av_dict_free(_opts);
> @@ -215,13 +249,13 @@ static void prepare_app_arguments(int *argc_ptr, char
> ***argv_ptr)
> if (win32_argv_utf8) {
> *argc_ptr = win32_argc;
> *argv_ptr = win32_argv_utf8;
> -return;
> +goto end;
We only use goto for error processing.
> }
>
> win32_argc = 0;
> argv_w = CommandLineToArgvW(GetCommandLineW(),
