[FFmpeg-devel,6/9] avformat/wavdec: fix s337m/spdif probing beyond data_end

---
 libavformat/wavdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fre 2020-01-03 klockan 16:56 +0100 skrev Nicolas Gaullier:
> ---
>  libavformat/wavdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavformat/wavdec.c b/libavformat/wavdec.c
> index 2796905e1f..ccb9576b84 100644
> --- a/libavformat/wavdec.c
> +++ b/libavformat/wavdec.c
> @@ -78,7 +78,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) {

Looks OK. I suppose this fixes a potential SIGSEGV too?

/Tomas

>> diff --git a/libavformat/wavdec.c b/libavformat/wavdec.c index 
>> 2796905e1f..ccb9576b84 100644
>> --- a/libavformat/wavdec.c
>> +++ b/libavformat/wavdec.c
>> @@ -78,7 +78,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) {
>
>Looks OK. I suppose this fixes a potential SIGSEGV too?
AVIO would just stop at EOF, I don't think a SIGSEGV could occur in any scenario.
This fix only affects probing (ie. reading start of file) in a surprising scenario where the data chunk would not extend to the end of the file.
This is many IF and I find it unlikely, but I think it should be fixed anyway.

Nicolas

Am Mo., 13. Jan. 2020 um 15:53 Uhr schrieb Gaullier Nicolas
<nicolas.gaullier@cji.paris>:
>
> >> diff --git a/libavformat/wavdec.c b/libavformat/wavdec.c index
> >> 2796905e1f..ccb9576b84 100644
> >> --- a/libavformat/wavdec.c
> >> +++ b/libavformat/wavdec.c
> >> @@ -78,7 +78,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) {
> >
> >Looks OK. I suppose this fixes a potential SIGSEGV too?
> AVIO would just stop at EOF, I don't think a SIGSEGV could occur in any scenario.
> This fix only affects probing (ie. reading start of file) in a surprising scenario where the data chunk would not extend to the end of the file.
> This is many IF and I find it unlikely, but I think it should be fixed anyway.

Could you elaborate?
I believe the code is fine as-is but maybe I miss something...

Carl Eugen

> > >> diff --git a/libavformat/wavdec.c b/libavformat/wavdec.c index
> > >> 2796905e1f..ccb9576b84 100644
> >> --- a/libavformat/wavdec.c
> > >> +++ b/libavformat/wavdec.c
> > >> @@ -78,7 +78,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) {
> > >
> > >Looks OK. I suppose this fixes a potential SIGSEGV too?
> > AVIO would just stop at EOF, I don't think a SIGSEGV could occur in any scenario.
> > This fix only affects probing (ie. reading start of file) in a surprising scenario where the data chunk would not extend to the end of the file.
> > This is many IF and I find it unlikely, but I think it should be fixed anyway.
>
> Could you elaborate?

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: 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
- the data chunk must be followed by an additional header chunk (usually those headers stand before it)
- the data chunk must emulate the sync codes

Nicolas