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

Baptiste Coudurier baptiste.coudurier
Mon Mar 29 20:21:15 CEST 2010


Hi,

On 03/28/2010 01:28 PM, Howard Chu wrote:
> 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 )
>

It seems to me that you don't care much about what I explained in the 
bug tracker:

On 01/19/2010 06:43 AM, Howard Chu wrote:
 >
 > Howard Chu<hyc at symas.com>  added the comment:
 >
 > Looks like it's just a spurious message from the pnm parser, the
 > parser will correctly rebuffer and retry when it gets an incomplete
 > record. This trivial patch shuts up the error message:

check_dimensions is here to check height. I think the correct fix is to
make pnm_get return the number of bytes read and fail if it's < requested.

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org



More information about the ffmpeg-devel mailing list