| Message ID | 20190614011532.147466-1-chcunningham@chromium.org |
|---|---|
| State | New |
| Headers | show |
+James This is patch is a follow up from an earlier thread: https://patchwork.ffmpeg.org/patch/11983/ I've implemented the easy part of that proposal. We also discussed disallowing multiple headers in an ogm change (generally a revert of: https://github.com/FFmpeg/FFmpeg/commit/81b743eb1026547270b88ac6a5cb451a3907ee94?diff=split#diff-efd47f302b213c4dc0e419c52a192819L384), but this was a little out of my depth and not critical to address my particular bug Chris
On 14.06.2019, at 03:15, Chris Cunningham <chcunningham@chromium.org> wrote: > Only "succeed" to read a header if the codec is valid. Otherwise > return AVERROR_INVALIDDATA. That doesn't sound right to me, an unknown codec in (possibly) a single stream is not an error. I understood the discussion more to say the if it's an unknown codec, we should not try to override valid codec configuration with a broken one.
On 6/14/2019 11:52 AM, Reimar Döffinger wrote: > > > On 14.06.2019, at 03:15, Chris Cunningham <chcunningham@chromium.org> wrote: > >> Only "succeed" to read a header if the codec is valid. Otherwise >> return AVERROR_INVALIDDATA. > > That doesn't sound right to me, an unknown codec in (possibly) a single stream is not an error. > I understood the discussion more to say the if it's an unknown codec, we should not try to override valid codec configuration with a broken one. I did request this change, seeing that returning codec_id none in this scenario results in a crash at a later point due to conflicting parameters. Do you suggest we should limit the change to only reject any duplicate header that may show up after the first one (and before the first data packet)?
On 14.06.2019, at 17:01, James Almer <jamrial@gmail.com> wrote: > On 6/14/2019 11:52 AM, Reimar Döffinger wrote: >> >> >> On 14.06.2019, at 03:15, Chris Cunningham <chcunningham@chromium.org> wrote: >> >>> Only "succeed" to read a header if the codec is valid. Otherwise >>> return AVERROR_INVALIDDATA. >> >> That doesn't sound right to me, an unknown codec in (possibly) a single stream is not an error. >> I understood the discussion more to say the if it's an unknown codec, we should not try to override valid codec configuration with a broken one. > > I did request this change, seeing that returning codec_id none in this > scenario results in a crash at a later point due to conflicting parameters. > > Do you suggest we should limit the change to only reject any duplicate > header that may show up after the first one (and before the first data > packet)? I don't know or understand the details, but I understood the suggestion as "do not override a valid codec ID to NONE". Either way, I would have suggested only skipping the affected stream - but I admit I have not checked how far-reaching it is to return an error here.
New patch implements the other half of James suggestion (stop parsing headers after data) and does not include the AVERROR_INVALIDDATA returns. http://ffmpeg.org/pipermail/ffmpeg-devel/2019-June/245502.html On Sun, Jun 16, 2019 at 6:16 AM Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote: > On 14.06.2019, at 17:01, James Almer <jamrial@gmail.com> wrote: > > > On 6/14/2019 11:52 AM, Reimar Döffinger wrote: > >> > >> > >> On 14.06.2019, at 03:15, Chris Cunningham <chcunningham@chromium.org> > wrote: > >> > >>> Only "succeed" to read a header if the codec is valid. Otherwise > >>> return AVERROR_INVALIDDATA. > >> > >> That doesn't sound right to me, an unknown codec in (possibly) a single > stream is not an error. > >> I understood the discussion more to say the if it's an unknown codec, > we should not try to override valid codec configuration with a broken one. > > > > I did request this change, seeing that returning codec_id none in this > > scenario results in a crash at a later point due to conflicting > parameters. > > > > Do you suggest we should limit the change to only reject any duplicate > > header that may show up after the first one (and before the first data > > packet)? > > I don't know or understand the details, but I understood the suggestion as > "do not override a valid codec ID to NONE". > Either way, I would have suggested only skipping the affected stream - but > I admit I have not checked how far-reaching it is to return an error here. > _______________________________________________ > 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".
diff --git a/libavformat/oggparseogm.c b/libavformat/oggparseogm.c index a07453760b..e71298d39a 100644 --- a/libavformat/oggparseogm.c +++ b/libavformat/oggparseogm.c @@ -24,9 +24,9 @@ #include <stdlib.h> -#include "libavutil/intreadwrite.h" - +#include "libavcodec/avcodec.h" #include "libavcodec/bytestream.h" +#include "libavutil/intreadwrite.h" #include "avformat.h" #include "internal.h" @@ -58,6 +58,8 @@ ogm_header(AVFormatContext *s, int idx) tag = bytestream2_get_le32(&p); st->codecpar->codec_id = ff_codec_get_id(ff_codec_bmp_tags, tag); st->codecpar->codec_tag = tag; + if (st->codecpar->codec_id == AV_CODEC_ID_NONE) + return AVERROR_INVALIDDATA; if (st->codecpar->codec_id == AV_CODEC_ID_MPEG4) st->need_parsing = AVSTREAM_PARSE_HEADERS; } else if (bytestream2_peek_byte(&p) == 't') { @@ -73,6 +75,8 @@ ogm_header(AVFormatContext *s, int idx) acid[4] = 0; cid = strtol(acid, NULL, 16); st->codecpar->codec_id = ff_codec_get_id(ff_codec_wav_tags, cid); + if (st->codecpar->codec_id == AV_CODEC_ID_NONE) + return AVERROR_INVALIDDATA; // our parser completely breaks AAC in Ogg if (st->codecpar->codec_id != AV_CODEC_ID_AAC) st->need_parsing = AVSTREAM_PARSE_FULL; @@ -147,6 +151,8 @@ ogm_dshow_header(AVFormatContext *s, int idx) st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO; st->codecpar->codec_id = ff_codec_get_id(ff_codec_bmp_tags, AV_RL32(p + 68)); + if (st->codecpar->codec_id == AV_CODEC_ID_NONE) + return AVERROR_INVALIDDATA; avpriv_set_pts_info(st, 64, AV_RL64(p + 164), 10000000); st->codecpar->width = AV_RL32(p + 176); st->codecpar->height = AV_RL32(p + 180); @@ -156,6 +162,8 @@ ogm_dshow_header(AVFormatContext *s, int idx) st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO; st->codecpar->codec_id = ff_codec_get_id(ff_codec_wav_tags, AV_RL16(p + 124)); + if (st->codecpar->codec_id == AV_CODEC_ID_NONE) + return AVERROR_INVALIDDATA; st->codecpar->channels = AV_RL16(p + 126); st->codecpar->sample_rate = AV_RL32(p + 128); st->codecpar->bit_rate = AV_RL32(p + 132) * 8;