[Ffmpeg-devel] Frank patch to h264.c for robustness

[Michael Niedermayer]

Hi

On Tue, Dec 05, 2006 at 01:24:53PM -0500, Francois Oligny-Lemieux wrote:
> Hello,
> 
> I do not have SVN account so I cannot commit my changes.
> I have made a patch with TortoiseSVN
> The modications are to prevent real crashes that
> have happened when decoding from a live 720x480 encoder.
> 
> Some crashes are due to network packet lost, packet dropped and sometime
> sending trash data by mistake on the decoder (that one resulted in an
> illegal sps_id)
> Anyway I hope the patch is welcome.
> 
> Francois O.L.

[...]
> @@ -1789,6 +1793,10 @@
>      si=di=0;
>      while(si<length){
>          //remove escapes (very rare 1:2^22)
> +        if ( dst == NULL ) // this check added by frank 15.Aug.2006
> +	  {  
> +	       length=0;
> +        }

this is a ugly hack and totally wrong, the only way to have dst=0
is if you run out of memory, and in that case that must be checked after
the realloc not at a random point later and the only thing to do in that
case is fail no random change to a random variable
also tabs are forbidden in svn
and do not put a comment that you modified a specific part to every
modification and keep the {} placement style like it is in the surrounding
code


[...]

> @@ -7792,7 +7807,14 @@
>      get_bits(&s->gb, 4); // reserved
>      level_idc= get_bits(&s->gb, 8);
>      sps_id= get_ue_golomb(&s->gb);
> -
> +    
> +    // frank: here we have crashes sometimes, 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;
> +    }

sps_id cannot be < 0


[...]
> @@ -8117,6 +8139,12 @@
>  
>          buf_index+=3;
>        }
> +      
> +        if ( h->is_avc && nalsize > 1000000 ) 
> +        {	
> +            // frank prevent problem when seeking in a h264 mov file. 
> +            return -1;
> +        }

whatever this is supposed to do it is wrong, nalsize can very well be
arbitrary large

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you really think that XML is the answer, then you definitly missunderstood
the question -- Attila Kinali