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

[Ramiro Polla]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: checked_alloc.diff
Type: text/x-diff
Size: 519 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090823/8f1539b8/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: malloc_error.diff
Type: text/x-diff
Size: 7804 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090823/8f1539b8/attachment-0001.diff>