[FFmpeg-devel] [PATCH] pnm parser crashes on corrupted frames

[Howard Chu]

Michael Niedermayer wrote:
> On Sat, Mar 27, 2010 at 02:45:11PM -0700, Howard Chu wrote:
>> However, your statement is quite inconsistent; there is already a check for
>> avctx->width on the preceding lines. I was simply adding the equally
>> necessary check on height:
>>
>>>>>>
>>      pnm_get(s, buf1, sizeof(buf1));
>>      avctx->width = atoi(buf1);
>>      if (avctx->width<= 0)
>>          return -1;
>>      pnm_get(s, buf1, sizeof(buf1));
>>      avctx->height = atoi(buf1);
>>      if (avctx->height<= 0)
>>          return -1;
>>      if(avcodec_check_dimensions(avctx, avctx->width, avctx->height))
>>          return -1;
>> <<<<
>>
>> Looking at the code in avcodec_check_dimensions, it may be that there's no
>> actual crash, this patch just avoids the log message that
>> avcodec_check_dimensions would spit out. Again, from a consistency
>> perspective, if the width can be ignored silently, then so should the
>> height. (And as I think about it, I was probably getting a ton of those log
>> messages before.)
>
> i guess the width check is there to prevent the pnm_get()
> this does not apply to height as avcodec_check_dimensions checks
> it completely alraedy

Why does the pnm_get() for height need to be protected, but not the pnm_get() 
for width? Considering that either item might be missing/corrupted, that makes 
no sense.

Of course, it's not my code and it doesn't need to make sense to me. If you 
think it's fine as-is, fine, I'll drop the matter. It's too trivial to spend 
much time on. I'll just keep carrying this patch in my own tree.
-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/