| Message ID | 20201204225740.1506052-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | e546d029198950f589f7d9820970e599fba2ad30 |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avcodec/av1dec: Fix leak in case of failure | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
On 12/4/2020 7:57 PM, Andreas Rheinhardt wrote: > A reference to an AV1RawFrameHeader and consequently the > AV1RawFrameHeader itself and everything it has a reference to leak > if the hardware has no AV1 decoding capabilities. It looks like it can happen if get_buffer() fails even with hardware support. > It happens e.g. > in the cbs-av1-av1-1-b8-02-allintra FATE-test; it has just been masked > because the return value of ffmpeg (which indicates failure when using > Valgrind or ASAN) is ignored when doing tests of type md5. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > I switched the av_buffer_ref() and update_context_with_frame_header() > because the latter does not need any cleanup on failure. > > Also notice that actual decoding with this patch applied is completely > untested. > > libavcodec/av1dec.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c > index 1589b8f0c0..d7b2ac9d46 100644 > --- a/libavcodec/av1dec.c > +++ b/libavcodec/av1dec.c > @@ -674,20 +674,20 @@ static int av1_frame_alloc(AVCodecContext *avctx, AV1Frame *f) > AVFrame *frame; > int ret; > > - f->header_ref = av_buffer_ref(s->header_ref); > - if (!f->header_ref) > - return AVERROR(ENOMEM); > - > - f->raw_frame_header = s->raw_frame_header; > - > ret = update_context_with_frame_header(avctx, header); > if (ret < 0) { > av_log(avctx, AV_LOG_ERROR, "Failed to update context with frame header\n"); > return ret; > } > > + f->header_ref = av_buffer_ref(s->header_ref); > + if (!f->header_ref) > + return AVERROR(ENOMEM); > + > + f->raw_frame_header = s->raw_frame_header; > + > if ((ret = ff_thread_get_buffer(avctx, &f->tf, AV_GET_BUFFER_FLAG_REF)) < 0) > - return ret; > + goto fail; > > frame = f->tf.f; > frame->key_frame = header->frame_type == AV1_FRAME_KEY; > @@ -710,8 +710,10 @@ static int av1_frame_alloc(AVCodecContext *avctx, AV1Frame *f) > if (hwaccel->frame_priv_data_size) { > f->hwaccel_priv_buf = > av_buffer_allocz(hwaccel->frame_priv_data_size); > - if (!f->hwaccel_priv_buf) > + if (!f->hwaccel_priv_buf) { > + ret = AVERROR(ENOMEM); > goto fail; > + } > f->hwaccel_picture_private = f->hwaccel_priv_buf->data; > } > } > @@ -719,7 +721,7 @@ static int av1_frame_alloc(AVCodecContext *avctx, AV1Frame *f) > > fail: Just to be safe, add a ret = 0 above this line. > av1_frame_unref(avctx, f); > - return AVERROR(ENOMEM); > + return ret; > } > > static int set_output_frame(AVCodecContext *avctx, AVFrame *frame, LGTM, and while unrelated to this fix, this also reveals that in some scenarios, decoding without hardware support reaches this point, when it's meant to abort when parsing the sequence header and being unable to set up a hardware pixel format in avctx. Looks like when parsing a second sequence header (Like in the second keyframe) where s->pix_fmt is already set to a software format, get_pixel_format() is not called, and so decoding proceeds to deal with frames.
James Almer: > On 12/4/2020 7:57 PM, Andreas Rheinhardt wrote: >> A reference to an AV1RawFrameHeader and consequently the >> AV1RawFrameHeader itself and everything it has a reference to leak >> if the hardware has no AV1 decoding capabilities. > > It looks like it can happen if get_buffer() fails even with hardware > support. > >> It happens e.g. >> in the cbs-av1-av1-1-b8-02-allintra FATE-test; it has just been masked >> because the return value of ffmpeg (which indicates failure when using >> Valgrind or ASAN) is ignored when doing tests of type md5. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> I switched the av_buffer_ref() and update_context_with_frame_header() >> because the latter does not need any cleanup on failure. >> >> Also notice that actual decoding with this patch applied is completely >> untested. >> >> libavcodec/av1dec.c | 20 +++++++++++--------- >> 1 file changed, 11 insertions(+), 9 deletions(-) >> >> diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c >> index 1589b8f0c0..d7b2ac9d46 100644 >> --- a/libavcodec/av1dec.c >> +++ b/libavcodec/av1dec.c >> @@ -674,20 +674,20 @@ static int av1_frame_alloc(AVCodecContext >> *avctx, AV1Frame *f) >> AVFrame *frame; >> int ret; >> - f->header_ref = av_buffer_ref(s->header_ref); >> - if (!f->header_ref) >> - return AVERROR(ENOMEM); >> - >> - f->raw_frame_header = s->raw_frame_header; >> - >> ret = update_context_with_frame_header(avctx, header); >> if (ret < 0) { >> av_log(avctx, AV_LOG_ERROR, "Failed to update context with >> frame header\n"); >> return ret; >> } >> + f->header_ref = av_buffer_ref(s->header_ref); >> + if (!f->header_ref) >> + return AVERROR(ENOMEM); >> + >> + f->raw_frame_header = s->raw_frame_header; >> + >> if ((ret = ff_thread_get_buffer(avctx, &f->tf, >> AV_GET_BUFFER_FLAG_REF)) < 0) >> - return ret; >> + goto fail; >> frame = f->tf.f; >> frame->key_frame = header->frame_type == AV1_FRAME_KEY; >> @@ -710,8 +710,10 @@ static int av1_frame_alloc(AVCodecContext *avctx, >> AV1Frame *f) >> if (hwaccel->frame_priv_data_size) { >> f->hwaccel_priv_buf = >> av_buffer_allocz(hwaccel->frame_priv_data_size); >> - if (!f->hwaccel_priv_buf) >> + if (!f->hwaccel_priv_buf) { >> + ret = AVERROR(ENOMEM); >> goto fail; >> + } >> f->hwaccel_picture_private = f->hwaccel_priv_buf->data; >> } >> } >> @@ -719,7 +721,7 @@ static int av1_frame_alloc(AVCodecContext *avctx, >> AV1Frame *f) >> fail: > > Just to be safe, add a ret = 0 above this line. > There is a "return 0;" above this line (the non-error path does not include this av1_frame_unref()), so this makes no sense. >> av1_frame_unref(avctx, f); >> - return AVERROR(ENOMEM); >> + return ret; >> } >> static int set_output_frame(AVCodecContext *avctx, AVFrame *frame, > > LGTM, and while unrelated to this fix, this also reveals that in some > scenarios, decoding without hardware support reaches this point, when > it's meant to abort when parsing the sequence header and being unable to > set up a hardware pixel format in avctx. > Yeah, I get a screen full of error messages from this decoder. > Looks like when parsing a second sequence header (Like in the second > keyframe) where s->pix_fmt is already set to a software format, > get_pixel_format() is not called, and so decoding proceeds to deal with > frames.
On 12/4/2020 8:19 PM, Andreas Rheinhardt wrote: > James Almer: >> On 12/4/2020 7:57 PM, Andreas Rheinhardt wrote: >>> A reference to an AV1RawFrameHeader and consequently the >>> AV1RawFrameHeader itself and everything it has a reference to leak >>> if the hardware has no AV1 decoding capabilities. >> >> It looks like it can happen if get_buffer() fails even with hardware >> support. >> >>> It happens e.g. >>> in the cbs-av1-av1-1-b8-02-allintra FATE-test; it has just been masked >>> because the return value of ffmpeg (which indicates failure when using >>> Valgrind or ASAN) is ignored when doing tests of type md5. >>> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>> --- >>> I switched the av_buffer_ref() and update_context_with_frame_header() >>> because the latter does not need any cleanup on failure. >>> >>> Also notice that actual decoding with this patch applied is completely >>> untested. >>> >>> libavcodec/av1dec.c | 20 +++++++++++--------- >>> 1 file changed, 11 insertions(+), 9 deletions(-) >>> >>> diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c >>> index 1589b8f0c0..d7b2ac9d46 100644 >>> --- a/libavcodec/av1dec.c >>> +++ b/libavcodec/av1dec.c >>> @@ -674,20 +674,20 @@ static int av1_frame_alloc(AVCodecContext >>> *avctx, AV1Frame *f) >>> AVFrame *frame; >>> int ret; >>> - f->header_ref = av_buffer_ref(s->header_ref); >>> - if (!f->header_ref) >>> - return AVERROR(ENOMEM); >>> - >>> - f->raw_frame_header = s->raw_frame_header; >>> - >>> ret = update_context_with_frame_header(avctx, header); >>> if (ret < 0) { >>> av_log(avctx, AV_LOG_ERROR, "Failed to update context with >>> frame header\n"); >>> return ret; >>> } >>> + f->header_ref = av_buffer_ref(s->header_ref); >>> + if (!f->header_ref) >>> + return AVERROR(ENOMEM); >>> + >>> + f->raw_frame_header = s->raw_frame_header; >>> + >>> if ((ret = ff_thread_get_buffer(avctx, &f->tf, >>> AV_GET_BUFFER_FLAG_REF)) < 0) >>> - return ret; >>> + goto fail; >>> frame = f->tf.f; >>> frame->key_frame = header->frame_type == AV1_FRAME_KEY; >>> @@ -710,8 +710,10 @@ static int av1_frame_alloc(AVCodecContext *avctx, >>> AV1Frame *f) >>> if (hwaccel->frame_priv_data_size) { >>> f->hwaccel_priv_buf = >>> av_buffer_allocz(hwaccel->frame_priv_data_size); >>> - if (!f->hwaccel_priv_buf) >>> + if (!f->hwaccel_priv_buf) { >>> + ret = AVERROR(ENOMEM); >>> goto fail; >>> + } >>> f->hwaccel_picture_private = f->hwaccel_priv_buf->data; >>> } >>> } >>> @@ -719,7 +721,7 @@ static int av1_frame_alloc(AVCodecContext *avctx, >>> AV1Frame *f) >>> fail: >> >> Just to be safe, add a ret = 0 above this line. >> > There is a "return 0;" above this line (the non-error path does not > include this av1_frame_unref()), so this makes no sense. Ah true, missed it. For some reason i assumed it was written the same way as some bsfs where it falls through. Nevermind then. > >>> av1_frame_unref(avctx, f); >>> - return AVERROR(ENOMEM); >>> + return ret; >>> } >>> static int set_output_frame(AVCodecContext *avctx, AVFrame *frame, >> >> LGTM, and while unrelated to this fix, this also reveals that in some >> scenarios, decoding without hardware support reaches this point, when >> it's meant to abort when parsing the sequence header and being unable to >> set up a hardware pixel format in avctx. >> > Yeah, I get a screen full of error messages from this decoder. You'll get errors no matter what, but the idea is that they should not be get_buffer() ones, since they are a lot noisier. > >> Looks like when parsing a second sequence header (Like in the second >> keyframe) where s->pix_fmt is already set to a software format, >> get_pixel_format() is not called, and so decoding proceeds to deal with >> frames. > _______________________________________________ > 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/libavcodec/av1dec.c b/libavcodec/av1dec.c index 1589b8f0c0..d7b2ac9d46 100644 --- a/libavcodec/av1dec.c +++ b/libavcodec/av1dec.c @@ -674,20 +674,20 @@ static int av1_frame_alloc(AVCodecContext *avctx, AV1Frame *f) AVFrame *frame; int ret; - f->header_ref = av_buffer_ref(s->header_ref); - if (!f->header_ref) - return AVERROR(ENOMEM); - - f->raw_frame_header = s->raw_frame_header; - ret = update_context_with_frame_header(avctx, header); if (ret < 0) { av_log(avctx, AV_LOG_ERROR, "Failed to update context with frame header\n"); return ret; } + f->header_ref = av_buffer_ref(s->header_ref); + if (!f->header_ref) + return AVERROR(ENOMEM); + + f->raw_frame_header = s->raw_frame_header; + if ((ret = ff_thread_get_buffer(avctx, &f->tf, AV_GET_BUFFER_FLAG_REF)) < 0) - return ret; + goto fail; frame = f->tf.f; frame->key_frame = header->frame_type == AV1_FRAME_KEY; @@ -710,8 +710,10 @@ static int av1_frame_alloc(AVCodecContext *avctx, AV1Frame *f) if (hwaccel->frame_priv_data_size) { f->hwaccel_priv_buf = av_buffer_allocz(hwaccel->frame_priv_data_size); - if (!f->hwaccel_priv_buf) + if (!f->hwaccel_priv_buf) { + ret = AVERROR(ENOMEM); goto fail; + } f->hwaccel_picture_private = f->hwaccel_priv_buf->data; } } @@ -719,7 +721,7 @@ static int av1_frame_alloc(AVCodecContext *avctx, AV1Frame *f) fail: av1_frame_unref(avctx, f); - return AVERROR(ENOMEM); + return ret; } static int set_output_frame(AVCodecContext *avctx, AVFrame *frame,
A reference to an AV1RawFrameHeader and consequently the AV1RawFrameHeader itself and everything it has a reference to leak if the hardware has no AV1 decoding capabilities. It happens e.g. in the cbs-av1-av1-1-b8-02-allintra FATE-test; it has just been masked because the return value of ffmpeg (which indicates failure when using Valgrind or ASAN) is ignored when doing tests of type md5. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- I switched the av_buffer_ref() and update_context_with_frame_header() because the latter does not need any cleanup on failure. Also notice that actual decoding with this patch applied is completely untested. libavcodec/av1dec.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)