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

[Ramiro Polla]

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...

> 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) ?