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

[Reimar Döffinger]

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.

> 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.

A somewhat strange conclusion, deciding to keep a patch for which you can't
even tell us what exactly it is for...