On Tue, Jan 05, 2016 at 12:02:29PM +0100, Andreas Cadhalpun wrote:
> On 05.01.2016 11:46, Michael Niedermayer wrote:
> > On Tue, Jan 05, 2016 at 12:44:40AM +0100, Andreas Cadhalpun wrote:
> >> Otherwise this can have some surprising effects (crashes), so let's
> >> better not allow it.
> >>
> >> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
> >> ---
> >> libavcodec/parser.c | 11 +++++++++++
> >> 1 file changed, 11 insertions(+)
> >>
> >> diff --git a/libavcodec/parser.c b/libavcodec/parser.c
> >> index 2809158..1f38edb 100644
> >> --- a/libavcodec/parser.c
> >> +++ b/libavcodec/parser.c
> >> @@ -141,6 +141,17 @@ int av_parser_parse2(AVCodecParserContext *s,
> >> AVCodecContext *avctx,
> >> int index, i;
> >> uint8_t dummy_buf[AV_INPUT_BUFFER_PADDING_SIZE];
> >>
> >> + if (avctx->codec_id != s->parser->codec_ids[0] &&
> >> + avctx->codec_id != s->parser->codec_ids[1] &&
> >> + avctx->codec_id != s->parser->codec_ids[2] &&
> >> + avctx->codec_id != s->parser->codec_ids[3] &&
> >> + avctx->codec_id != s->parser->codec_ids[4]) {
> >> + av_log(avctx, AV_LOG_ERROR,
> >> + "The parser doesn't match the codec %s.\n",
> >> + avcodec_get_name(avctx->codec_id));
> >> + return buf_size;
> >> + }
> >
> > does it also work to check if a parser is set when the codec id
> > is changed ? as in below
>
> That wouldn't work, as the codec id wasn't changed in force_codec_ids,
> but in the API using program.
> To reiterate, the problematic steps were:
> * call avformat_find_stream_info, which detects a codec and initializes
> a parser for it
> * afterwards change the codec id in the API using program, so it
> doesn't match with the parser
>
> Thus I think the only reliable way to detect this is a check in
> av_parser_parse2.shouldnt this be a av_assert1/2() then ? i mean this is a programming error in the user app not a error condition that should happen that way in correct usage IMHO if we need 5 extra checks thats fine but if this is just to detect a user app bug then iam not sure these checks should be run for every packet [...] > > > (that would avoid doing 5 extra checks per packet) > > Parsers aren't that speed critical code, I think. if you demux/remux audio with small packets the parser might be a sizable part speedwise, probably doesnt matter hugely but probably not entirely irrelevant either [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If a bugfix only changes things apparently unrelated to the bug with no further explanation, that is a good sign that the bugfix is wrong.
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
