media: mediatek: encoder: memset encoder structure data

[PATCH] media: mediatek: encoder: memset encoder structure data

Posted by Irui Wang 2 months, 3 weeks ago

Utilized memset to set all bytes of encoder structure to zero,
this prevents any undefined behavior due to uninitialized use.

Signed-off-by: Irui Wang <irui.wang@mediatek.com>
---
 .../media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c  | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c
index a01dc25a7699..ecac1aec7215 100644
--- a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c
+++ b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c
@@ -886,6 +886,7 @@ static int vb2ops_venc_start_streaming(struct vb2_queue *q, unsigned int count)
 			return 0;
 	}
 
+	memset(&param, 0, sizeof(param));
 	mtk_venc_set_param(ctx, &param);
 	ret = venc_if_set_param(ctx, VENC_SET_PARAM_ENC, &param);
 	if (ret) {
@@ -1021,12 +1022,14 @@ static int mtk_venc_encode_header(void *priv)
 	struct mtk_vcodec_mem bs_buf;
 	struct venc_done_result enc_result;
 
+	memset(&enc_result, 0, sizeof(enc_result));
 	dst_buf = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
 	if (!dst_buf) {
 		mtk_v4l2_venc_dbg(1, ctx, "No dst buffer");
 		return -EINVAL;
 	}
 
+	memset(&bs_buf, 0, sizeof(bs_buf));
 	bs_buf.va = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
 	bs_buf.dma_addr = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
 	bs_buf.size = (size_t)dst_buf->vb2_buf.planes[0].length;
@@ -1143,6 +1146,7 @@ static void mtk_venc_worker(struct work_struct *work)
 	struct venc_done_result enc_result;
 	int ret, i;
 
+	memset(&enc_result, 0, sizeof(enc_result));
 	/* check dst_buf, dst_buf may be removed in device_run
 	 * to stored encdoe header so we need check dst_buf and
 	 * call job_finish here to prevent recursion
@@ -1175,6 +1179,7 @@ static void mtk_venc_worker(struct work_struct *work)
 		frm_buf.fb_addr[i].size =
 				(size_t)src_buf->vb2_buf.planes[i].length;
 	}
+	memset(&bs_buf, 0, sizeof(bs_buf));
 	bs_buf.va = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
 	bs_buf.dma_addr = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
 	bs_buf.size = (size_t)dst_buf->vb2_buf.planes[0].length;
-- 
2.46.0

Re: [PATCH] media: mediatek: encoder: memset encoder structure data

Posted by AngeloGioacchino Del Regno 2 months, 3 weeks ago

Il 15/07/25 10:15, Irui Wang ha scritto:
> Utilized memset to set all bytes of encoder structure to zero,
> this prevents any undefined behavior due to uninitialized use.
> 
> Signed-off-by: Irui Wang <irui.wang@mediatek.com>

This commit needs a Fixes tag, as you're fixing something important here.

Also, please clarify what is this undefined behavior that you've seen and
what problem are you trying to resolve by zeroing all those mem locations.

There's also more feedback, check below...

> ---
>   .../media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c  | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c
> index a01dc25a7699..ecac1aec7215 100644
> --- a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c
> +++ b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c
> @@ -886,6 +886,7 @@ static int vb2ops_venc_start_streaming(struct vb2_queue *q, unsigned int count)
>   			return 0;
>   	}
>   
> +	memset(&param, 0, sizeof(param));

Have you considered doing, instead...

	struct venc_enc_param param = { 0 }; ?

>   	mtk_venc_set_param(ctx, &param);
>   	ret = venc_if_set_param(ctx, VENC_SET_PARAM_ENC, &param);
>   	if (ret) {
> @@ -1021,12 +1022,14 @@ static int mtk_venc_encode_header(void *priv)
>   	struct mtk_vcodec_mem bs_buf;
>   	struct venc_done_result enc_result;

  	struct venc_done_result enc_result = { 0 };

>   
> +	memset(&enc_result, 0, sizeof(enc_result));
>   	dst_buf = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
>   	if (!dst_buf) {
>   		mtk_v4l2_venc_dbg(1, ctx, "No dst buffer");
>   		return -EINVAL;
>   	}
>   
> +	memset(&bs_buf, 0, sizeof(bs_buf));
>   	bs_buf.va = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
>   	bs_buf.dma_addr = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
>   	bs_buf.size = (size_t)dst_buf->vb2_buf.planes[0].length;
> @@ -1143,6 +1146,7 @@ static void mtk_venc_worker(struct work_struct *work)
>   	struct venc_done_result enc_result;
>   	int ret, i;
>   
> +	memset(&enc_result, 0, sizeof(enc_result));

You should probably move this to before the first usage, instead.

>   	/* check dst_buf, dst_buf may be removed in device_run
>   	 * to stored encdoe header so we need check dst_buf and
>   	 * call job_finish here to prevent recursion
> @@ -1175,6 +1179,7 @@ static void mtk_venc_worker(struct work_struct *work)
>   		frm_buf.fb_addr[i].size =
>   				(size_t)src_buf->vb2_buf.planes[i].length;
>   	}
> +	memset(&bs_buf, 0, sizeof(bs_buf));

here it's fine to use memset, as there are multiple ways out before actually using
bs_buf.

Cheers,
Angelo

>   	bs_buf.va = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
>   	bs_buf.dma_addr = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
>   	bs_buf.size = (size_t)dst_buf->vb2_buf.planes[0].length;

Re: [PATCH] media: mediatek: encoder: memset encoder structure data

Posted by Irui Wang (王瑞) 2 months, 3 weeks ago

Dear Angelo,

Thanks for your reviewing.

On Tue, 2025-07-15 at 15:26 +0200, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 15/07/25 10:15, Irui Wang ha scritto:
> > Utilized memset to set all bytes of encoder structure to zero,
> > this prevents any undefined behavior due to uninitialized use.
> > 
> > Signed-off-by: Irui Wang <irui.wang@mediatek.com>
> 
> This commit needs a Fixes tag, as you're fixing something important
> here.
> 
> Also, please clarify what is this undefined behavior that you've seen
> and
> what problem are you trying to resolve by zeroing all those mem
> locations.
OK, fix it in next patch.
> 
> There's also more feedback, check below...
> 
> > ---
> >   .../media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c  | 5
> > +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c
> > b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c
> > index a01dc25a7699..ecac1aec7215 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c
> > +++
> > b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c
> > @@ -886,6 +886,7 @@ static int vb2ops_venc_start_streaming(struct
> > vb2_queue *q, unsigned int count)
> >                       return 0;
> >       }
> > 
> > +     memset(&param, 0, sizeof(param));
> 
> Have you considered doing, instead...
> 
>         struct venc_enc_param param = { 0 }; ?
OK, fix it in next patch.
> 
> >       mtk_venc_set_param(ctx, &param);
> >       ret = venc_if_set_param(ctx, VENC_SET_PARAM_ENC, &param);
> >       if (ret) {
> > @@ -1021,12 +1022,14 @@ static int mtk_venc_encode_header(void
> > *priv)
> >       struct mtk_vcodec_mem bs_buf;
> >       struct venc_done_result enc_result;
> 
>         struct venc_done_result enc_result = { 0 };
> 
> > 
> > +     memset(&enc_result, 0, sizeof(enc_result));
> >       dst_buf = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
> >       if (!dst_buf) {
> >               mtk_v4l2_venc_dbg(1, ctx, "No dst buffer");
> >               return -EINVAL;
> >       }
> > 
> > +     memset(&bs_buf, 0, sizeof(bs_buf));
> >       bs_buf.va = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
> >       bs_buf.dma_addr = vb2_dma_contig_plane_dma_addr(&dst_buf-
> > >vb2_buf, 0);
> >       bs_buf.size = (size_t)dst_buf->vb2_buf.planes[0].length;
> > @@ -1143,6 +1146,7 @@ static void mtk_venc_worker(struct
> > work_struct *work)
> >       struct venc_done_result enc_result;
> >       int ret, i;
> > 
> > +     memset(&enc_result, 0, sizeof(enc_result));
> 
> You should probably move this to before the first usage, instead.
OK, fix it in next patch.
> 
> >       /* check dst_buf, dst_buf may be removed in device_run
> >        * to stored encdoe header so we need check dst_buf and
> >        * call job_finish here to prevent recursion
> > @@ -1175,6 +1179,7 @@ static void mtk_venc_worker(struct
> > work_struct *work)
> >               frm_buf.fb_addr[i].size =
> >                               (size_t)src_buf-
> > >vb2_buf.planes[i].length;
> >       }
> > +     memset(&bs_buf, 0, sizeof(bs_buf));
> 
> here it's fine to use memset, as there are multiple ways out before
> actually using
> bs_buf.
We will remove the bs_buf memset, because the bs_buf's variables are
set(va/dma_add/size) after definition.

Thanks!
> 
> Cheers,
> Angelo
> 
> >       bs_buf.va = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
> >       bs_buf.dma_addr = vb2_dma_contig_plane_dma_addr(&dst_buf-
> > >vb2_buf, 0);
> >       bs_buf.size = (size_t)dst_buf->vb2_buf.planes[0].length;
> 
>