[Ffmpeg-devel] h264, protection against corrupted data (second try patch)

[Michael Niedermayer]

Hi

On Thu, Jan 18, 2007 at 12:48:39PM -0500, Frank wrote:
> Hello
> 
> On 1/17/07, Michael Niedermayer <michaelni at gmx.at> wrote:
> >
> >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
> 
> 
> done.
> I've also changed pps_id to unsigned int while at it.
> There was one issue where a pps_id was assigned to an int so I did:
> int dequant_coeff_pps = (int) pps_id;
> which I believe is safe since pps_id is validated as being lower than 256
> when assigned and if uninitialized will be zero.
> The cast is compile-time difference and doesn't add run-time convertion.
> Anyway I don't see a lot of these cast in ffmpeg so this is why I mention it
> (in case it is not ok).

yes, we dont like such casts they make code unreadable


[...]
> +    

trailling whitespace isnt allowed in svn


[...]

> -        while(ptr[dst_length - 1] == 0 && dst_length > 1)
[...]
> +        while( ptr[dst_length - 1] == 0 && dst_length > 1 )

cosmetic change

[...]


except these the patch looks ok

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- 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/20070119/b8cbe016/attachment.pgp>