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

Olivier MAIGNIAL olivier.maignial at smile.fr
Wed Jul 3 10:28:04 EEST 2019


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 at 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 at 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 at 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 at 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 at ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".


More information about the ffmpeg-devel mailing list