diff mbox series

[FFmpeg-devel,v2,7/8] avformat/wavdec: fix s337m/spdif probing beyond data_end

Message ID 20200115105602.1184-8-nicolas.gaullier@cji.paris
State New
Headers show
Series avformat: Use s337m subdemux inside wav
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Nicolas Gaullier Jan. 15, 2020, 10:56 a.m. UTC
---
 libavformat/wavdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Carl Eugen Hoyos Jan. 15, 2020, 12:39 p.m. UTC | #1
Am Mi., 15. Jan. 2020 um 11:56 Uhr schrieb Nicolas Gaullier
<nicolas.gaullier@cji.paris>:
>
> ---
>  libavformat/wavdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavformat/wavdec.c b/libavformat/wavdec.c
> index 3571733817..d8a27c79cf 100644
> --- a/libavformat/wavdec.c
> +++ b/libavformat/wavdec.c
> @@ -77,7 +77,7 @@ static void set_spdif_s337m(AVFormatContext *s, WAVDemuxContext *wav)
>                  ret = AVERROR(ENOMEM);
>              } else {
>                  int64_t pos = avio_tell(s->pb);
> -                len = ret = avio_read(s->pb, buf, len);
> +                len = ret = avio_read(s->pb, buf, FFMIN(len, wav->data_end - pos));

Sorry if this was already answered:
What exactly does this fix? Is it possible that avio_read() overreads without
this check?

Carl Eugen
Nicolas Gaullier Jan. 15, 2020, 3:07 p.m. UTC | #2
>>
>> ---
>>  libavformat/wavdec.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavformat/wavdec.c b/libavformat/wavdec.c index 
>> 3571733817..d8a27c79cf 100644
>> --- a/libavformat/wavdec.c
>> +++ b/libavformat/wavdec.c
>> @@ -77,7 +77,7 @@ static void set_spdif_s337m(AVFormatContext *s, WAVDemuxContext *wav)
>>                  ret = AVERROR(ENOMEM);
>>              } else {
>>                  int64_t pos = avio_tell(s->pb);
>> -                len = ret = avio_read(s->pb, buf, len);
>> +                len = ret = avio_read(s->pb, buf, FFMIN(len, 
>> + wav->data_end - pos));
>
>Sorry if this was already answered:
>What exactly does this fix? Is it possible that avio_read() overreads without this check?
>
>Carl Eugen

There is no severe low level avio overread risk, but there is a risk to read bytes that are not audio data in case audio data is followed by other data (a RIFF chunk that would come just after the audio data RIFF chunk).

Here is a sample a build : http://0x0.st/zhiO.wav This sample is detected as ac3 whereas the sync codes are beyond the data chunk, in a 'junk' (I called it this way) chunk that is not supposed to be read (ex: the duration can be cross-checked with sox or audacity for example).
But again, this is obviously very improbable "in the nature" as they are three conditions :
- the wav file must be short enough (for the probe to seek in the end of the file)
- the audio data chunk must be followed by another chunk with misc data (usually those are considered "header metadata" and stand before it)
- the misc data chunk must emulate the sync codes

Nicolas
Carl Eugen Hoyos Jan. 15, 2020, 3:18 p.m. UTC | #3
Am Mi., 15. Jan. 2020 um 16:07 Uhr schrieb Gaullier Nicolas
<nicolas.gaullier@cji.paris>:
>
> >>
> >> ---
> >>  libavformat/wavdec.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/libavformat/wavdec.c b/libavformat/wavdec.c index
> >> 3571733817..d8a27c79cf 100644
> >> --- a/libavformat/wavdec.c
> >> +++ b/libavformat/wavdec.c
> >> @@ -77,7 +77,7 @@ static void set_spdif_s337m(AVFormatContext *s, WAVDemuxContext *wav)
> >>                  ret = AVERROR(ENOMEM);
> >>              } else {
> >>                  int64_t pos = avio_tell(s->pb);
> >> -                len = ret = avio_read(s->pb, buf, len);
> >> +                len = ret = avio_read(s->pb, buf, FFMIN(len,
> >> + wav->data_end - pos));
> >
> >Sorry if this was already answered:
> >What exactly does this fix? Is it possible that avio_read() overreads without this check?
> >
> >Carl Eugen
>
> There is no severe low level avio overread risk, but there is a risk to read bytes that are not audio data in case audio data is followed by other data (a RIFF chunk that would come just after the audio data RIFF chunk).
>
> Here is a sample a build : http://0x0.st/zhiO.wav This sample is detected as ac3 whereas the sync codes are beyond the data chunk, in a 'junk' (I called it this way) chunk that is not supposed to be read (ex: the duration can be cross-checked with sox or audacity for example).
> But again, this is obviously very improbable "in the nature" as they are three conditions :
> - the wav file must be short enough (for the probe to seek in the end of the file)
> - the audio data chunk must be followed by another chunk with misc data (usually those are considered "header metadata" and stand before it)
> - the misc data chunk must emulate the sync codes

Thank you for the explanation.

Carl Eugen
diff mbox series

Patch

diff --git a/libavformat/wavdec.c b/libavformat/wavdec.c
index 3571733817..d8a27c79cf 100644
--- a/libavformat/wavdec.c
+++ b/libavformat/wavdec.c
@@ -77,7 +77,7 @@  static void set_spdif_s337m(AVFormatContext *s, WAVDemuxContext *wav)
                 ret = AVERROR(ENOMEM);
             } else {
                 int64_t pos = avio_tell(s->pb);
-                len = ret = avio_read(s->pb, buf, len);
+                len = ret = avio_read(s->pb, buf, FFMIN(len, wav->data_end - pos));
                 if (len >= 0) {
                     ret = ff_spdif_probe(buf, len, &codec);
                     if (ret > AVPROBE_SCORE_EXTENSION) {