[FFmpeg-devel] [PATCH] ffprobe: implement string validation policy setting

[Stefano Sabatini]

On date Wednesday 2013-10-02 19:07:54 +0200, Clément Bœsch encoded:
> On Wed, Oct 02, 2013 at 05:52:05PM +0200, Stefano Sabatini wrote:
> > This should fix trac tickets #1163, #2502, #2955.
> > ---
> >  doc/ffprobe.texi |  24 ++++++++++
> >  ffprobe.c        | 141 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
> >  2 files changed, 158 insertions(+), 7 deletions(-)
> > 
> > diff --git a/doc/ffprobe.texi b/doc/ffprobe.texi
> > index 777dbe7..55c6e80 100644
> > --- a/doc/ffprobe.texi
> > +++ b/doc/ffprobe.texi
> > @@ -317,6 +317,30 @@ Show information related to program and library versions. This is the
> >  equivalent of setting both @option{-show_program_version} and
> >  @option{-show_library_versions} options.
> >  
> > + at item -string_validation_policy @var{policy}
> > +Set string validation policy. It accepts the following values.
> > +
> > + at table @samp
> > + at item fail
> > +The program will fail immediately in case an invalid string (UTF-8)
> > +sequence is found in the input. This is especially useful to validate
> > +input metadata.
> > +
> > + at item replace=REPLACEMENT
> 
> replace[=@var{REPLACEMENT}]

Will make the poor parser explode, changed in a differet fashion.

[...]
> > +static int opt_string_validation_policy(void *optctx, const char *opt, const char *arg)
> > +{
> > +    char *mode = av_strdup(arg);
> > +    char *next;
> > +    int ret = 0;
> > +
> > +    if (!mode) return AVERROR(ENOMEM);
> > +
> 
> I don't see any strtok or similar here, so I think you can drop the
> av_strdup()/av_free() for mode.

I'm still modifying the input string, which is const.

> 
> > +    next = strchr(mode, '=');
> > +    if (next)
> > +        *next++ = 0;
> > +
> > +    if (!strcmp(mode, "fail")) {
> > +        string_validation_policy = STRING_VALIDATION_POLICY_FAIL;
> > +        if (next) {
> > +            av_log(NULL, AV_LOG_ERROR,
> > +                   "No argument must be specified for the option %s with mode 'fail'\n",
> 
> "option '%s'" for consistency with most messages.
> 
> > +                   opt);
> > +            ret = AVERROR(EINVAL);
> > +            goto end;
> > +        }
> > +    } else if (!strcmp(mode, "replace")) {
> > +        string_validation_policy = STRING_VALIDATION_POLICY_REPLACE;
> > +        string_validation_replace = av_strdup(next);
> > +
> 
> I don't see any free of this
> 
> > +        if (next && !string_validation_replace) {
> > +            ret = AVERROR(ENOMEM);
> > +            goto end;
> > +        }
> > +    } else {
> > +        av_log(NULL, AV_LOG_ERROR,
> > +               "Invalid argument '%s' for option '%s', "
> 
> > +               "choose between fail, or replace=REPLACEMENT\n", arg, opt);
> 
> 'fail', or 'replace[=REPLACEMENT]'
> 
> > +        ret = AVERROR(EINVAL);
> > +        goto end;
> > +    }
> > +
> > +end:
> > +    av_free(mode);
> > +    return ret;
> > +}
> > +
> >  static int opt_pretty(void *optctx, const char *opt, const char *arg)
> >  {
> >      show_value_unit              = 1;
> > @@ -2633,6 +2759,7 @@ static const OptionDef real_options[] = {
> >      { "private",           OPT_BOOL, {(void*)&show_private_data}, "same as show_private_data" },
> >      { "bitexact", OPT_BOOL, {&do_bitexact}, "force bitexact output" },
> >      { "read_intervals", HAS_ARG, {.func_arg = opt_read_intervals}, "set read intervals", "read_intervals" },
> > +    { "string_validation_policy",  HAS_ARG, {.func_arg = opt_string_validation_policy}, "select the string validation policy", "policy_specification" },
> >      { "default", HAS_ARG | OPT_AUDIO | OPT_VIDEO | OPT_EXPERT, {.func_arg = opt_default}, "generic catch all option", "" },
> >      { "i", HAS_ARG, {.func_arg = opt_input_file_i}, "read specified file", "input_file"},
> >      { NULL, },
> 
> Could you add a broken char sequence to tests/test.ffmeta so this code is
> covered?

Unfortunately there are too many broken sequences to test.

Also which UNICODE subsets should be accepted?

For example:
http://www.w3.org/TR/REC-xml/#NT-Char

lists some disallowed characters. Other characters should be avoid in
specific circumstances, for example CANC, and others are avoided when
dealing with subtitles (check_utf8 in utils).

Ideally all these cases should be addressed in the new routine
av_get_utf8(), on which the new patch relies.
-- 
FFmpeg = Free Fabulous Magnificient Peaceless Ecumenical Guide
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-ffprobe-implement-string-validation-policy-setting.patch
Type: text/x-diff
Size: 10215 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131003/8de1dc27/attachment.bin>