| Message ID | 1556588750-8637-1-git-send-email-mryancen@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Yan Cen > Sent: Tuesday, April 30, 2019 9:46 AM > To: ffmpeg-devel@ffmpeg.org > Cc: Yan, CenX <cenx.yan@intel.com> > Subject: [FFmpeg-devel] [PATCH] libavcodec/vp9: fix ref-frame size judging > method > > From: Yan Cen <cenx.yan@intel.com> > > There is no need all reference frame demension is valid in libvpx. > > So this change contains three part: > 1. Change each judgement's loglevel from "ERROR" to "WARNING" > 2. Make sure at least one of frames that this frame references has valid > dimension. > 3. All judgements fail would report "ERROR". > > Signed-off-by: Yan Cen<cenx.yan@intel.com> > --- > libavcodec/vp9.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c index acf3ffc..8dd14e0 > 100644 > --- a/libavcodec/vp9.c > +++ b/libavcodec/vp9.c > @@ -790,6 +790,7 @@ static int decode_frame_header(AVCodecContext > *avctx, > > /* check reference frames */ > if (!s->s.h.keyframe && !s->s.h.intraonly) { > + int has_valid_ref_frame = 0; > for (i = 0; i < 3; i++) { > AVFrame *ref = s->s.refs[s->s.h.refidx[i]].f; > int refw = ref->width, refh = ref->height; @@ -802,12 > +803,14 @@ static int decode_frame_header(AVCodecContext *avctx, > return AVERROR_INVALIDDATA; > } else if (refw == w && refh == h) { > s->mvscale[i][0] = s->mvscale[i][1] = 0; > + has_valid_ref_frame = 1; > } else { > - if (w * 2 < refw || h * 2 < refh || w > 16 * refw || h > 16 > * refh) { > - av_log(avctx, AV_LOG_ERROR, > + int is_ref_frame_invalid = (w * 2 < refw || h * 2 < refh > || w > 16 * refw || h > 16 * refh); > + has_valid_ref_frame |= !is_ref_frame_invalid; > + if (is_ref_frame_invalid) { > + av_log(avctx, AV_LOG_WARNING, > "Invalid ref frame dimensions %dx%d for > frame size %dx%d\n", > refw, refh, w, h); > - return AVERROR_INVALIDDATA; > } Would be higher efficient and easier to read if change as blew? if (is_ref_frame_invalid) warning_log; else has_valid_ref_frame = 1; > s->mvscale[i][0] = (refw << 14) / w; > s->mvscale[i][1] = (refh << 14) / h; @@ -815,6 +818,11 > @@ static int decode_frame_header(AVCodecContext *avctx, > s->mvstep[i][1] = 16 * s->mvscale[i][1] >> 14; > } > } > + if (!has_valid_ref_frame) { > + av_log(avctx, AV_LOG_ERROR, > + "Referenced frame has invalid size\n"); > + return AVERROR_INVALIDDATA; > + } > } > > if (s->s.h.keyframe || s->s.h.errorres || (s->s.h.intraonly && > s->s.h.resetctx == 3)) { > -- > 2.7.4 Patch makes sense, but I wonder is there any clip to reproduce the issue you want to fix?
Hi, On Mon, Apr 29, 2019 at 10:03 PM Yan Cen <mryancen@gmail.com> wrote: > From: Yan Cen <cenx.yan@intel.com> > > There is no need all reference frame demension is valid in libvpx. I think you're touching on a bigger issue here: libvpx allows having invalid (or even missing) references, and we don't. Note that this means we lack per-block reference validity checks in the block reconstruction code, and your patch is not adding them, thus exposing us to potential security issues there. Ronald
Hi, On Wed, May 22, 2019 at 2:12 AM Yan, CenX <cenx.yan@intel.com> wrote: > Hi, Ronald > > > > Sorry to reply late. > > > > For you question, “we lack per-block reference validity checks in the > block reconstruction code”, would you mean encode process need validity > checks? > No. The decoder needs validity checks. I understand you're modifying libavcodec/vp9.c because of how its intertwined with the vp9 hw deoders, but it is primarily a software decoder. Ronald
Hi, Ronald Sorry to reply late. For you question, “we lack per-block reference validity checks in the block reconstruction code”, I'm little confused about the red word,would you mean encode process need validity checks or there need to check the reference id of each frame? And this patch will act on decode process, all references have already been appointed by the stream. Br Cen Ronald S. Bultje <rsbultje@gmail.com> 于2019年5月1日周三 上午7:19写道： > Hi, > > On Mon, Apr 29, 2019 at 10:03 PM Yan Cen <mryancen@gmail.com> wrote: > > > From: Yan Cen <cenx.yan@intel.com> > > > > There is no need all reference frame demension is valid in libvpx. > > > I think you're touching on a bigger issue here: libvpx allows having > invalid (or even missing) references, and we don't. Note that this means we > lack per-block reference validity checks in the block reconstruction code, > and your patch is not adding them, thus exposing us to potential security > issues there. > > Ronald > _______________________________________________ > 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".
Hi guys, On Wed, May 22, 2019 at 11:14 PM 严小复 <mryancen@gmail.com> wrote: > code”, I'm little confused about the red word,would you mean encode process > need validity checks or there need to check the reference id of each frame? > > And this patch will act on decode process, all references have already been > appointed by the stream. No. As said before, the *decode* process needs such checks. But since you don't seem to understand, let me try to be more helpful. If you have an array of references, and we refer to that as s->s.refs[8], in which some reference is missing, e.g. s->s.refs[5] is NULL (but the rest is fine). Now let's also say that we have a header in s->s.h in which there is an array of reference indices s->s.h.refidx[7] assigned as per the "spec". Let's imagine that we encounter a frame in which s->s.refs[5] is used as an active reference in this frame, for example s->s.h.refidx[0] = 5. Right now, with the code in git/master, we abort decoding. Your patch will make it continue decoding. So now, imagine that we encounter, in this frame, an inter block in which we use this reference (so b->ref[0] = 0, which means s->s.h.refidx[b->ref[0]] = 5), and have prediction code that does something like vp9_mc_template.c line 39: ThreadFrame *tref1 = &s->s.refs[s->s.h.refidx[b->ref[0]]]; Then later on this code will call a function which may in some cases be called mc_luma_dir() as per vp9_mc_template.c line 413, which is implemented in vp9recon.c line 298 in the function called mc_luma_unscaled() (see #define on line 383 of same file). This function now calls a DSP function in line 331: mc[!!mx][!!my](dst, dst_stride, ref, ref_stride, bh, mx << 1, my << 1); which directly accesses the reference pixels (see e.g. vp9dsp_template.c line 2044). Note how especially *none of these functions* check anywhere whether s->s.refs[s->s.h.refidx[b->ref[0]]] (or tref1) is NULL. They don't do that, because ... the header check already did that, so the check was redundant. But! You are *removing* that header check, so in this brave new world which will exist after applying your patch, what will happen is that I can craft a special stream where s->s.refs[5] becomes NULL, then send a subsequent frame using refs[5] by having s->s.h.refidx[0] = 5, then have a frame in which the block uses inter coding and uses reference 0 so that this causes access into s->s.refs[s->s.h.refidx[b->ref[0]]]], which is a NULL pointer, which is undefined behaviour by the C standard, which means a bad person could craft such a stream that would allow this bad person to break into your computer and steal all your data. In more straightforward cases, it might also crash Firefox and VLC, which use the FFmpeg VP9 decoder. Note also that having your data stolen or your application crashing is considered *bad*. We *don't want that*. Therefore, as I've tried to say a few times already, if you remove the header check, you need to add a per-block check instead, so that Firefox/VLC don't crash and so that bad persons cannot steal your data. Please add such a check and test using a fuzzer that the check prevents crashes as described above. Similar checks may be needed in other places also, since there's multiple places where the software decoder does MC and accesses references. Good luck finding these places by reading the code and fuzzing away. HTH, and please ask more questions (on IRC please, #ffmpeg-devel on Freenode) if this is still unclear. Ronald
On Thu, 2019-05-23 at 08:12 -0400, Ronald S. Bultje wrote:
Hi guys,
On Wed, May 22, 2019 at 11:14 PM 严小复 <mryancen@gmail.com<mailto:mryancen@gmail.com>> wrote:
code”, I'm little confused about the red word,would you mean encode process
need validity checks or there need to check the reference id of each frame?
And this patch will act on decode process, all references have already been
appointed by the stream.
No. As said before, the *decode* process needs such checks.
But since you don't seem to understand, let me try to be more helpful.
If you have an array of references, and we refer to that as s->s.refs[8], in which some reference is missing, e.g. s->s.refs[5] is NULL (but the rest is fine).
Do you mean s->s.refs[5].f ? s->s.refs[5] is not a pointer.
Now let's also say that we have a header in s->s.h in which there is an array of reference indices s->s.h.refidx[7] assigned as per the "spec". Let's imagine that we encounter a frame in which s->s.refs[5] is used as an active reference in this frame, for example s->s.h.refidx[0] = 5. Right now, with the code in git/master, we abort decoding. Your patch will make it continue decoding.
If so, even without Cen's patch, there is still a similar issue because the reference is used without any check in decode_frame_header () in vp9.c line 794-795
AVFrame *ref = s->s.refs[s->s.h.refidx[i]].f;
int refw = ref->width, refh = ref->height;
So I presumed s->s.refs[s->s.h.refidx[i]].f is assigned somewhere, otherwise we must add a check here.
So now, imagine that we encounter, in this frame, an inter block in which we use this reference (so b->ref[0] = 0, which means s->s.h.refidx[b->ref[0]] = 5), and have prediction code that does something like vp9_mc_template.c line 39:
ThreadFrame *tref1 = &s->s.refs[s->s.h.refidx[b->ref[0]]];
Then later on this code will call a function which may in some cases be called mc_luma_dir() as per vp9_mc_template.c line 413, which is implemented in vp9recon.c line 298 in the function called mc_luma_unscaled() (see #define on line 383 of same file). This function now calls a DSP function in line 331:
mc[!!mx][!!my](dst, dst_stride, ref, ref_stride, bh, mx << 1, my << 1);
which directly accesses the reference pixels (see e.g. vp9dsp_template.c line 2044).
Note how especially *none of these functions* check anywhere whether s->s.refs[s->s.h.refidx[b->ref[0]]] (or tref1) is NULL. They don't do that, because ... the header check already did that, so the check was redundant.
But! You are *removing* that header check, so in this brave new world which will exist after applying your patch, what will happen is that I can craft a special stream where s->s.refs[5] becomes NULL, then send a subsequent frame using refs[5] by having s->s.h.refidx[0] = 5, then have a frame in which the block uses inter coding and uses reference 0 so that this causes access into s->s.refs[s->s.h.refidx[b->ref[0]]]], which is a NULL pointer, which is undefined behaviour by the C standard, which means a bad person could craft such a stream that would allow this bad person to break into your computer and steal all your data. In more straightforward cases, it might also crash Firefox and VLC, which use the FFmpeg VP9 decoder.
Note also that having your data stolen or your application crashing is considered *bad*. We *don't want that*. Therefore, as I've tried to say a few times already, if you remove the header check, you need to add a per-block check instead, so that Firefox/VLC don't crash and so that bad persons cannot steal your data.
Please add such a check and test using a fuzzer that the check prevents crashes as described above. Similar checks may be needed in other places also, since there's multiple places where the software decoder does MC and accesses references. Good luck finding these places by reading the code and fuzzing away.
HTH, and please ask more questions (on IRC please, #ffmpeg-devel on Freenode) if this is still unclear.
Ronald
On Mon, 2019-05-27 at 04:44 +0000, Xiang, Haihao wrote: > On Thu, 2019-05-23 at 08:12 -0400, Ronald S. Bultje wrote: > Hi guys, > > On Wed, May 22, 2019 at 11:14 PM 严小复 <mryancen@gmail.com<mailto: > mryancen@gmail.com>> wrote: > code”, I'm little confused about the red word,would you mean encode process > need validity checks or there need to check the reference id of each frame? > > And this patch will act on decode process, all references have already been > appointed by the stream. > > No. As said before, the *decode* process needs such checks. > > But since you don't seem to understand, let me try to be more helpful. > > If you have an array of references, and we refer to that as s->s.refs[8], in > which some reference is missing, e.g. s->s.refs[5] is NULL (but the rest is > fine). > Do you mean s->s.refs[5].f ? s->s.refs[5] is not a pointer. > > Now let's also say that we have a header in s->s.h in which there is an array > of reference indices s->s.h.refidx[7] assigned as per the "spec". Let's > imagine that we encounter a frame in which s->s.refs[5] is used as an active > reference in this frame, for example s->s.h.refidx[0] = 5. Right now, with the > code in git/master, we abort decoding. Your patch will make it continue > decoding. > If so, even without Cen's patch, there is still a similar issue because the reference is used without any check in decode_frame_header () in vp9.c line 794- 795 AVFrame *ref = s->s.refs[s->s.h.refidx[i]].f; int refw = ref->width, refh = ref->height; So I presumed s->s.refs[s->s.h.refidx[i]].f is assigned somewhere, otherwise we must add a check here. > > > So now, imagine that we encounter, in this frame, an inter block in which we > use this reference (so b->ref[0] = 0, which means s->s.h.refidx[b->ref[0]] = > 5), and have prediction code that does something like vp9_mc_template.c line > 39: > > ThreadFrame *tref1 = &s->s.refs[s->s.h.refidx[b->ref[0]]]; > > Then later on this code will call a function which may in some cases be called > mc_luma_dir() as per vp9_mc_template.c line 413, which is implemented in > vp9recon.c line 298 in the function called mc_luma_unscaled() (see #define on > line 383 of same file). This function now calls a DSP function in line 331: > > mc[!!mx][!!my](dst, dst_stride, ref, ref_stride, bh, mx << 1, my << 1); > > which directly accesses the reference pixels (see e.g. vp9dsp_template.c line > 2044). > > Note how especially *none of these functions* check anywhere whether s- > >s.refs[s->s.h.refidx[b->ref[0]]] (or tref1) is NULL. They don't do that, > because ... the header check already did that, so the check was redundant. > > But! You are *removing* that header check, so in this brave new world which > will exist after applying your patch, what will happen is that I can craft a > special stream where s->s.refs[5] becomes NULL, then send a subsequent frame > using refs[5] by having s->s.h.refidx[0] = 5, then have a frame in which the > block uses inter coding and uses reference 0 so that this causes access into > s->s.refs[s->s.h.refidx[b->ref[0]]]], which is a NULL pointer, which is > undefined behaviour by the C standard, which means a bad person could craft > such a stream that would allow this bad person to break into your computer and > steal all your data. In more straightforward cases, it might also crash > Firefox and VLC, which use the FFmpeg VP9 decoder. > > Note also that having your data stolen or your application crashing is > considered *bad*. We *don't want that*. Therefore, as I've tried to say a few > times already, if you remove the header check, you need to add a per-block > check instead, so that Firefox/VLC don't crash and so that bad persons cannot > steal your data. > > Please add such a check and test using a fuzzer that the check prevents > crashes as described above. Similar checks may be needed in other places also, > since there's multiple places where the software decoder does MC and accesses > references. Good luck finding these places by reading the code and fuzzing > away. > > HTH, and please ask more questions (on IRC please, #ffmpeg-devel on Freenode) > if this is still unclear. > Ronald > _______________________________________________ > 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".
Please ignore this email because my comment was mixed with yours. Thanks Haihao > On Thu, 2019-05-23 at 08:12 -0400, Ronald S. Bultje wrote: > Hi guys, > > On Wed, May 22, 2019 at 11:14 PM 严小复 <mryancen@gmail.com<mailto: > mryancen@gmail.com>> wrote: > code”, I'm little confused about the red word,would you mean encode process > need validity checks or there need to check the reference id of each frame? > > And this patch will act on decode process, all references have already been > appointed by the stream. > > No. As said before, the *decode* process needs such checks. > > But since you don't seem to understand, let me try to be more helpful. > > If you have an array of references, and we refer to that as s->s.refs[8], in > which some reference is missing, e.g. s->s.refs[5] is NULL (but the rest is > fine). > > Do you mean s->s.refs[5].f ? s->s.refs[5] is not a pointer. > > Now let's also say that we have a header in s->s.h in which there is an array > of reference indices s->s.h.refidx[7] assigned as per the "spec". Let's > imagine that we encounter a frame in which s->s.refs[5] is used as an active > reference in this frame, for example s->s.h.refidx[0] = 5. Right now, with the > code in git/master, we abort decoding. Your patch will make it continue > decoding. > > If so, even without Cen's patch, there is still a similar issue because the > reference is used without any check in decode_frame_header () in vp9.c line > 794-795 > > AVFrame *ref = s->s.refs[s->s.h.refidx[i]].f; > int refw = ref->width, refh = ref->height; > > So I presumed s->s.refs[s->s.h.refidx[i]].f is assigned somewhere, otherwise > we must add a check here. > > > > So now, imagine that we encounter, in this frame, an inter block in which we > use this reference (so b->ref[0] = 0, which means s->s.h.refidx[b->ref[0]] = > 5), and have prediction code that does something like vp9_mc_template.c line > 39: > > ThreadFrame *tref1 = &s->s.refs[s->s.h.refidx[b->ref[0]]]; > > Then later on this code will call a function which may in some cases be called > mc_luma_dir() as per vp9_mc_template.c line 413, which is implemented in > vp9recon.c line 298 in the function called mc_luma_unscaled() (see #define on > line 383 of same file). This function now calls a DSP function in line 331: > > mc[!!mx][!!my](dst, dst_stride, ref, ref_stride, bh, mx << 1, my << 1); > > which directly accesses the reference pixels (see e.g. vp9dsp_template.c line > 2044). > > Note how especially *none of these functions* check anywhere whether s- > >s.refs[s->s.h.refidx[b->ref[0]]] (or tref1) is NULL. They don't do that, > because ... the header check already did that, so the check was redundant. > > But! You are *removing* that header check, so in this brave new world which > will exist after applying your patch, what will happen is that I can craft a > special stream where s->s.refs[5] becomes NULL, then send a subsequent frame > using refs[5] by having s->s.h.refidx[0] = 5, then have a frame in which the > block uses inter coding and uses reference 0 so that this causes access into > s->s.refs[s->s.h.refidx[b->ref[0]]]], which is a NULL pointer, which is > undefined behaviour by the C standard, which means a bad person could craft > such a stream that would allow this bad person to break into your computer and > steal all your data. In more straightforward cases, it might also crash > Firefox and VLC, which use the FFmpeg VP9 decoder. > > Note also that having your data stolen or your application crashing is > considered *bad*. We *don't want that*. Therefore, as I've tried to say a few > times already, if you remove the header check, you need to add a per-block > check instead, so that Firefox/VLC don't crash and so that bad persons cannot > steal your data. > > Please add such a check and test using a fuzzer that the check prevents > crashes as described above. Similar checks may be needed in other places also, > since there's multiple places where the software decoder does MC and accesses > references. Good luck finding these places by reading the code and fuzzing > away. > > HTH, and please ask more questions (on IRC please, #ffmpeg-devel on Freenode) > if this is still unclear. > Ronald > _______________________________________________ > 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".
Hi, RonaldHave you see the reply of Haihao？ He mentioned that decode_frame_header() has use s->s.refs[s->s.h.refidx[i]].f without safe check. For this suggestion, We must avoid the point on vp9.c 794-795.Could I restore the null point in decode_frame_header()？ Thanks,Cen -------- 原始信息 --------由： "Xiang, Haihao" <haihao.xiang@intel.com> 日期: 2019/5/27 12:53 (GMT+08:00) 收件人： rsbultje@gmail.com, ffmpeg-devel@ffmpeg.org 抄送： "Yan, CenX" <cenx.yan@intel.com> 主题： Re: [FFmpeg-devel] [PATCH] libavcodec/vp9: fix ref-frame size judging method On Mon, 2019-05-27 at 04:44 +0000, Xiang, Haihao wrote: > On Thu, 2019-05-23 at 08:12 -0400, Ronald S. Bultje wrote: > Hi guys, > > On Wed, May 22, 2019 at 11:14 PM 严小复 <mryancen@gmail.com<mailto: > mryancen@gmail.com>> wrote: > code”, I'm little confused about the red word,would you mean encode process > need validity checks or there need to check the reference id of each frame? > > And this patch will act on decode process, all references have already been > appointed by the stream. > > No. As said before, the *decode* process needs such checks. > > But since you don't seem to understand, let me try to be more helpful. > > If you have an array of references, and we refer to that as s->s.refs[8], in > which some reference is missing, e.g. s->s.refs[5] is NULL (but the rest is > fine). > Do you mean s->s.refs[5].f ? s->s.refs[5] is not a pointer. > > Now let's also say that we have a header in s->s.h in which there is an array > of reference indices s->s.h.refidx[7] assigned as per the "spec". Let's > imagine that we encounter a frame in which s->s.refs[5] is used as an active > reference in this frame, for example s->s.h.refidx[0] = 5. Right now, with the > code in git/master, we abort decoding. Your patch will make it continue > decoding. > If so, even without Cen's patch, there is still a similar issue because the reference is used without any check in decode_frame_header () in vp9.c line 794- 795 AVFrame *ref = s->s.refs[s->s.h.refidx[i]].f; int refw = ref->width, refh = ref->height; So I presumed s->s.refs[s->s.h.refidx[i]].f is assigned somewhere, otherwise we must add a check here. > > > So now, imagine that we encounter, in this frame, an inter block in which we > use this reference (so b->ref[0] = 0, which means s->s.h.refidx[b->ref[0]] = > 5), and have prediction code that does something like vp9_mc_template.c line > 39: > > ThreadFrame *tref1 = &s->s.refs[s->s.h.refidx[b->ref[0]]]; > > Then later on this code will call a function which may in some cases be called > mc_luma_dir() as per vp9_mc_template.c line 413, which is implemented in > vp9recon.c line 298 in the function called mc_luma_unscaled() (see #define on > line 383 of same file). This function now calls a DSP function in line 331: > > mc[!!mx][!!my](dst, dst_stride, ref, ref_stride, bh, mx << 1, my << 1); > > which directly accesses the reference pixels (see e.g. vp9dsp_template.c line > 2044). > > Note how especially *none of these functions* check anywhere whether s- > >s.refs[s->s.h.refidx[b->ref[0]]] (or tref1) is NULL. They don't do that, > because ... the header check already did that, so the check was redundant. > > But! You are *removing* that header check, so in this brave new world which > will exist after applying your patch, what will happen is that I can craft a > special stream where s->s.refs[5] becomes NULL, then send a subsequent frame > using refs[5] by having s->s.h.refidx[0] = 5, then have a frame in which the > block uses inter coding and uses reference 0 so that this causes access into > s->s.refs[s->s.h.refidx[b->ref[0]]]], which is a NULL pointer, which is > undefined behaviour by the C standard, which means a bad person could craft > such a stream that would allow this bad person to break into your computer and > steal all your data. In more straightforward cases, it might also crash > Firefox and VLC, which use the FFmpeg VP9 decoder. > > Note also that having your data stolen or your application crashing is > considered *bad*. We *don't want that*. Therefore, as I've tried to say a few > times already, if you remove the header check, you need to add a per-block > check instead, so that Firefox/VLC don't crash and so that bad persons cannot > steal your data. > > Please add such a check and test using a fuzzer that the check prevents > crashes as described above. Similar checks may be needed in other places also, > since there's multiple places where the software decoder does MC and accesses > references. Good luck finding these places by reading the code and fuzzing > away. > > HTH, and please ask more questions (on IRC please, #ffmpeg-devel on Freenode) > if this is still unclear. > Ronald > _______________________________________________ > 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/vp9.c b/libavcodec/vp9.c index acf3ffc..8dd14e0 100644 --- a/libavcodec/vp9.c +++ b/libavcodec/vp9.c @@ -790,6 +790,7 @@ static int decode_frame_header(AVCodecContext *avctx, /* check reference frames */ if (!s->s.h.keyframe && !s->s.h.intraonly) { + int has_valid_ref_frame = 0; for (i = 0; i < 3; i++) { AVFrame *ref = s->s.refs[s->s.h.refidx[i]].f; int refw = ref->width, refh = ref->height; @@ -802,12 +803,14 @@ static int decode_frame_header(AVCodecContext *avctx, return AVERROR_INVALIDDATA; } else if (refw == w && refh == h) { s->mvscale[i][0] = s->mvscale[i][1] = 0; + has_valid_ref_frame = 1; } else { - if (w * 2 < refw || h * 2 < refh || w > 16 * refw || h > 16 * refh) { - av_log(avctx, AV_LOG_ERROR, + int is_ref_frame_invalid = (w * 2 < refw || h * 2 < refh || w > 16 * refw || h > 16 * refh); + has_valid_ref_frame |= !is_ref_frame_invalid; + if (is_ref_frame_invalid) { + av_log(avctx, AV_LOG_WARNING, "Invalid ref frame dimensions %dx%d for frame size %dx%d\n", refw, refh, w, h); - return AVERROR_INVALIDDATA; } s->mvscale[i][0] = (refw << 14) / w; s->mvscale[i][1] = (refh << 14) / h; @@ -815,6 +818,11 @@ static int decode_frame_header(AVCodecContext *avctx, s->mvstep[i][1] = 16 * s->mvscale[i][1] >> 14; } } + if (!has_valid_ref_frame) { + av_log(avctx, AV_LOG_ERROR, + "Referenced frame has invalid size\n"); + return AVERROR_INVALIDDATA; + } } if (s->s.h.keyframe || s->s.h.errorres || (s->s.h.intraonly && s->s.h.resetctx == 3)) {