[FFmpeg-devel] [PATCH] parser: error out if the codec doesn't match

[Michael Niedermayer]

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.Cadhalpun at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160105/e1b6e3af/attachment.sig>