Hi
On Wed, Jan 17, 2007 at 02:22:07PM -0500, Frank wrote:
> Attached is a patch for h264.c which are modifications I use to prevent
> crashes. I mean all modified lines are where I had a crash and debugged to
> find out where it happened, There are probably other places where index are
> applied to array which could result in overflow (or negative array index). I
> read the post about fuzzer bugs (zzuf application) posted January 15th 2007
> and it looks like decoders are really sensible to corrupted data and it
> convinced me to re-post my patch and also mention it would be a good idea to
> increase array index verifications in h264.c (from my point of view of
> course)
>
> There is one crash which is due to sps_id being negative. I submited this
> fix several weeks ago and it was rejected because apparently sps_id cannot
> be negative. To reply to that I would say from a programming point of view
> it is an "int" and when the 32bit value from the byte stream is bigger than
> INT_MAX, it goes negative. Unless get_bits() shave bits to INT_MAX ?
sps_id should then be changed to unsigned int
>
> Some other are just verifying NULL pointer or index of an array. For example
> checking the return value of remove_short() which returns negative on
> failure and the return value was used as an array index right after.
>
> I have also included 3 comments where it crashed one of which I don't know
> how it can easily be fixed (line 4195 on pic being invalid pointer). Please
> remove the crash comments if you don't like them.(I have attached a patch
> without them)
ive no problem with the comments but they should be in a seperate patch as
its a seperate thing (fix crashes vs. comments about crashes which havnt been
fixed)
of course comments like
if(x==NULL) //added protection
are not ok as they are useless
>
> Anyway I hope this tiny patch is welcome, Please don't think I'm criticizing
> h264.c, It is great and allow me and lots of other people to undergo
> interesting projects. Thanks.
patches are always welcome, note disscussuions about them should stay on
ffmpeg-dev
[...]
> @@ -1400,6 +1400,11 @@
> const int is_b8x8 = IS_8X8(*mb_type);
> int sub_mb_type;
> int i8, i4;
> +
> + if ( h->ref_list[1][0].mb_type == 0 ){
> + //fixes a crash
> + return;
> + }
as this is dereferenced a few lines above, this change of course is not
correct
also a simple return is no solution for a missing reference frame, possible
solutions would be to either drop b frames which refer to missing frames, or
to somehow change the reference and motion vectors to some guess simply
returning randomly is no solution
>
> #define MB_TYPE_16x16_OR_INTRA (MB_TYPE_16x16|MB_TYPE_INTRA4x4|MB_TYPE_INTRA16x16|MB_TYPE_INTRA_PCM)
> if(IS_8X8(mb_type_col) && !h->sps.direct_8x8_inference_flag){
> @@ -1779,6 +1784,10 @@
>
> h->rbsp_buffer= av_fast_realloc(h->rbsp_buffer, &h->rbsp_buffer_size, length);
> dst= h->rbsp_buffer;
> +
> + if ( dst == NULL ){ // i do length=0 but anyone can modify this to an appropriate return
> + length=0;
> + }
well you have to change this to a appropriate return before it can be applied
we never accept code which is half wrong
>
> //printf("decoding esc\n");
> si=di=0;
> @@ -3949,11 +3958,17 @@
> ref->pic_id= ref->frame_num;
> }else{
> pic_id= get_ue_golomb(&s->gb); //long_term_pic_idx
> - ref = h->long_ref[pic_id];
> - ref->pic_id= pic_id;
> - assert(ref->reference == 3);
> - assert(ref->long_ref);
> - i=0;
> + ref = h->long_ref[pic_id];
> + if ( ref ){//added protection
> + ref->pic_id= pic_id;
> + assert(ref->reference == 3);
> + assert(ref->long_ref);
> + i=0;
> + }
> + else
> + {
> + i=-1;
> + }
the {} placement doesnt match the existing code
[...]
> @@ -4259,8 +4275,10 @@
> if(pic) unreference_pic(h, pic);
>
> h->long_ref[ mmco[i].long_index ]= remove_short(h, mmco[i].short_frame_num);
> - h->long_ref[ mmco[i].long_index ]->long_ref=1;
> - h->long_ref_count++;
> + if ( h->long_ref[ mmco[i].long_index ] ){ // added protection
> + h->long_ref[ mmco[i].long_index ]->long_ref=1;
> + h->long_ref_count++;
> + }
this is indented by 3 spaces while 4 is used everywhere else in ffmpeg
also the comment is not usefull outside this patch -> it should not be here
[...]
> @@ -7683,6 +7701,13 @@
> get_bits(&s->gb, 4); // reserved
> level_idc= get_bits(&s->gb, 8);
> sps_id= get_ue_golomb(&s->gb);
> +
> + // if we get the wrong sps_id, we get a wrong sps pointer ! ouch.
> + if ( sps_id < 0 || sps_id >= MAX_SPS_COUNT ){
> + // ok it has gone out of hand, someone is sending us bad stuff.
> + av_log(h->s.avctx, AV_LOG_ERROR, "illegal sps_id (%d)\n", sps_id);
> + return -1;
> + }
a sps_id >= MAX_SPS_COUNT check with unsigned int sps_id is ok the sps_id<0
is not
>
> sps= &h->sps_buffer[ sps_id ];
> sps->profile_idc= profile_idc;
> @@ -8009,6 +8034,11 @@
> buf_index+=3;
> }
>
> + if ( h->is_avc && nalsize > 7100000 ){ // FIXME add a max_nal_size
> + // added when problem seeking in a h264 mov file.
> + return -1;
> + }
why?
> +
> ptr= decode_nal(h, buf + buf_index, &dst_length, &consumed, h->is_avc ? nalsize : buf_size - buf_index);
> while(ptr[dst_length - 1] == 0 && dst_length > 1)
> dst_length--;
> @@ -8122,7 +8152,7 @@
> execute_ref_pic_marking(h, h->mmco, h->mmco_index);
>
> ff_er_frame_end(s);
> -
> +
> MPV_frame_end(s);
cosmetic change
[...]
> @@ -8530,7 +8561,8 @@
> decode_end,
> decode_frame,
> /*CODEC_CAP_DRAW_HORIZ_BAND |*/ CODEC_CAP_DR1 | CODEC_CAP_TRUNCATED | CODEC_CAP_DELAY,
> - .flush= flush_dpb,
> + NULL, //next
> + /*.flush=*/ flush_dpb
unrelated and not ok
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
No evil is honorable: but death is honorable; therefore death is not evil.
-- Citium Zeno
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070118/4deb7de9/attachment.pgp>
RetroSearch is an open source project built by @garambo | Open a GitHub Issue
Search and Browse the WWW like it's 1997 | Search results from DuckDuckGo
HTML:
3.2
| Encoding:
UTF-8
| Version:
0.7.4