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

Submitted by Olivier Maignial on May 22, 2019, 4:10 p.m.

Details

Message ID 1558541436-2054-1-git-send-email-olivier.maignial@smile.fr
State New
Headers show

Commit Message

Olivier Maignial May 22, 2019, 4:10 p.m.

Comments

Olivier Maignial June 7, 2019, 9:43 a.m.
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
>
>
Michael Niedermayer June 7, 2019, 6:45 p.m.
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

[...]

Patch hide | download patch | download mbox

=== 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)