2021-12-31 07:50:41

by Jiasheng Jiang

Subject: [PATCH] media: vidtv: Check for null return of vzalloc

As the possible failure of the vzalloc(), e->encoder_buf might be NULL.
Therefore, it should be better to check it like the kzalloc() in order
to guarantee the success of the initialization.

Fixes: f90cf6079bf6 ("media: vidtv: add a bridge driver")
Signed-off-by: Jiasheng Jiang <[email protected]>
---
drivers/media/test-drivers/vidtv/vidtv_s302m.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/drivers/media/test-drivers/vidtv/vidtv_s302m.c b/drivers/media/test-drivers/vidtv/vidtv_s302m.c
index d79b65854627..d7907f1ae530 100644
--- a/drivers/media/test-drivers/vidtv/vidtv_s302m.c
+++ b/drivers/media/test-drivers/vidtv/vidtv_s302m.c
@@ -455,6 +455,11 @@ struct vidtv_encoder
e->name = kstrdup(args.name, GFP_KERNEL);

e->encoder_buf = vzalloc(VIDTV_S302M_BUF_SZ);
+ if (!e->encoder_buf) {
+ kfree(e);
+ return NULL;
+ }
+
e->encoder_buf_sz = VIDTV_S302M_BUF_SZ;
e->encoder_buf_offset = 0;

--
2.25.1

2022-01-11 09:58:19

by Hans Verkuil

Subject: Re: [PATCH] media: vidtv: Check for null return of vzalloc

On 31/12/2021 08:50, Jiasheng Jiang wrote:
> As the possible failure of the vzalloc(), e->encoder_buf might be NULL.
> Therefore, it should be better to check it like the kzalloc() in order
> to guarantee the success of the initialization.
>
> Fixes: f90cf6079bf6 ("media: vidtv: add a bridge driver")
> Signed-off-by: Jiasheng Jiang <[email protected]>
> ---
> drivers/media/test-drivers/vidtv/vidtv_s302m.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/media/test-drivers/vidtv/vidtv_s302m.c b/drivers/media/test-drivers/vidtv/vidtv_s302m.c
> index d79b65854627..d7907f1ae530 100644
> --- a/drivers/media/test-drivers/vidtv/vidtv_s302m.c
> +++ b/drivers/media/test-drivers/vidtv/vidtv_s302m.c
> @@ -455,6 +455,11 @@ struct vidtv_encoder
> e->name = kstrdup(args.name, GFP_KERNEL);
>
> e->encoder_buf = vzalloc(VIDTV_S302M_BUF_SZ);
> + if (!e->encoder_buf) {

This doesn't free e->name!

Actually, the same issue is also present later in this function:

ctx = kzalloc(priv_sz, GFP_KERNEL);
if (!ctx) {
kfree(e);
return NULL;
}

Regards,

Hans

> + kfree(e);
> + return NULL;
> + }
> +
> e->encoder_buf_sz = VIDTV_S302M_BUF_SZ;
> e->encoder_buf_offset = 0;
>

2022-01-12 02:07:46

by Jiasheng Jiang

Subject: Re: Re: [PATCH] media: vidtv: Check for null return of vzalloc

On Tue, Jan 11, 2022 at 05:58:10PM +0800, Hans Verkuil wrote:
> This doesn't free e->name!
>
> Actually, the same issue is also present later in this function:
>
> ctx = kzalloc(priv_sz, GFP_KERNEL);
> if (!ctx) {
> kfree(e);
> return NULL;
> }

>> + kfree(e);
>> + return NULL;
>> + }

Thanks for your reminder.
I have sent a v2 to fix it.
And also I have sent a patch to fix the 'ctx' with
'Reported-by: Hans Verkuil <[email protected]>',
since they are two different things.

Sincerely thanks,
Jiang

2022-01-13 10:12:14

by Hans Verkuil

Subject: Re: [PATCH] media: vidtv: Check for null return of vzalloc

On 12/01/2022 03:07, Jiasheng Jiang wrote:
> On Tue, Jan 11, 2022 at 05:58:10PM +0800, Hans Verkuil wrote:
>> This doesn't free e->name!
>>
>> Actually, the same issue is also present later in this function:
>>
>> ctx = kzalloc(priv_sz, GFP_KERNEL);
>> if (!ctx) {
>> kfree(e);
>> return NULL;
>> }
>
>>> + kfree(e);
>>> + return NULL;
>>> + }
>
> Thanks for your reminder.
> I have sent a v2 to fix it.
> And also I have sent a patch to fix the 'ctx' with
> 'Reported-by: Hans Verkuil <[email protected]>',
> since they are two different things.

Actually no. It's all the same thing: not correctly freeing allocated memory
in a function.

Please combine it all in a single patch.

Perhaps this should use the 'goto' method of cleaning up after errors instead
of duplicating kfree()s in two places. A bit borderline what works best here.

Regards,

Hans

>
> Sincerely thanks,
> Jiang
>