[FFmpeg-devel,v5] Fix integer parameters size check in SDP fmtp line

Hello,

Just a reminder about this patch
If you need something else to validate it, please let me know

Olivier

On Wed, May 22, 2019 at 6:10 PM Olivier Maignial <olivier.maignial@smile.fr>
wrote:

> === PROBLEM ===
>
> I was trying to record h264 + aac streams from an RTSP server to mp4 file.
> using this command line:
>     ffmpeg -v verbose -y -i "rtsp://<ip>/my_resources" -codec copy -bsf:a
> aac_adtstoasc test.mp4
>
> FFmpeg then fail to record audio and output this logs:
>     [rtsp @ 0xcda1f0] The profile-level-id field size is invalid (40)
>     [rtsp @ 0xcda1f0] Error parsing AU headers
>     ...
>     [rtsp @ 0xcda1f0] Could not find codec parameters for stream 1 (Audio:
> aac, 48000 Hz, 1 channels): unspecified sample format
>
> In SDP provided by my RTSP server I had this fmtp line:
>     a=fmtp:98 streamType=5; profile-level-id=40; mode=AAC-hbr;
> config=1188; sizeLength=13; indexLength=3; indexDeltaLength=3;
>
> In FFmpeg code, I found a check introduced by commit
> 24130234cd9dd733116d17b724ea4c8e12ce097a. It disallows values greater than
> 32 for fmtp line parameters.
> However, In RFC-6416 (RTP Payload Format for MPEG-4 Audio/Visual Streams)
> give examples of "profile-level-id" values for AAC, up to 55.
> Furthermore, RFC-4566 (SDP: Session Description Protocol) do not give any
> limit of size on interger parameters given in fmtp line.
>
> === FIX ===
>
> Instead of prohibit values over 32, I propose to check the possible
> integer overflow.
> The use of strtol allow to check the string validity and the possible
> overflow.
>
> To store and check return of strtol I use "long int" type and LONG_MIN/MAX
> definitions despite differences on 32/64bits platforms.
> It is consistent with the strtol man page, and it is the only way to check
> if overflow or underflow is detected by strtol.
> As the value is later checked against INT32_MIN and INT32_MAX, the
> behavior will finnaly be the same on both type of platform.
>
> This patch fix my problem and I now can record my RTSP AAC stream to mp4.
> It has passed the full fate tests suite sucessfully.
>
> Signed-off-by: Olivier Maignial <olivier.maignial@smile.fr>
> ---
>
> Changes V4 -> V5:
>     - Check value against INT32_MAX/MIN instead of INT_MAX/MIN which can
> be defferent depending on platform
>
>  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..cf35afb 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 > INT32_MAX) {
> +                        av_log(s, AV_LOG_ERROR,
> +                               "Value of field %s overflows maximum
> integer value.\n",
> +                               attr);
> +                        return AVERROR_INVALIDDATA;
> +                    }
> +                    if ((val == LONG_MIN && errno == ERANGE) ||
> +                        val < INT32_MIN)
> +                    {
> +                        av_log(s, AV_LOG_ERROR,
> +                               "Value of field %s underflows 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
>
>

On Wed, May 22, 2019 at 06:10:36PM +0200, Olivier Maignial wrote:
> === PROBLEM ===
> 
> I was trying to record h264 + aac streams from an RTSP server to mp4 file. using this command line:
>     ffmpeg -v verbose -y -i "rtsp://<ip>/my_resources" -codec copy -bsf:a aac_adtstoasc test.mp4
> 
> FFmpeg then fail to record audio and output this logs:
>     [rtsp @ 0xcda1f0] The profile-level-id field size is invalid (40)
>     [rtsp @ 0xcda1f0] Error parsing AU headers
>     ...
>     [rtsp @ 0xcda1f0] Could not find codec parameters for stream 1 (Audio: aac, 48000 Hz, 1 channels): unspecified sample format
> 
> In SDP provided by my RTSP server I had this fmtp line:
>     a=fmtp:98 streamType=5; profile-level-id=40; mode=AAC-hbr; config=1188; sizeLength=13; indexLength=3; indexDeltaLength=3;
> 
> In FFmpeg code, I found a check introduced by commit 24130234cd9dd733116d17b724ea4c8e12ce097a. It disallows values greater than 32 for fmtp line parameters.
> However, In RFC-6416 (RTP Payload Format for MPEG-4 Audio/Visual Streams) give examples of "profile-level-id" values for AAC, up to 55.
> Furthermore, RFC-4566 (SDP: Session Description Protocol) do not give any limit of size on interger parameters given in fmtp line.
> 
> === FIX ===
> 
> Instead of prohibit values over 32, I propose to check the possible integer overflow.
> The use of strtol allow to check the string validity and the possible overflow.
> 
> To store and check return of strtol I use "long int" type and LONG_MIN/MAX definitions despite differences on 32/64bits platforms.
> It is consistent with the strtol man page, and it is the only way to check if overflow or underflow is detected by strtol.
> As the value is later checked against INT32_MIN and INT32_MAX, the behavior will finnaly be the same on both type of platform.
> 
> This patch fix my problem and I now can record my RTSP AAC stream to mp4.
> It has passed the full fate tests suite sucessfully.
> 
> Signed-off-by: Olivier Maignial <olivier.maignial@smile.fr>
> ---
> 
> Changes V4 -> V5:
>     - Check value against INT32_MAX/MIN instead of INT_MAX/MIN which can be defferent depending on platform
> 
>  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..cf35afb 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 > INT32_MAX) {
> +                        av_log(s, AV_LOG_ERROR,
> +                               "Value of field %s overflows maximum integer value.\n",
> +                               attr);
> +                        return AVERROR_INVALIDDATA;
> +                    }
> +                    if ((val == LONG_MIN && errno == ERANGE) ||
> +                        val < INT32_MIN)
> +                    {
> +                        av_log(s, AV_LOG_ERROR,
> +                               "Value of field %s underflows minimum integer value.\n",
> +                               attr);
> +                        return AVERROR_INVALIDDATA;
> +                    }

That is ...  alot of code for reading a number

it doesnt feel right to do this here this way
I think either

1. this should be simplified alot, maybe down to end_ptr and LONG_MAX/LONG_MIN
check with a single errror message.

OR

2. this should be moved into its own function in libavutil so this quite 
complete error checking and reporting would be available to all code

thx

[...]