[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(),