Re: [PATCH] Fix possible NULL Pointer Dereference in 'asoc_qcom_lpass_cpu_platform_probe'

Re: [PATCH] Fix possible NULL Pointer Dereference in 'asoc_qcom_lpass_cpu_platform_probe'

[Markus Elfring]

> A 'devm_kzalloc' in 'asoc_qcom_lpass_cpu_platform_probe' could possibly return NULL pointer.
> NULL Pointer Dereference may be triggerred in 'asoc_qcom_lpass_cpu_platform_probe' without addtional check.
> Add a null check for the returned pointer.

How do you think about a wording variant like the following?

  The result from a call of the function “devm_kzalloc” was passed to
  a subsequent function call without checking for a null pointer before
  (according to a memory allocation failure).
  Thus return directly after a failed devm_kzalloc() call.


…
> Signed-off-by: Zichen Xie <zichenxie0106@gmail.com>
> Reported-by: Zichen Xie <zichenxie0106@gmail.com>
…

How good does such a tag combination fit together for the same person?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.12-rc1#n525


Can a subject like “[PATCH] ASoC: qcom: lpass-cpu: Return directly after a failed devm_kzalloc() call
in asoc_qcom_lpass_cpu_platform_probe()” be more appropriate?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.12-rc1#n613


…
> +++ b/sound/soc/qcom/lpass-cpu.c
> @@ -1243,6 +1243,9 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev)
>  	drvdata->i2sctl = devm_kzalloc(&pdev->dev, sizeof(struct lpaif_i2sctl),
>  					GFP_KERNEL);
>
> +	if (!drvdata->i2sctl)
> +		return -ENOMEM;
…

I suggest to omit a blank line here.


By the way:
Would you become interested to omit the label “err” from this function implementation finally?

Regards,
Markus

Re: [PATCH] Fix possible NULL Pointer Dereference in 'asoc_qcom_lpass_cpu_platform_probe'

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 539 bytes --]

On Mon, Sep 30, 2024 at 06:33:49PM +0200, Markus Elfring wrote:

> How do you think about a wording variant like the following?

>   The result from a call of the function “devm_kzalloc” was passed to
>   a subsequent function call without checking for a null pointer before
>   (according to a memory allocation failure).
>   Thus return directly after a failed devm_kzalloc() call.

Feel free to ignore Markus, he has a long history of sending
unhelpful review comments and continues to ignore repeated requests
to stop.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Fix possible NULL Pointer Dereference in 'asoc_qcom_lpass_cpu_platform_probe'

[Markus Elfring]

>> How do you think about a wording variant like the following?
>
>>   The result from a call of the function “devm_kzalloc” was passed to
>>   a subsequent function call without checking for a null pointer before
>>   (according to a memory allocation failure).
>>   Thus return directly after a failed devm_kzalloc() call.
>
> Feel free to ignore Markus, he has a long history of sending
> unhelpful review comments and continues to ignore repeated requests
> to stop.

30 non-merge commits were published with related subjects so far.
Can you get a more constructive view from parts of public software development history?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?h=v6.12-rc1&qt=grep&q=Return+directly+after+a+failed

Regards,
Markus

Re: [PATCH] Fix possible NULL Pointer Dereference in 'asoc_qcom_lpass_cpu_platform_probe'

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 292 bytes --]

On Tue, Oct 01, 2024 at 02:48:54PM +0200, Markus Elfring wrote:

> > Your description and patch looks good to me.

> Interesting “view” …

Feel free to ignore Markus, he has a long history of sending
unhelpful review comments and continues to ignore repeated requests
to stop.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] Fix possible NULL Pointer Dereference in 'asoc_qcom_lpass_cpu_platform_probe'

[Markus Elfring]

> > A 'devm_kzalloc' in 'asoc_qcom_lpass_cpu_platform_probe' could possibly return NULL pointer.
> > NULL Pointer Dereference may be triggerred in 'asoc_qcom_lpass_cpu_platform_probe' without addtional check.
> > Add a null check for the returned pointer.
>
> Your description and patch looks good to me.

Interesting “view” …


> But please run git log on the changed file and add a prefix to your
> subject to match other changes to this file, and please wrap your commit
> message at 75 characters.

* How does your initial information fit to this patch review advice?

* Would you like to “tolerate” any typos (at the first glance)?


Regards,
Markus