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

[Ramiro Polla]

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.

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.

Ramiro Polla
-------------- next part --------------
A non-text attachment was scrubbed...
Name: malloc_error_2.diff
Type: text/x-diff
Size: 9139 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090814/31784736/attachment.diff>