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

[Ramiro Polla]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: check_malloc.diff
Type: text/x-diff
Size: 6044 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090822/69dc7ae0/attachment.diff>