diff mbox

[FFmpeg-devel,v3] libavcodec/vp9: fix ref-frame size judging method

Message ID 1562623051-7944-1-git-send-email-mryancen@gmail.com
State New
Headers show

Commit Message

Yan Cen July 8, 2019, 9:57 p.m. UTC 
From: yancen <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: Xiang Haihao <haihao.xiang@intel.com>
Signed-off-by: Li Zhong <zhong.li@intel.com>
Signed-off-by: Yan Cen <cenx.yan@intel.com>
---
 libavcodec/vp9.c | 51 +++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 43 insertions(+), 8 deletions(-)

Comments

Ronald S. Bultje July 9, 2019, 11:22 a.m. UTC | #1 
Hi,

On Mon, Jul 8, 2019 at 6:23 PM Yan Cen <mryancen@gmail.com> wrote:

> From: yancen <cenx.yan@intel.com>
>
> There is no need all reference frame demension is valid in libvpx.
>

Haven't we discussed this before? Anyway, it seems you're really eager to
get this in, so I'll drop my objection. (I still think this could cause
issues in HW decoders.)

-            if (!s->s.refs[s->s.h.refidx[0]].f->buf[0] ||
> -                !s->s.refs[s->s.h.refidx[1]].f->buf[0] ||
> -                !s->s.refs[s->s.h.refidx[2]].f->buf[0]) {
> -                av_log(avctx, AV_LOG_ERROR, "Not all references are
> available\n");
> -                return AVERROR_INVALIDDATA;
> +            if (0 == sizeof(s->s.refs[s->s.h.refidx[0]])) {
> +                if (0 == sizeof(s->s.refs[s->s.h.refidx[1]].f->buf[0])) {
> +                    if (0 == s->s.refs[s->s.h.refidx[2]].f->buf[0]) {
> +                        av_log(avctx, AV_LOG_ERROR, "All references are
> unavailable\n");
> +                        return AVERROR_INVALIDDATA;
> +                    } else {
> +
> av_frame_copy(s->s.refs[s->s.h.refidx[1]].f,s->s.refs[s->s.h.refidx[2]].f);
> +
> av_frame_copy(s->s.refs[s->s.h.refidx[0]].f,s->s.refs[s->s.h.refidx[2]].f);
> +                    }
>
[..]

This is concealment code for missing references and is unrelated to the ref
frame size judgement patch. Could you please split this off in a separate
patch? Also, we don't use 0 == sizeof(..) or 0 == .. in ffmpeg, we just use
!.., please adjust that style.

Ronald
Paul B Mahol July 9, 2019, 11:38 a.m. UTC | #2 
On 7/9/19, Ronald S. Bultje <rsbultje@gmail.com> wrote:
> Hi,
>
> On Mon, Jul 8, 2019 at 6:23 PM Yan Cen <mryancen@gmail.com> wrote:
>
>> From: yancen <cenx.yan@intel.com>
>>
>> There is no need all reference frame demension is valid in libvpx.
>>
>
> Haven't we discussed this before? Anyway, it seems you're really eager to
> get this in, so I'll drop my objection. (I still think this could cause
> issues in HW decoders.)

Sorry but patch quality is unacceptable.

>
> -            if (!s->s.refs[s->s.h.refidx[0]].f->buf[0] ||
>> -                !s->s.refs[s->s.h.refidx[1]].f->buf[0] ||
>> -                !s->s.refs[s->s.h.refidx[2]].f->buf[0]) {
>> -                av_log(avctx, AV_LOG_ERROR, "Not all references are
>> available\n");
>> -                return AVERROR_INVALIDDATA;
>> +            if (0 == sizeof(s->s.refs[s->s.h.refidx[0]])) {
>> +                if (0 == sizeof(s->s.refs[s->s.h.refidx[1]].f->buf[0])) {
>> +                    if (0 == s->s.refs[s->s.h.refidx[2]].f->buf[0]) {
>> +                        av_log(avctx, AV_LOG_ERROR, "All references are
>> unavailable\n");
>> +                        return AVERROR_INVALIDDATA;
>> +                    } else {
>> +
>> av_frame_copy(s->s.refs[s->s.h.refidx[1]].f,s->s.refs[s->s.h.refidx[2]].f);
>> +
>> av_frame_copy(s->s.refs[s->s.h.refidx[0]].f,s->s.refs[s->s.h.refidx[2]].f);
>> +                    }
>>
> [..]
>
> This is concealment code for missing references and is unrelated to the ref
> frame size judgement patch. Could you please split this off in a separate
> patch? Also, we don't use 0 == sizeof(..) or 0 == .. in ffmpeg, we just use
> !.., please adjust that style.
>
> 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".
Ronald S. Bultje July 10, 2019, 1:56 p.m. UTC | #3 
Hi,

On Tue, Jul 9, 2019 at 7:22 AM Ronald S. Bultje <rsbultje@gmail.com> wrote:

> Hi,
>
> On Mon, Jul 8, 2019 at 6:23 PM Yan Cen <mryancen@gmail.com> wrote:
>
>> From: yancen <cenx.yan@intel.com>
>>
>> There is no need all reference frame demension is valid in libvpx.
>>
>
> Haven't we discussed this before? Anyway, it seems you're really eager to
> get this in, so I'll drop my objection. (I still think this could cause
> issues in HW decoders.)
>

I want to take this discussion one step further though. So, obviously, the
central goal of this patch is to allow streams that can be decoded with
libvpx to be decoded with ffvp9, specifically these that have frame size
vs. ref size discrepancies that go beyond the limits formally allowed. We
used to say that all refs have to comply with the limits, and you're
changing it so that only one ref has to comply with the limits. I
understand that so far.

Now, what does libvpx do with the "invalid" refs? And what do hardware
decoders do? What does the spec formally require? Is this mandated? Or
optional? I can imagine several things happening in libvpx:
- it just allows using invalid references like this;
- it conceals as if the ref were missing and uses another one;
- something else??

What does libvpx do, and what are we formally mandated to do, if anything?
Are these streams considered proper? If so, isn't this an hardware exploit?
Else, why have ref size limits at all?

Ronald
diff mbox

Patch

diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
index acf3ffc..58e7312 100644
--- a/libavcodec/vp9.c
+++ b/libavcodec/vp9.c
@@ -555,11 +555,37 @@  static int decode_frame_header(AVCodecContext *avctx,
             s->s.h.signbias[1]    = get_bits1(&s->gb) && !s->s.h.errorres;
             s->s.h.refidx[2]      = get_bits(&s->gb, 3);
             s->s.h.signbias[2]    = get_bits1(&s->gb) && !s->s.h.errorres;
-            if (!s->s.refs[s->s.h.refidx[0]].f->buf[0] ||
-                !s->s.refs[s->s.h.refidx[1]].f->buf[0] ||
-                !s->s.refs[s->s.h.refidx[2]].f->buf[0]) {
-                av_log(avctx, AV_LOG_ERROR, "Not all references are available\n");
-                return AVERROR_INVALIDDATA;
+            if (0 == sizeof(s->s.refs[s->s.h.refidx[0]])) {
+                if (0 == sizeof(s->s.refs[s->s.h.refidx[1]].f->buf[0])) {
+                    if (0 == s->s.refs[s->s.h.refidx[2]].f->buf[0]) {
+                        av_log(avctx, AV_LOG_ERROR, "All references are unavailable\n");
+                        return AVERROR_INVALIDDATA;
+                    } else {
+                        av_frame_copy(s->s.refs[s->s.h.refidx[1]].f,s->s.refs[s->s.h.refidx[2]].f);
+                        av_frame_copy(s->s.refs[s->s.h.refidx[0]].f,s->s.refs[s->s.h.refidx[2]].f);
+                    }
+                } else {
+                    if (0 == sizeof(s->s.refs[s->s.h.refidx[2]].f->buf[0])) {
+                        av_frame_copy(s->s.refs[s->s.h.refidx[0]].f,s->s.refs[s->s.h.refidx[1]].f);
+                        av_frame_copy(s->s.refs[s->s.h.refidx[2]].f,s->s.refs[s->s.h.refidx[1]].f);
+                    } else {
+                        av_frame_copy(s->s.refs[s->s.h.refidx[0]].f,s->s.refs[s->s.h.refidx[1]].f);
+                    }
+                }
+            } else {
+                if (0 == sizeof(s->s.refs[s->s.h.refidx[1]].f->buf[0])) {
+                    if (0 == sizeof(s->s.refs[s->s.h.refidx[2]].f->buf[0])) {
+                        s->s.refs[s->s.h.refidx[1]].f = s->s.refs[s->s.h.refidx[2]].f = s->s.refs[s->s.h.refidx[0]].f;
+                        av_frame_copy(s->s.refs[s->s.h.refidx[1]].f,s->s.refs[s->s.h.refidx[0]].f);
+                        av_frame_copy(s->s.refs[s->s.h.refidx[2]].f,s->s.refs[s->s.h.refidx[0]].f);
+                    } else {
+                        av_frame_copy(s->s.refs[s->s.h.refidx[1]].f,s->s.refs[s->s.h.refidx[0]].f);
+                    }
+                } else {
+                    if (0 == sizeof(s->s.refs[s->s.h.refidx[2]].f->buf[0])) {
+                        av_frame_copy(s->s.refs[s->s.h.refidx[2]].f,s->s.refs[s->s.h.refidx[1]].f);
+                    }
+                }
             }
             if (get_bits1(&s->gb)) {
                 w = s->s.refs[s->s.h.refidx[0]].f->width;
@@ -790,6 +816,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 +829,15 @@  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);
+                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;
+                } else {
+                    has_valid_ref_frame = 1;
                 }
                 s->mvscale[i][0] = (refw << 14) / w;
                 s->mvscale[i][1] = (refh << 14) / h;
@@ -815,6 +845,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)) {