diff mbox

[FFmpeg-devel,ogg] Respect AVERROR codes returned by ogg header parsing.

Message ID CAPUDrwduhebrLx84GR7pUN-uBf0Xfk_+vNY++gG5W_UFZv2EoA@mail.gmail.com
State Superseded
Headers show

Commit Message

Dale Curtis Nov. 28, 2017, 10:17 p.m. UTC
Fixes bad parens in the above patch.

- dale

On Tue, Nov 28, 2017 at 2:03 PM, Dale Curtis <dalecurtis@chromium.org>
wrote:

> Actually packet() was broken too, updated the patch to fix this case too.
>
> - dale
>
> On Tue, Nov 28, 2017 at 1:41 PM, Dale Curtis <dalecurtis@chromium.org>
> wrote:
>
>> Fixes ticket #6804. All of the ogg header parsers may return
>> standard AVERROR codes; these return values should not be
>> treated as success.
>>
>> Signed-off-by: Dale Curtis <dalecurtis@chromium.org>
>>
>>
>>
>

Comments

Michael Niedermayer Nov. 29, 2017, 12:32 a.m. UTC | #1
On Tue, Nov 28, 2017 at 02:17:21PM -0800, Dale Curtis wrote:
> Fixes bad parens in the above patch.
> 
> - dale
> 
> On Tue, Nov 28, 2017 at 2:03 PM, Dale Curtis <dalecurtis@chromium.org>
> wrote:
> 
> > Actually packet() was broken too, updated the patch to fix this case too.
> >
> > - dale
> >
> > On Tue, Nov 28, 2017 at 1:41 PM, Dale Curtis <dalecurtis@chromium.org>
> > wrote:
> >
> >> Fixes ticket #6804. All of the ogg header parsers may return
> >> standard AVERROR codes; these return values should not be
> >> treated as success.
> >>
> >> Signed-off-by: Dale Curtis <dalecurtis@chromium.org>
> >>
> >>
> >>
> >

>  oggdec.c |   10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 07e15d08be7fcce839206cdd699eea804b1520ff  fix_ogg_v3.patch
> From 8d3b21ab59d835430f057cfb40a63318b25e7014 Mon Sep 17 00:00:00 2001
> From: Dale Curtis <dalecurtis@chromium.org>
> Date: Tue, 28 Nov 2017 13:40:20 -0800
> Subject: [PATCH] Respect AVERROR codes returned by ogg parsers.
> 
> Fixes ticket #6804. All of the ogg header and packet parsers may
> return standard AVERROR codes; these return values should not be
> treated as success.
> 
> Signed-off-by: Dale Curtis <dalecurtis@chromium.org>
> ---
>  libavformat/oggdec.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)

This breaks converting of this:
./ffmpeg -i bgc.sub.dub.ogm -vf subtitles=bgc.sub.dub.ogm -an  file.avi

https://samples.ffmpeg.org/ogg/bgc.sub.dub.ogm

[...]
Dale Curtis Nov. 29, 2017, 7:57 p.m. UTC | #2
On Tue, Nov 28, 2017 at 4:32 PM, Michael Niedermayer <michael@niedermayer.cc
> wrote:
>
>
> This breaks converting of this:
> ./ffmpeg -i bgc.sub.dub.ogm -vf subtitles=bgc.sub.dub.ogm -an  file.avi
>
> https://samples.ffmpeg.org/ogg/bgc.sub.dub.ogm
>
>
That's due to the vorbis parser returning AVERROR_INVALIDDATA here, with
pkt_type == 5 and priv->packet[0] == priv_packet[1] == null.
http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavformat/oggparsevorbis.c;h=65b1998a02d92d0fcaeb740d8f4523641502dbea;hb=HEAD#l319


Which were added in 2010 here:
http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=73c44cb2869bfdbea829942eb35efa6d4c4e2f91

It seems that file has invalid vorbis streams if the check is to be
believed. What do you want to do here? Restrict this error behind
AV_EF_EXPLODE?

- dale
Michael Niedermayer Nov. 30, 2017, 1:58 a.m. UTC | #3
On Wed, Nov 29, 2017 at 11:57:17AM -0800, Dale Curtis wrote:
> On Tue, Nov 28, 2017 at 4:32 PM, Michael Niedermayer <michael@niedermayer.cc
> > wrote:
> >
> >
> > This breaks converting of this:
> > ./ffmpeg -i bgc.sub.dub.ogm -vf subtitles=bgc.sub.dub.ogm -an  file.avi
> >
> > https://samples.ffmpeg.org/ogg/bgc.sub.dub.ogm
> >
> >
> That's due to the vorbis parser returning AVERROR_INVALIDDATA here, with
> pkt_type == 5 and priv->packet[0] == priv_packet[1] == null.
> http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavformat/oggparsevorbis.c;h=65b1998a02d92d0fcaeb740d8f4523641502dbea;hb=HEAD#l319
> 
> 
> Which were added in 2010 here:
> http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=73c44cb2869bfdbea829942eb35efa6d4c4e2f91
> 
> It seems that file has invalid vorbis streams if the check is to be
> believed. What do you want to do here? Restrict this error behind
> AV_EF_EXPLODE?

AV_EF_EXPLODE appears to be a possibility, yes
in general demuxers should try to extract as much as possible from a
file and not give up if some broken packet is found


[...]
diff mbox

Patch

From 8d3b21ab59d835430f057cfb40a63318b25e7014 Mon Sep 17 00:00:00 2001
From: Dale Curtis <dalecurtis@chromium.org>
Date: Tue, 28 Nov 2017 13:40:20 -0800
Subject: [PATCH] Respect AVERROR codes returned by ogg parsers.

Fixes ticket #6804. All of the ogg header and packet parsers may
return standard AVERROR codes; these return values should not be
treated as success.

Signed-off-by: Dale Curtis <dalecurtis@chromium.org>
---
 libavformat/oggdec.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index 193a286e43..281eba44cf 100644
--- a/libavformat/oggdec.c
+++ b/libavformat/oggdec.c
@@ -543,7 +543,9 @@  static int ogg_packet(AVFormatContext *s, int *sid, int *dstart, int *dsize,
     os->incomplete = 0;
 
     if (os->header) {
-        os->header = os->codec->header(s, idx);
+        if ((ret = os->codec->header(s, idx)) < 0)
+            return ret;
+        os->header = ret;
         if (!os->header) {
             os->segp  = segp;
             os->psize = psize;
@@ -574,8 +576,10 @@  static int ogg_packet(AVFormatContext *s, int *sid, int *dstart, int *dsize,
     } else {
         os->pflags    = 0;
         os->pduration = 0;
-        if (os->codec && os->codec->packet)
-            os->codec->packet(s, idx);
+        if (os->codec && os->codec->packet) {
+            if ((ret = os->codec->packet(s, idx)) < 0)
+                return ret;
+        }
         if (sid)
             *sid = idx;
         if (dstart)
-- 
2.15.0.417.g466bffb3ac-goog