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

[Michael Niedermayer]

On Sun, Aug 23, 2009 at 06:50:09PM -0300, Ramiro Polla wrote:
> On Sun, Aug 23, 2009 at 6:02 PM, Michael Niedermayer<michaelni at gmx.at> wrote:
> > On Sun, Aug 23, 2009 at 05:48:28PM -0300, Ramiro Polla wrote:
> >> On Sun, Aug 23, 2009 at 11:37 AM, Michael Niedermayer<michaelni at gmx.at> wrote:
> >> > 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
> >>
> >> hmm, I never liked those checked malloc macros, I find them ugly (and
> >> there's the "people will have to look up the macro" issue). But if you
> >> prefer the macro, that's ok...
> >
> > well, i thought MALLOC_OR_GOTO_ERROR() was so clear noone would need
> > to look it up?
> >
> >
> >>
> >> > we do have somethig similar in mpegvideo?.c
> >>
> >> CHECKED_ALLOCZ from lavu/internal.h
> >>
> >> Should I add CHECKED_ALLOC and CHECKED_ALLOCZ macros to
> >> swscale_internal and use them (or try to reuse from lavu/internal.h) ?
> >
> > you cant use lavu/internal.h directly but you can use
> > libavutil/avutil.h
> 
> 2 patches attached.
> 

> If you want I can send a patch renaming CHECKED_ALLOC(Z) to
> MALLOC(Z)_OR_GOTO_ERROR.

dunno, iam slightly in favor though maybe others have some better ideas
or oppions?


>  internal.h |    9 +++++++++
>  1 file changed, 9 insertions(+)
> a6445117d941414b61ecd5993afbda22f31902cb  checked_alloc.diff

ok

[...]

> @@ -1737,7 +1738,7 @@
>  
>      // Note the +1 is for the MMX scaler which reads over the end
>      /* align at 16 for AltiVec (needed by hScale_altivec_real) */
> -    *outFilter= av_mallocz(*outFilterSize*(dstW+1)*sizeof(int16_t));
> +    CHECKED_ALLOCZ(*outFilter, *outFilterSize*(dstW+1)*sizeof(int16_t));
>  
>      /* normalize & store in outFilter */
>      for (i=0; i<dstW; i++) {
> @@ -1764,7 +1765,7 @@
>      }
>  
>      ret=0;
> -error:
> +fail:
>      av_free(filter);
>      av_free(filter2);
>      return ret;

i doubt, this with the lack of checks of the return value wil work

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
-------------- 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/20090824/757e50df/attachment.pgp>