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 严小复 <mryan...@gmail.com<mailto: > mryan...@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-requ...@ffmpeg.org with subject "unsubscribe". _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
