diff mbox

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

Message ID 1558519435-31008-1-git-send-email-olivier.maignial@smile.fr
State Superseded
Headers show

Commit Message

Olivier Maignial May 22, 2019, 10:03 a.m. UTC

Comments

Moritz Barsnick May 22, 2019, 1:50 p.m. UTC | #1 
On Wed, May 22, 2019 at 12:03:55 +0200, Olivier Maignial wrote:
> 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 disallow 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.

This explanation is much more concise than before. Thanks. Thus,
understanding the intent, I can now finally comment on the code:

> Using INT_MIN, LONG_MIN, INT_MAX and LON_MAX definitions ensure that it will work whatever the size of int/long given by compiler.
[...]
> +                    long int val = strtol(value, &end_ptr, 10);

According to your comment, you do realize that this depends on the
compiler. I actually believe it depends mainly on the platform, but
that's an academic discussion. Either way, why not support the same
range regardless of compiler or platform? I.e. avoid "long int", which
is sometimes 32 bits, sometimes 64.

(Anything else could lead to
unexpected behavior. In other words, an observation such as: "That's
peculiar, my [64 bit] PC handles this stream just fine, but my [32 bit]
RaspPi doesn't.")

> +                               "Value of field %s overflow maximum integer value.\n",

Singular: "underflows".

> +                               "Value of field %s underflow minimum integer value.\n",

Ditto.

Moritz
diff mbox

Patch

=== 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 disallow 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.
Using INT_MIN, LONG_MIN, INT_MAX and LON_MAX definitions ensure that it will work whatever the size of int/long given by compiler.

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 v3->v4
    - Rebased my patch on master
    - Updated comit log to provide better explanation of the problem
    - Re-passed fate tests on master


 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)