[FFmpeg-devel] 答复: [PATCH] fftools/ffmpeg_ffplay_ffprobe_cmdutils: add -mask_url to replace the protocol address in the command with the asterisk (*)

["Wujian(Chin)" <wujian2@huawei.com>]

>> @@ -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-bounces@ffmpeg.org] 代表 Nicolas George
发送时间: 2022年12月19日 21:30
收件人: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
抄送: wangqinghua (I) <wangqinghua9@huawei.com>
主题: 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 <wujian2@huawei.com>
> ---
>  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(&swr_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(), &win32_argc);
>      if (win32_argc <= 0 || !argv_w)
> -        return;
> +        goto end;
>  
>      /* determine the UTF-8 buffer size (including NULL-termination symbols) */
>      for (i = 0; i < win32_argc; i++)
> @@ -232,7 +266,7 @@ static void prepare_app_arguments(int *argc_ptr, char ***argv_ptr)
>      argstr_flat     = (char *)win32_argv_utf8 + sizeof(char *) * (win32_argc + 1);
>      if (!win32_argv_utf8) {
>          LocalFree(argv_w);
> -        return;
> +        goto end;
>      }
>  
>      for (i = 0; i < win32_argc; i++) { @@ -243,9 +277,14 @@ static 
> void prepare_app_arguments(int *argc_ptr, char ***argv_ptr)
>      }
>      win32_argv_utf8[i] = NULL;
>      LocalFree(argv_w);
> -
>      *argc_ptr = win32_argc;
>      *argv_ptr = win32_argv_utf8;
> +end:
> +    if (*argc_ptr > 1 && !strcmp((*argv_ptr)[1], "-mask_url")) {
> +        (*argv_ptr)[1] = (*argv_ptr)[0];
> +        (*argc_ptr)--;
> +        (*argv_ptr)++;
> +    }
>  }
>  #else
>  static inline void prepare_app_arguments(int *argc_ptr, char 
> ***argv_ptr) diff --git a/fftools/cmdutils.h b/fftools/cmdutils.h 
> index 4496221..ce4c1db 100644
> --- a/fftools/cmdutils.h
> +++ b/fftools/cmdutils.h
> @@ -50,6 +50,21 @@ extern AVDictionary *format_opts, *codec_opts;  
> extern int hide_banner;
>  
>  /**
> + * Using to masking sensitive info.
> + */
> +void param_masking(int argc, char **argv);
> +
> +/**
> + * Using to copy ori argv.
> + */
> +char **copy_argv(int argc, char **argv);
> +
> +/**
> + * Free **
> + */
> +void free_pp(int argc, char **argv);
> +
> +/**
>   * Register a program-specific cleanup routine.
>   */
>  void register_exit(void (*cb)(int ret)); diff --git 
> a/fftools/ffmpeg.c b/fftools/ffmpeg.c index 881d6f0..fccbde9 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -3865,9 +3865,9 @@ static int64_t getmaxrss(void)
>  
>  int main(int argc, char **argv)
>  {
> -    int ret;

> +    int ret, maskFlag;

We do not do camelCase.

>      BenchmarkTimeStamps ti;
> -
> +    char **argv2;
>      init_dynload();
>  
>      register_exit(ffmpeg_cleanup);
> @@ -3877,15 +3877,25 @@ int main(int argc, char **argv)
>      av_log_set_flags(AV_LOG_SKIP_REPEATED);
>      parse_loglevel(argc, argv, options);
>  

> +    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.

>  #if CONFIG_AVDEVICE
>      avdevice_register_all();
>  #endif
>      avformat_network_init();
>  
>      show_banner(argc, argv, options);

> +    argv2 = copy_argv(argc, argv);
> +    if (maskFlag)
> +        param_masking(argc, argv);

This is duplicated in all three files and unnecessary: have a single function do the copy and the masking.

>  
>      /* parse options and open all input/output files */
> -    ret = ffmpeg_parse_options(argc, argv);
> +    ret = ffmpeg_parse_options(argc, argv2);
>      if (ret < 0)
>          exit_program(1);
>  
> diff --git a/fftools/ffplay.c b/fftools/ffplay.c index 
> fc7e1c2..5d282f1 100644
> --- a/fftools/ffplay.c
> +++ b/fftools/ffplay.c
> @@ -3663,10 +3663,18 @@ void show_help_default(const char *opt, const 
> char *arg)
>  /* Called from the main */
>  int main(int argc, char **argv)
>  {
> -    int flags;
> +    int flags, maskFlag;
> +    char **argv2;
>      VideoState *is;
>  
>      init_dynload();
> +    maskFlag = 0;
> +    if (argc > 1 && !strcmp(argv[1], "-mask_url")) {
> +        argv[1] = argv[0];
> +        maskFlag = 1;
> +        argc--;
> +        argv++;
> +    }
>  
>      av_log_set_flags(AV_LOG_SKIP_REPEATED);
>      parse_loglevel(argc, argv, options); @@ -3682,7 +3690,10 @@ int 
> main(int argc, char **argv)
>  
>      show_banner(argc, argv, options);
>  
> -    parse_options(NULL, argc, argv, options, opt_input_file);
> +    argv2 = copy_argv(argc, argv);
> +    parse_options(NULL, argc, argv2, options, opt_input_file);
> +    if (maskFlag)
> +        param_masking(argc, argv);
>  
>      if (!input_filename) {
>          show_usage();
> diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c index 
> d2f126d..e69f49f 100644
> --- a/fftools/ffprobe.c
> +++ b/fftools/ffprobe.c
> @@ -4035,9 +4035,16 @@ int main(int argc, char **argv)
>      WriterContext *wctx;
>      char *buf;
>      char *w_name = NULL, *w_args = NULL;
> -    int ret, input_ret, i;
> -
> +    int ret, input_ret, i, maskFlag;
> +    char **argv2;
>      init_dynload();
> +    maskFlag = 0;
> +    if (argc > 1 && !strcmp(argv[1], "-mask_url")) {
> +        argv[1] = argv[0];
> +        maskFlag = 1;
> +        argc--;
> +        argv++;
> +    }
>  
>  #if HAVE_THREADS
>      ret = pthread_mutex_init(&log_mutex, NULL); @@ -4056,8 +4063,10 
> @@ int main(int argc, char **argv)  #endif
>  
>      show_banner(argc, argv, options);
> -    parse_options(NULL, argc, argv, options, opt_input_file);
> -
> +    argv2 = copy_argv(argc, argv);
> +    parse_options(NULL, argc, argv2, options, opt_input_file);
> +    if (maskFlag)
> +        param_masking(argc, argv);
>      if (do_show_log)
>          av_log_set_callback(log_callback);
>  
> @@ -4173,6 +4182,7 @@ end:
>      av_freep(&print_format);
>      av_freep(&read_intervals);
>      av_hash_freep(&hash);
> +    free_pp(argc, argv2);
>  
>      uninit_opts();
>      for (i = 0; i < FF_ARRAY_ELEMS(sections); i++)

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-request@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-request@ffmpeg.org with subject "unsubscribe".