[x265] [PATCH] Validate cli argument

[Steve Borho]

On Tue, Feb 25, 2014 at 6:36 AM,  <sagar at multicorewareinc.com> wrote:
> # HG changeset patch
> # User Sagar Kotecha <sagar at multicorewareinc.com>
> # Date 1393331780 -19800
> #      Tue Feb 25 18:06:20 2014 +0530
> # Node ID 3ba68259294fc7b0dadd2e3f47b324d9c07ac2d6
> # Parent  559ef195b954fa4d3c8bbc9bf8b7619991596c36
> Validate cli argument

the checks being done in param.cpp are not necessarily CLI arguments,
so a better description would be:

param: add more validation checks

>
> diff -r 559ef195b954 -r 3ba68259294f source/common/param.cpp
> --- a/source/common/param.cpp   Tue Feb 25 14:55:20 2014 +0530
> +++ b/source/common/param.cpp   Tue Feb 25 18:06:20 2014 +0530
> @@ -444,8 +444,11 @@
>  #pragma warning(disable: 4127) // conditional expression is constant
>  #endif
>  #define OPT(STR) else if (!strcmp(name, STR))
> +#define OPTCHECK(STR) \
> +    else if (!strcmp(name, STR) && ((int32_t)atoi(value) > -1))
> +
>      if (0) ;
> -    OPT("fps")
> +    OPTCHECK("fps")

this OPTCHECK makes no sense, fps can be a float or rational number;
calling atoi() here would break it.  Please remove.

>      {
>          if (sscanf(value, "%u/%u", &p->fpsNum, &p->fpsDenom) == 2)
>              ;
> @@ -891,6 +894,29 @@
>            "Default Display Window Top Offset must be 0 or greater");
>      CHECK(param->defDispWinBottomOffset < 0,
>            "Default Display Window Bottom Offset must be 0 or greater");
> +    CHECK(param->rc.rfConstant < 0 || param->rc.rfConstant > 51,
> +          "Valid quality based VBR range 0 - 51");
> +    CHECK(param->bFrameAdaptive < 0 || param->bFrameAdaptive > 2,
> +          "Valid adaptive b scheduling values 0 - none, 1 - fast, 2 - full");
> +    CHECK(param->logLevel < -1 || param->logLevel > 3,
> +          "Valid Logging level 0:ERROR 1:WARNING 2:INFO 3:DEBUG -1:NONE");
> +    CHECK(param->scenecutThreshold < 0,
> +          "scenecutThreshold must be greter than 0");
> +    CHECK(param->rdPenalty < 0 || param->rdPenalty > 2,
> +          "Valid penalty for 32x32 intra TU in non-I slices. 0:disabled 1:RD-penalty 2:maximum");
> +    CHECK(param->keyframeMax < -1,
> +          "Invalid max IDR period in frames. value should be greter than -1");
> +    CHECK(param->decodedPictureHashSEI < 0 || param->decodedPictureHashSEI > 3,
> +          "Invalid hash option. Decoded Picture Hash SEI 0: disabled, 1: MD5, 2: CRC, 3: Checksum");
> +    CHECK(param->rc.vbvBufferSize < 0,
> +          "Size of the vbv buffer can not be less than zero");
> +    CHECK(param->rc.vbvMaxBitrate < 0,
> +          "Maximum local bit rate can not be less than zero");
> +    CHECK(param->rc.vbvBufferInit < 0 || param->rc.vbvBufferInit > 1,
> +          "Valid VBV buffer occpancy range 0 - 1");
> +    CHECK(param->rc.bitrate < 0,
> +          "Target bitrate can not be less than zero");
> +    CHECK(param->bFrameBias < 0, "Bias towards B frame decisions must be 0 or greter");
>      return check_failed;
>  }
>
> diff -r 559ef195b954 -r 3ba68259294f source/x265.cpp
> --- a/source/x265.cpp   Tue Feb 25 14:55:20 2014 +0530
> +++ b/source/x265.cpp   Tue Feb 25 18:06:20 2014 +0530
> @@ -479,13 +479,15 @@
>              }
>  #define OPT(longname) \
>      else if (!strcmp(long_options[long_options_index].name, longname))
> +#define OPTCHECK(longname, minval , maxval) \
> +    else if (!strcmp(long_options[long_options_index].name, longname) && (atoi(optarg) > minval && atoi(optarg) < maxval))
>
>              if (0) ;
> -            OPT("cpuid") cpuid = atoi(optarg);
> -            OPT("frames") this->framesToBeEncoded = (uint32_t)atoi(optarg);
> +            OPTCHECK("cpuid", -1, 2) cpuid = atoi(optarg);
> +            OPTCHECK("frames", 0, UINT_MAX) this->framesToBeEncoded = (uint32_t)atoi(optarg);
>              OPT("no-progress") this->bProgress = false;
>              OPT("seek") this->seek = (uint32_t)atoi(optarg);
> -            OPT("frame-skip") this->seek = (uint32_t)atoi(optarg);
> +            OPTCHECK("frame-skip", -1, UINT_MAX) this->seek = (uint32_t)atoi(optarg);
>              OPT("output") bitstreamfn = optarg;
>              OPT("input") inputfn = optarg;
>              OPT("recon") reconfn = optarg;

We should be using x265_atoi() here to catch invalid int strings. So
remove the static from x265_atoi() in param.cpp and use it here.

The cpuid check is wrong (it is a bitmap) and the frame-skip check is
just odd.  Both because 'seek' and 'frame-skip' are the same thing but
you only check one of them, plus you cast to a uint32_t then compare
with -1.

It's better to decouple the range checks from the parameter parsing,
so do these range checks as if() expressions after this getopt option
loop.

-- 
Steve Borho