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

Hello here!

A simple ping about this patch
If you have any question, feel free to ask!

Regards,
Olivier

On Wed, Jun 19, 2019 at 3:38 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.
> Value is then checked against INT32_MIN and INT32_MAX. Using INT32_MIN/MAX
> ensure to have the same behavior on 32 or 64 bits platforms.
>
> 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 V5 -> V6:
>     - Simplify code
>
>  libavformat/rtpdec_mpeg4.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/libavformat/rtpdec_mpeg4.c b/libavformat/rtpdec_mpeg4.c
> index 4f70599..9c4f8a1 100644
> --- a/libavformat/rtpdec_mpeg4.c
> +++ b/libavformat/rtpdec_mpeg4.c
> @@ -289,15 +289,20 @@ 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 (end_ptr == value || end_ptr[0] != '\0' ||
> +                        errno == ERANGE ||
> +                        val < INT32_MIN || val > INT32_MAX) {
>                          av_log(s, AV_LOG_ERROR,
> -                               "The %s field size is invalid (%d)\n",
> -                               attr, val);
> +                               "The %s field value is not a valid number,
> or overflows int32: %s\n",
> +                               attr, value);
>                          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
>
>

I don't think we should be using errno when avoidable, and it is avoidable here by disallowing min/max int32 values themselves. Or using strtoll.
I'm also rather sceptical about allowing negative values here, does that make sense?
Admittedly the type is set to just "int", but maybe it should be unsigned instead?

On 28.06.2019, at 08:46, Olivier MAIGNIAL <olivier.maignial@smile.fr> wrote:

> Hello here!
> 
> A simple ping about this patch
> If you have any question, feel free to ask!
> 
> Regards,
> Olivier
> 
> On Wed, Jun 19, 2019 at 3:38 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.
>> Value is then checked against INT32_MIN and INT32_MAX. Using INT32_MIN/MAX
>> ensure to have the same behavior on 32 or 64 bits platforms.
>> 
>> 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 V5 -> V6:
>>    - Simplify code
>> 
>> libavformat/rtpdec_mpeg4.c | 15 ++++++++++-----
>> 1 file changed, 10 insertions(+), 5 deletions(-)
>> 
>> diff --git a/libavformat/rtpdec_mpeg4.c b/libavformat/rtpdec_mpeg4.c
>> index 4f70599..9c4f8a1 100644
>> --- a/libavformat/rtpdec_mpeg4.c
>> +++ b/libavformat/rtpdec_mpeg4.c
>> @@ -289,15 +289,20 @@ 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 (end_ptr == value || end_ptr[0] != '\0' ||
>> +                        errno == ERANGE ||
>> +                        val < INT32_MIN || val > INT32_MAX) {
>>                         av_log(s, AV_LOG_ERROR,
>> -                               "The %s field size is invalid (%d)\n",
>> -                               attr, val);
>> +                               "The %s field value is not a valid number,
>> or overflows int32: %s\n",
>> +                               attr, value);
>>                         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
>> 
>> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Hello, thanks for review,

1) Can you give some reason about why we shouldn't use errno? I think it is
more clear to accept the full int32 range, even if min/max int32 values are
very unlikely to be used.
2) The RFC 4566 don't give any limit on fmtp parameters values. In addition
I can't find a spec that says fmtp integers parameters for AAC must be
positive. So I don't think we should limit values to positive integers here
as it would not be consistent to RFC.

On Sat, Jun 29, 2019 at 5:58 AM Reimar Döffinger <Reimar.Doeffinger@gmx.de>
wrote:

> I don't think we should be using errno when avoidable, and it is avoidable
> here by disallowing min/max int32 values themselves. Or using strtoll.
> I'm also rather sceptical about allowing negative values here, does that
> make sense?
> Admittedly the type is set to just "int", but maybe it should be unsigned
> instead?
>
> On 28.06.2019, at 08:46, Olivier MAIGNIAL <olivier.maignial@smile.fr>
> wrote:
>
> > Hello here!
> >
> > A simple ping about this patch
> > If you have any question, feel free to ask!
> >
> > Regards,
> > Olivier
> >
> > On Wed, Jun 19, 2019 at 3:38 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.
> >> Value is then checked against INT32_MIN and INT32_MAX. Using
> INT32_MIN/MAX
> >> ensure to have the same behavior on 32 or 64 bits platforms.
> >>
> >> 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 V5 -> V6:
> >>    - Simplify code
> >>
> >> libavformat/rtpdec_mpeg4.c | 15 ++++++++++-----
> >> 1 file changed, 10 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/libavformat/rtpdec_mpeg4.c b/libavformat/rtpdec_mpeg4.c
> >> index 4f70599..9c4f8a1 100644
> >> --- a/libavformat/rtpdec_mpeg4.c
> >> +++ b/libavformat/rtpdec_mpeg4.c
> >> @@ -289,15 +289,20 @@ 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 (end_ptr == value || end_ptr[0] != '\0' ||
> >> +                        errno == ERANGE ||
> >> +                        val < INT32_MIN || val > INT32_MAX) {
> >>                         av_log(s, AV_LOG_ERROR,
> >> -                               "The %s field size is invalid (%d)\n",
> >> -                               attr, val);
> >> +                               "The %s field value is not a valid
> number,
> >> or overflows int32: %s\n",
> >> +                               attr, value);
> >>                         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
> >>
> >>
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

On 03.07.2019, at 09:28, Olivier MAIGNIAL <olivier.maignial@smile.fr> wrote:

> Hello, thanks for review,
> 
> 1) Can you give some reason about why we shouldn't use errno? I think it is
> more clear to accept the full int32 range, even if min/max int32 values are
> very unlikely to be used.

It is a global variable, with all the issues that has, also for code clarity (to review code you need to know which functions modify it).
On some systems it is not even thread-local, so using it becomes completely unsafe once threads are involved, but even when it is, signal handlers can by accident change it.
I see the argument that these are rare, special and broken cases, but it's not like using errno gains all that much (esp. when using stroll is also an alternative that avoids the need).
The shorter version of that argument is that errno is a major misdesign beyond even strcpy and strncpy levels, which I personally find good reason to avoid it where reasonable.
Note that there is also the issue that "long" is 32 bit on 32 bit systems, I would expect the if condition to trigger compiler warnings there, which strtoll should avoid.

> 2) The RFC 4566 don't give any limit on fmtp parameters values. In addition
> I can't find a spec that says fmtp integers parameters for AAC must be
> positive. So I don't think we should limit values to positive integers here
> as it would not be consistent to RFC.

I admit I have not found any requirement on it being integer at all, so not sure if that assumption is not wrong to start with and should at least not trigger an error.
And nothing that would make > 32 bit wrong.