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

[Howard Chu]

Reimar D?ffinger wrote:
> On Sun, Mar 28, 2010 at 12:13:24AM -0700, Howard Chu wrote:
>> 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.
>
> There's no pnm_get in-between reading height and avcodec_check_dimensions...
> However I don't see the point of the first check, particularly since it hinders
> printing a possibly helpful error message.
> The only issue I can see is that the "skip whitespace" code does not fully check
> for reaching the end of the input stream.
> And a real-world sample file would still be very useful.

Unfortunately I don't have a sample file. The stream of samples was produced 
by gource; they were generated on the fly and written through a pipe to 
ffmpeg. The deeper problem is why was gource producing corrupted frames, but 
that's not for this forum. If you feel like it's worth chasing down to produce 
a sample, you could run gource on your own source repo to make a movie. (I 
used the OpenLDAP repo. http://www.youtube.com/watch?v=4sClZGLZ6dY )


-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/