[FFmpeg-devel] [PATCH v3] Fix sdp size check on fmtp integer parameters

[Olivier MAIGNIAL]

Hello,
Just a reminder about this patch :)

On Fri, Apr 19, 2019 at 3:00 PM Olivier Maignial <olivier.maignial at smile.fr>
wrote:

> RFC-4566 do not give any limit of size on interger parameters given in
> fmtp line.
> By reading some more RFCs it is possible to find examples where some
> integers parameters are greater than 32 (see RFC-6416, 7.4)
>
> Instead I propose to check just check the eventual integer overflow.
> Using INT_MIN and INT_MAX ensure that it will work whatever the size of
> int given by compiler
>
> Signed-off-by: Olivier Maignial <olivier.maignial at smile.fr>
> ---
>
> Changes v2 -> v3:
>     Fix over/underflow checking in case of sizeof(int) == sizeof(long)
>
> If MAX/MIN_INT == MAX/MIN_LONG overflow would not be detected by just
> checking value is in range [MAX_INT,MIN_INT].
>
> In case of over/underflow strtol return MAX/MIN_LONG and set errno to
> ERANGE.
> As MAX/MIN_LONG are valid values, the only way to detect over/underflow is
> to check errno.
>
>  libavformat/rtpdec_mpeg4.c | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/libavformat/rtpdec_mpeg4.c b/libavformat/rtpdec_mpeg4.c
> index 4f70599..d40cb5a 100644
> --- a/libavformat/rtpdec_mpeg4.c
> +++ b/libavformat/rtpdec_mpeg4.c
> @@ -289,15 +289,33 @@ static int parse_fmtp(AVFormatContext *s,
>          for (i = 0; attr_names[i].str; ++i) {
>              if (!av_strcasecmp(attr, attr_names[i].str)) {
>                  if (attr_names[i].type == ATTR_NAME_TYPE_INT) {
> -                    int val = atoi(value);
> -                    if (val > 32) {
> +                    char *end_ptr = NULL;
> +                    errno = 0;
> +                    long int val = strtol(value, &end_ptr, 10);
> +                    if (value[0] == '\n' || end_ptr[0] != '\0') {
>                          av_log(s, AV_LOG_ERROR,
> -                               "The %s field size is invalid (%d)\n",
> -                               attr, val);
> +                               "The %s field value is not a number
> (%s)\n",
> +                               attr, value);
>                          return AVERROR_INVALIDDATA;
>                      }
> +                    if ((val == LONG_MAX && errno == ERANGE) ||
> +                        val > INT_MAX) {
> +                        av_log(s, AV_LOG_ERROR,
> +                               "Value of field %s overflow maximum
> integer value.\n",
> +                               attr);
> +                        return AVERROR_INVALIDDATA;
> +                    }
> +                    if ((val == LONG_MIN && errno == ERANGE) ||
> +                        val < INT_MIN)
> +                    {
> +                        av_log(s, AV_LOG_ERROR,
> +                               "Value of field %s underflow minimum
> integer value.\n",
> +                               attr);
> +                        return AVERROR_INVALIDDATA;
> +                    }
> +
>                      *(int *)((char *)data+
> -                        attr_names[i].offset) = val;
> +                        attr_names[i].offset) = (int) val;
>                  } else if (attr_names[i].type == ATTR_NAME_TYPE_STR) {
>                      char *val = av_strdup(value);
>                      if (!val)
> --
> 2.7.4
>
>