[FFmpeg-devel,1/2] lavc/qsvenc: Fix the memory leak for enc_ctrl.Payload

Submitted by Linjie Fu on April 10, 2019, 11:27 a.m.

Details

Message ID 20190410112714.18995-1-linjie.fu@intel.com
State New
Headers show

Commit Message

Linjie Fu April 10, 2019, 11:27 a.m.
frame->enc_ctrl.Payload is malloced in get_free_frame, directly memset
the whole structure of enc_ctrl to zero will cause the memory leak for
enc_ctrl.Payload.

Fix the memory leak issue and reset other members in mfxEncodeCtrl.

Signed-off-by: Linjie Fu <linjie.fu@intel.com>
---
 libavcodec/qsvenc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Carl Eugen Hoyos April 10, 2019, 11:34 a.m.
2019-04-10 13:27 GMT+02:00, Linjie Fu <linjie.fu@intel.com>:
> frame->enc_ctrl.Payload is malloced in get_free_frame, directly memset
> the whole structure of enc_ctrl to zero will cause the memory leak for
> enc_ctrl.Payload.
>
> Fix the memory leak issue and reset other members in mfxEncodeCtrl.
>
> Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> ---
>  libavcodec/qsvenc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
> index 5aa020d47b..029bb562d6 100644
> --- a/libavcodec/qsvenc.c
> +++ b/libavcodec/qsvenc.c
> @@ -1254,7 +1254,7 @@ static int encode_frame(AVCodecContext *avctx,
> QSVEncContext *q,
>      if (qsv_frame) {
>          surf = &qsv_frame->surface;
>          enc_ctrl = &qsv_frame->enc_ctrl;
> -        memset(enc_ctrl, 0, sizeof(mfxEncodeCtrl));
> +        memset(enc_ctrl, 0, sizeof(mfxEncodeCtrl) - sizeof(mfxPayload **));

Am I correct that this assumes a particular order of
contents in a struct not defined within FFmpeg?
Maybe saving the pointer in a variable and writing
it back is more future proof?

Carl Eugen
Zhong Li April 11, 2019, 3:14 a.m.
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Linjie Fu

> Sent: Wednesday, April 10, 2019 7:27 PM

> To: ffmpeg-devel@ffmpeg.org

> Cc: Fu, Linjie <linjie.fu@intel.com>

> Subject: [FFmpeg-devel] [PATCH 1/2] lavc/qsvenc: Fix the memory leak for

> enc_ctrl.Payload

> 

> frame->enc_ctrl.Payload is malloced in get_free_frame, directly memset

> the whole structure of enc_ctrl to zero will cause the memory leak for

> enc_ctrl.Payload.

> 

> Fix the memory leak issue and reset other members in mfxEncodeCtrl.

> 

> Signed-off-by: Linjie Fu <linjie.fu@intel.com>

> ---

>  libavcodec/qsvenc.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index

> 5aa020d47b..029bb562d6 100644

> --- a/libavcodec/qsvenc.c

> +++ b/libavcodec/qsvenc.c

> @@ -1254,7 +1254,7 @@ static int encode_frame(AVCodecContext *avctx,

> QSVEncContext *q,

>      if (qsv_frame) {

>          surf = &qsv_frame->surface;

>          enc_ctrl = &qsv_frame->enc_ctrl;

> -        memset(enc_ctrl, 0, sizeof(mfxEncodeCtrl));

> +        memset(enc_ctrl, 0, sizeof(mfxEncodeCtrl) - sizeof(mfxPayload

> + **));


Looks a little tricky, if someone didn't read carefully the sequence of mfxPayload usages, they can't know why this change happening. 
And potential risk for other elements such as "mfxExtBuffer** ExtParam" is still existed. 

IMHO, one good habit is that is you want to memset a buffer, would better memset it when you allocate it. 
So I believe the correct fix should be: memset mfxEncodeCtrl at the start of qsv frame allocated: the place should be in the function get_free_frame()
Linjie Fu April 11, 2019, 6:50 a.m.
> -----Original Message-----

> From: Li, Zhong

> Sent: Thursday, April 11, 2019 11:14

> To: FFmpeg development discussions and patches <ffmpeg-

> devel@ffmpeg.org>

> Cc: Fu, Linjie <linjie.fu@intel.com>

> Subject: RE: [FFmpeg-devel] [PATCH 1/2] lavc/qsvenc: Fix the memory leak

> for enc_ctrl.Payload

> 

> > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On

> Behalf

> > Of Linjie Fu

> > Sent: Wednesday, April 10, 2019 7:27 PM

> > To: ffmpeg-devel@ffmpeg.org

> > Cc: Fu, Linjie <linjie.fu@intel.com>

> > Subject: [FFmpeg-devel] [PATCH 1/2] lavc/qsvenc: Fix the memory leak for

> > enc_ctrl.Payload

> >

> > frame->enc_ctrl.Payload is malloced in get_free_frame, directly memset

> > the whole structure of enc_ctrl to zero will cause the memory leak for

> > enc_ctrl.Payload.

> >

> > Fix the memory leak issue and reset other members in mfxEncodeCtrl.

> >

> > Signed-off-by: Linjie Fu <linjie.fu@intel.com>

> > ---

> >  libavcodec/qsvenc.c | 2 +-

> >  1 file changed, 1 insertion(+), 1 deletion(-)

> >

> > diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index

> > 5aa020d47b..029bb562d6 100644

> > --- a/libavcodec/qsvenc.c

> > +++ b/libavcodec/qsvenc.c

> > @@ -1254,7 +1254,7 @@ static int encode_frame(AVCodecContext *avctx,

> > QSVEncContext *q,

> >      if (qsv_frame) {

> >          surf = &qsv_frame->surface;

> >          enc_ctrl = &qsv_frame->enc_ctrl;

> > -        memset(enc_ctrl, 0, sizeof(mfxEncodeCtrl));

> > +        memset(enc_ctrl, 0, sizeof(mfxEncodeCtrl) - sizeof(mfxPayload

> > + **));

> 

> Looks a little tricky, if someone didn't read carefully the sequence of

> mfxPayload usages, they can't know why this change happening.

> And potential risk for other elements such as "mfxExtBuffer** ExtParam" is

> still existed.

> 

> IMHO, one good habit is that is you want to memset a buffer, would better

> memset it when you allocate it.

> So I believe the correct fix should be: memset mfxEncodeCtrl at the start of

> qsv frame allocated: the place should be in the function get_free_frame()

> 

Aware of the risk of mfxExtBuffer too, and move the memset at the start of the allocation is great, will modify.

Thanks,
Linjie
Linjie Fu April 11, 2019, 6:52 a.m.
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Carl Eugen Hoyos

> Sent: Wednesday, April 10, 2019 19:35

> To: FFmpeg development discussions and patches <ffmpeg-

> devel@ffmpeg.org>

> Subject: Re: [FFmpeg-devel] [PATCH 1/2] lavc/qsvenc: Fix the memory leak

> for enc_ctrl.Payload

> 

> 2019-04-10 13:27 GMT+02:00, Linjie Fu <linjie.fu@intel.com>:

> > frame->enc_ctrl.Payload is malloced in get_free_frame, directly memset

> > the whole structure of enc_ctrl to zero will cause the memory leak for

> > enc_ctrl.Payload.

> >

> > Fix the memory leak issue and reset other members in mfxEncodeCtrl.

> >

> > Signed-off-by: Linjie Fu <linjie.fu@intel.com>

> > ---

> >  libavcodec/qsvenc.c | 2 +-

> >  1 file changed, 1 insertion(+), 1 deletion(-)

> >

> > diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c

> > index 5aa020d47b..029bb562d6 100644

> > --- a/libavcodec/qsvenc.c

> > +++ b/libavcodec/qsvenc.c

> > @@ -1254,7 +1254,7 @@ static int encode_frame(AVCodecContext *avctx,

> > QSVEncContext *q,

> >      if (qsv_frame) {

> >          surf = &qsv_frame->surface;

> >          enc_ctrl = &qsv_frame->enc_ctrl;

> > -        memset(enc_ctrl, 0, sizeof(mfxEncodeCtrl));

> > +        memset(enc_ctrl, 0, sizeof(mfxEncodeCtrl) - sizeof(mfxPayload **));

> 

> Am I correct that this assumes a particular order of

> contents in a struct not defined within FFmpeg?

> Maybe saving the pointer in a variable and writing

> it back is more future proof?

> 

Yes it's a structure defined in MediaSDK, and will move the memset to the
start of the allocation to avoid this.

Thanks,
Linjie
Linjie Fu April 11, 2019, 9:15 a.m.
> -----Original Message-----

> From: Li, Zhong

> Sent: Thursday, April 11, 2019 11:14

> To: FFmpeg development discussions and patches <ffmpeg-

> devel@ffmpeg.org>

> Cc: Fu, Linjie <linjie.fu@intel.com>

> Subject: RE: [FFmpeg-devel] [PATCH 1/2] lavc/qsvenc: Fix the memory leak

> for enc_ctrl.Payload

> 

> > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On

> Behalf

> > Of Linjie Fu

> > Sent: Wednesday, April 10, 2019 7:27 PM

> > To: ffmpeg-devel@ffmpeg.org

> > Cc: Fu, Linjie <linjie.fu@intel.com>

> > Subject: [FFmpeg-devel] [PATCH 1/2] lavc/qsvenc: Fix the memory leak for

> > enc_ctrl.Payload

> >

> > frame->enc_ctrl.Payload is malloced in get_free_frame, directly memset

> > the whole structure of enc_ctrl to zero will cause the memory leak for

> > enc_ctrl.Payload.

> >

> > Fix the memory leak issue and reset other members in mfxEncodeCtrl.

> >

> > Signed-off-by: Linjie Fu <linjie.fu@intel.com>

> > ---

> >  libavcodec/qsvenc.c | 2 +-

> >  1 file changed, 1 insertion(+), 1 deletion(-)

> >

> > diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index

> > 5aa020d47b..029bb562d6 100644

> > --- a/libavcodec/qsvenc.c

> > +++ b/libavcodec/qsvenc.c

> > @@ -1254,7 +1254,7 @@ static int encode_frame(AVCodecContext *avctx,

> > QSVEncContext *q,

> >      if (qsv_frame) {

> >          surf = &qsv_frame->surface;

> >          enc_ctrl = &qsv_frame->enc_ctrl;

> > -        memset(enc_ctrl, 0, sizeof(mfxEncodeCtrl));

> > +        memset(enc_ctrl, 0, sizeof(mfxEncodeCtrl) - sizeof(mfxPayload

> > + **));

> 

> Looks a little tricky, if someone didn't read carefully the sequence of

> mfxPayload usages, they can't know why this change happening.

> And potential risk for other elements such as "mfxExtBuffer** ExtParam" is

> still existed.

> 

> IMHO, one good habit is that is you want to memset a buffer, would better

> memset it when you allocate it.

> So I believe the correct fix should be: memset mfxEncodeCtrl at the start of

> qsv frame allocated: the place should be in the function get_free_frame()


frame->enc_ctrl  as a structure will be malloc and init to zero by calling frame = av_mallocz(sizeof(*frame)),
the memset may be redundant and can be removed in my opinion.

Patch hide | download patch | download mbox

diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
index 5aa020d47b..029bb562d6 100644
--- a/libavcodec/qsvenc.c
+++ b/libavcodec/qsvenc.c
@@ -1254,7 +1254,7 @@  static int encode_frame(AVCodecContext *avctx, QSVEncContext *q,
     if (qsv_frame) {
         surf = &qsv_frame->surface;
         enc_ctrl = &qsv_frame->enc_ctrl;
-        memset(enc_ctrl, 0, sizeof(mfxEncodeCtrl));
+        memset(enc_ctrl, 0, sizeof(mfxEncodeCtrl) - sizeof(mfxPayload **));
 
         if (frame->pict_type == AV_PICTURE_TYPE_I) {
             enc_ctrl->FrameType = MFX_FRAMETYPE_I | MFX_FRAMETYPE_REF;