[FFmpeg-devel] avformat/hlsenc: Improve checks for invalid stream mappings

The mapping of streams to the various variant streams to be created by
the HLS muxer is roughly as follows: Space and tab separate variant
stream group maps while the entries in each variant stream group map are
separated by ','.

The parsing process of each variant stream group proceeded as follows:
At first the number of occurences of "a:", "v:" and "s:" in each variant
stream group is calculated so that one can can allocate an array of
streams with this number of entries. Then each entry is checked and the
check for stream numbers was deficient: It did check that there is a
number beginning after the ":", but it did not check that the number
extends until the next "," (or until the end).

This means that an invalid variant stream group like v:0_v:1 will not be
rejected; the problem is that the variant stream in this example is
supposed to have two streams associated with it (because it contains two
"v:"), yet only one stream is actually associated with it (because there
is no ',' to start a second stream specifier). This discrepancy led to
segfaults (null pointer dereferencing) in the rest of the code (when the
nonexistent second stream associated to the variant stream was inspected).

Furthermore, this commit also removes an instance of using atoi() whose
behaviour on a range error is undefined.

Fixes ticket #8652.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavformat/hlsenc.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

> 2020年5月6日 下午12:17，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道：
> 
> The mapping of streams to the various variant streams to be created by
> the HLS muxer is roughly as follows: Space and tab separate variant
> stream group maps while the entries in each variant stream group map are
> separated by ','.
> 
> The parsing process of each variant stream group proceeded as follows:
> At first the number of occurences of "a:", "v:" and "s:" in each variant
> stream group is calculated so that one can can allocate an array of
> streams with this number of entries. Then each entry is checked and the
> check for stream numbers was deficient: It did check that there is a
> number beginning after the ":", but it did not check that the number
> extends until the next "," (or until the end).
> 
> This means that an invalid variant stream group like v:0_v:1 will not be
> rejected; the problem is that the variant stream in this example is
> supposed to have two streams associated with it (because it contains two
> "v:"), yet only one stream is actually associated with it (because there
> is no ',' to start a second stream specifier). This discrepancy led to
> segfaults (null pointer dereferencing) in the rest of the code (when the
> nonexistent second stream associated to the variant stream was inspected).
> 
> Furthermore, this commit also removes an instance of using atoi() whose
> behaviour on a range error is undefined.
> 
> Fixes ticket #8652.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> libavformat/hlsenc.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index b269d015d8..5695c6cc95 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -1880,7 +1880,7 @@ fail:
> 
> static int get_nth_codec_stream_index(AVFormatContext *s,
>                                       enum AVMediaType codec_type,
> -                                      int stream_id)
> +                                      int64_t stream_id)
> {
>     unsigned int stream_index, cnt;
>     if (stream_id < 0 || stream_id > s->nb_streams - 1)
> @@ -1963,6 +1963,8 @@ static int parse_variant_stream_mapstring(AVFormatContext *s)
> 
>         nb_streams = 0;
>         while (keyval = av_strtok(varstr, ",", &saveptr2)) {
> +            int64_t num;
> +            char *end;
>             varstr = NULL;
>             if (av_strstart(keyval, "language:", &val)) {
>                 av_free(vs->language);
> @@ -2011,10 +2013,12 @@ static int parse_variant_stream_mapstring(AVFormatContext *s)
>                 return AVERROR(EINVAL);
>             }
> 
> -            stream_index = -1;
> -            if (av_isdigit(*val))
> -                stream_index = get_nth_codec_stream_index (s, codec_type,
> -                                                           atoi(val));
> +            num = strtoll(val, &end, 0);
> +            if (!av_isdigit(*val) || *end != '\0') {
> +                av_log(s, AV_LOG_ERROR, "Invalid stream number: '%s'\n", val);
> +                return AVERROR(EINVAL);
> +            }
> +            stream_index = get_nth_codec_stream_index(s, codec_type, num);
> 
>             if (stream_index >= 0 && nb_streams < vs->nb_streams) {
>                 for (i = 0; nb_streams > 0 && i < nb_streams; i++) {
> -- 
> 2.20.1
> 
> _______________________________________________
> 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".

LGTM


Thanks

Steven Liu

Steven Liu:
> 
> 
>> 2020年5月6日 下午12:17，Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 写道：
>>
>> The mapping of streams to the various variant streams to be created by
>> the HLS muxer is roughly as follows: Space and tab separate variant
>> stream group maps while the entries in each variant stream group map are
>> separated by ','.
>>
>> The parsing process of each variant stream group proceeded as follows:
>> At first the number of occurences of "a:", "v:" and "s:" in each variant
>> stream group is calculated so that one can can allocate an array of
>> streams with this number of entries. Then each entry is checked and the
>> check for stream numbers was deficient: It did check that there is a
>> number beginning after the ":", but it did not check that the number
>> extends until the next "," (or until the end).
>>
>> This means that an invalid variant stream group like v:0_v:1 will not be
>> rejected; the problem is that the variant stream in this example is
>> supposed to have two streams associated with it (because it contains two
>> "v:"), yet only one stream is actually associated with it (because there
>> is no ',' to start a second stream specifier). This discrepancy led to
>> segfaults (null pointer dereferencing) in the rest of the code (when the
>> nonexistent second stream associated to the variant stream was inspected).
>>
>> Furthermore, this commit also removes an instance of using atoi() whose
>> behaviour on a range error is undefined.
>>
>> Fixes ticket #8652.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>> libavformat/hlsenc.c | 14 +++++++++-----
>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
>> index b269d015d8..5695c6cc95 100644
>> --- a/libavformat/hlsenc.c
>> +++ b/libavformat/hlsenc.c
>> @@ -1880,7 +1880,7 @@ fail:
>>
>> static int get_nth_codec_stream_index(AVFormatContext *s,
>>                                       enum AVMediaType codec_type,
>> -                                      int stream_id)
>> +                                      int64_t stream_id)
>> {
>>     unsigned int stream_index, cnt;
>>     if (stream_id < 0 || stream_id > s->nb_streams - 1)
>> @@ -1963,6 +1963,8 @@ static int parse_variant_stream_mapstring(AVFormatContext *s)
>>
>>         nb_streams = 0;
>>         while (keyval = av_strtok(varstr, ",", &saveptr2)) {
>> +            int64_t num;
>> +            char *end;
>>             varstr = NULL;
>>             if (av_strstart(keyval, "language:", &val)) {
>>                 av_free(vs->language);
>> @@ -2011,10 +2013,12 @@ static int parse_variant_stream_mapstring(AVFormatContext *s)
>>                 return AVERROR(EINVAL);
>>             }
>>
>> -            stream_index = -1;
>> -            if (av_isdigit(*val))
>> -                stream_index = get_nth_codec_stream_index (s, codec_type,
>> -                                                           atoi(val));
>> +            num = strtoll(val, &end, 0);
>> +            if (!av_isdigit(*val) || *end != '\0') {
>> +                av_log(s, AV_LOG_ERROR, "Invalid stream number: '%s'\n", val);
>> +                return AVERROR(EINVAL);
>> +            }
>> +            stream_index = get_nth_codec_stream_index(s, codec_type, num);
>>
>>             if (stream_index >= 0 && nb_streams < vs->nb_streams) {
>>                 for (i = 0; nb_streams > 0 && i < nb_streams; i++) {
>> -- 
>> 2.20.1
>>
>> _______________________________________________
>> 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".
> 
> LGTM
> 
> 
> Thanks
> 
> Steven Liu
> 
> 
> 
Applied. Thanks.

- Andreas