[FFmpeg-devel] [PATCH] Check malloc values in swscale.

[Michael Niedermayer]

On Sat, Aug 22, 2009 at 01:34:20PM -0300, Ramiro Polla wrote:
> On Fri, Aug 14, 2009 at 11:34 PM, Ramiro Polla<ramiro.polla at gmail.com> wrote:
> > On Fri, Aug 14, 2009 at 4:40 AM, Reimar
> > D?ffinger<Reimar.Doeffinger at gmx.de> wrote:
> >> On Thu, Aug 13, 2009 at 10:11:45PM -0300, Ramiro Polla wrote:
> >>> Am I being overparanoid cleaning up all previously allocated blocks
> >>> before leaving the function, like in:
> >>> ? ? ? ? ?for (i=0; i<c->vLumBufSize; i++)
> >>> + ? ? ? ?{
> >>> ? ? ? ? ? ? ?c->alpPixBuf[i]= c->alpPixBuf[i+c->vLumBufSize]= av_mallocz(VOF+1);
> >>> + ? ? ? ? ? ?if (!c->alpPixBuf[i]) {
> >>> + ? ? ? ? ? ? ? ?for (; i >= 0; i--)
> >>> + ? ? ? ? ? ? ? ? ? ?av_free(c->alpPixBuf[i]);
> >>> + ? ? ? ? ? ? ? ?return NULL;
> >>> + ? ? ? ? ? ?}
> >>> + ? ? ? ?}
> >>>
> >>> Is ok to just return NULL and leave those previous blocks there?
> >>
> >> You should free them, but
> >>
> >>> @@ -2905,18 +2939,47 @@ SwsContext *sws_getContext(int srcW, int srcH, enum PixelFormat srcFormat, int d
> >>>
> >>> ? ? ?// allocate pixbufs (we use dynamic allocation because otherwise we would need to
> >>> ? ? ?c->lumPixBuf= av_malloc(c->vLumBufSize*2*sizeof(int16_t*));
> >>> + ? ?if (!c->lumPixBuf)
> >>> + ? ? ? ?return NULL;
> >>> ? ? ?c->chrPixBuf= av_malloc(c->vChrBufSize*2*sizeof(int16_t*));
> >>> + ? ?if (!c->chrPixBuf)
> >>> + ? ? ? ?return NULL;
> >>> ? ? ?if (CONFIG_SWSCALE_ALPHA && isALPHA(c->srcFormat) && isALPHA(c->dstFormat))
> >>> + ? ?{
> >>> ? ? ? ? ?c->alpPixBuf= av_malloc(c->vLumBufSize*2*sizeof(int16_t*));
> >>> + ? ? ? ?if (!c->alpPixBuf)
> >>> + ? ? ? ? ? ?return NULL;
> >>> + ? ?}
> >>> ? ? ?//Note we need at least one pixel more at the end because of the MMX code (just in case someone wanna replace the 4000/8000)
> >>> ? ? ?/* align at 16 bytes for AltiVec */
> >>> ? ? ?for (i=0; i<c->vLumBufSize; i++)
> >>> + ? ?{
> >>> ? ? ? ? ?c->lumPixBuf[i]= c->lumPixBuf[i+c->vLumBufSize]= av_mallocz(VOF+1);
> >>> + ? ? ? ?if (!c->lumPixBuf[i]) {
> >>> + ? ? ? ? ? ?for (; i >= 0; i--)
> >>> + ? ? ? ? ? ? ? ?av_free(c->lumPixBuf[i]);
> >>> + ? ? ? ? ? ?return NULL;
> >>> + ? ? ? ?}
> >>> + ? ?}
> >>
> >> It seems a bit pointless if you go only half the way and don't free
> >> lumPixBuf etc.
> >> That kind of thing is why so often goto err_out or so is used.
> >> Also you should use av_freep everywhere (and check if you can make it so
> >> you can just call the normal uninit function instead of adding a lot of
> >> code to free).
> >
> > Hmmm, right. Using sws_freeContext() makes much more sense. New patch attached.
> 
> New patch with remaining allocs.
> 
> > And regarding Kostya's comment:
> >> Diego will kill you for such opening brace placement.
> >
> > I don't really like to change a line just for {}. I'll do it in a
> > subsequent commit.
> 
> Same comment applies to this patch.

>  colorspace-test.c |    3 +++
>  swscale-example.c |    3 +++
>  swscale.c         |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 55 insertions(+)
> aa069dc8d01ac938442201de25cdcec97b99486d  check_malloc.diff
> Index: swscale.c
> ===================================================================
> --- swscale.c	(revision 29544)
> +++ swscale.c	(working copy)
> @@ -1451,11 +1451,15 @@
>  
>      // NOTE: the +1 is for the MMX scaler which reads over the end
>      *filterPos = av_malloc((dstW+1)*sizeof(int16_t));
> +    if (!*filterPos)
> +        goto error;
>  
>      if (FFABS(xInc - 0x10000) <10) { // unscaled
>          int i;
>          filterSize= 1;
>          filter= av_mallocz(dstW*sizeof(*filter)*filterSize);
> +        if (!filter)
> +            goto error;
>  

iam not a friend of macros but as there are a quite a few
may a MALLOC_OR_GOTO_ERROR() could be added ?
of course unless ther is a cleaner solution

we do have somethig similar in mpegvideo?.c

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090823/4430e073/attachment.pgp>