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

Michael Niedermayer michaelni
Sun Aug 23 23:02:11 CEST 2009


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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato
-------------- 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/94d59056/attachment.pgp>



More information about the ffmpeg-devel mailing list