[PATCH] sound: fix uninit-value in sof_ipc4_pcm_dai_link_fixup_rate

[PATCH] sound: fix uninit-value in sof_ipc4_pcm_dai_link_fixup_rate

[Suraj Sonawane]

Fix an issue detected by the Smatch tool:

sound/soc/sof/ipc4-pcm.c:615 sof_ipc4_pcm_dai_link_fixup_rate()
error: uninitialized symbol 'be_rate'.
sound/soc/sof/ipc4-pcm.c:636 sof_ipc4_pcm_dai_link_fixup_rate()
error: uninitialized symbol 'be_rate'.

These errors occurred because the variable 'be_rate' is declared but
may not be assigned a value before it is used. Specifically, if the
loop that assigns values to 'be_rate' does not execute (for example,
when 'num_input_formats' is zero), 'be_rate' remains uninitialized,
leading to potential undefined behavior.

To resolve this issue, initialize 'be_rate' to 0 at the point of
declaration. This ensures that 'be_rate' has a defined value before
it is used in subsequent calculations, preventing any warnings or
undefined behavior in cases where the loop does not run.

Signed-off-by: Suraj Sonawane <surajsonawane0215@gmail.com>
---
 sound/soc/sof/ipc4-pcm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/sof/ipc4-pcm.c b/sound/soc/sof/ipc4-pcm.c
index 4df2be3d3..d08419859 100644
--- a/sound/soc/sof/ipc4-pcm.c
+++ b/sound/soc/sof/ipc4-pcm.c
@@ -600,7 +600,7 @@ static int sof_ipc4_pcm_dai_link_fixup_rate(struct snd_sof_dev *sdev,
 	unsigned int fe_rate = params_rate(params);
 	bool fe_be_rate_match = false;
 	bool single_be_rate = true;
-	unsigned int be_rate;
+	unsigned int be_rate = 0;
 	int i;
 
 	/*
-- 
2.34.1

Re: [PATCH] sound: fix uninit-value in sof_ipc4_pcm_dai_link_fixup_rate

[Mark Brown]

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

On Wed, Oct 30, 2024 at 09:27:05PM +0530, Suraj Sonawane wrote:
> Fix an issue detected by the Smatch tool:

Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.

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

Re: [PATCH] sound: fix uninit-value in sof_ipc4_pcm_dai_link_fixup_rate

[Suraj Sonawane]

On 30/10/24 22:40, Mark Brown wrote:
> Please submit patches using subject lines reflecting the style for the
> subsystem, this makes it easier for people to identify relevant patches.
> Look at what existing commits in the area you're changing are doing and
> make sure your subject lines visually resemble what they're doing.
> There's no need to resubmit to fix this alone.
Thank you for the guidance! I’ll make sure to follow the subsystem’s 
subject line style in future submissions.

Re: [PATCH] sound: fix uninit-value in sof_ipc4_pcm_dai_link_fixup_rate

[Mark Brown]

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

On Wed, Oct 30, 2024 at 09:27:05PM +0530, Suraj Sonawane wrote:

> These errors occurred because the variable 'be_rate' is declared but
> may not be assigned a value before it is used. Specifically, if the
> loop that assigns values to 'be_rate' does not execute (for example,
> when 'num_input_formats' is zero), 'be_rate' remains uninitialized,
> leading to potential undefined behavior.
> 
> To resolve this issue, initialize 'be_rate' to 0 at the point of
> declaration. This ensures that 'be_rate' has a defined value before
> it is used in subsequent calculations, preventing any warnings or
> undefined behavior in cases where the loop does not run.

Again, this shuts the warning up but is this actually a good fix?

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

[PATCH v2] sound: fix uninit-value in sof_ipc4_pcm_dai_link_fixup_rate

[Suraj Sonawane]

Fix an issue detected by the Smatch tool:

sound/soc/sof/ipc4-pcm.c: sof_ipc4_pcm_dai_link_fixup_rate()
error: uninitialized symbol 'be_rate'.

This issue occurred because the variable 'be_rate' could remain
uninitialized if num_input_formats is zero. In such cases, the
loop that assigns a value to 'be_rate' would not execute,
potentially leading to undefined behavior when rate->min and
rate->max are set with an uninitialized 'be_rate'.

To resolve this, an additional check for num_input_formats > 0
was added before setting rate->min and rate->max with 'be_rate'.
This ensures that 'be_rate' is assigned only when there are valid
input formats, preventing any use of uninitialized data.

This solution maintains defined behavior for rate->min and rate->max,
ensuring they are only assigned when valid be_rate data is available.

Signed-off-by: Suraj Sonawane <surajsonawane0215@gmail.com>
---
V1: Initialize 'be_rate' to 0.
V2: Add conditional assignment based on num_input_formats to ensure
be_rate is used only when assigned.

 sound/soc/sof/ipc4-pcm.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/sound/soc/sof/ipc4-pcm.c b/sound/soc/sof/ipc4-pcm.c
index 4df2be3d3..d5d7ffc69 100644
--- a/sound/soc/sof/ipc4-pcm.c
+++ b/sound/soc/sof/ipc4-pcm.c
@@ -633,8 +633,11 @@ static int sof_ipc4_pcm_dai_link_fixup_rate(struct snd_sof_dev *sdev,
 			return -EINVAL;
 		}
 
-		rate->min = be_rate;
-		rate->max = rate->min;
+		/* Set rate only if be_rate was assigned */
+		if (num_input_formats > 0) {
+			rate->min = be_rate;
+			rate->max = rate->min;
+		}
 	}
 
 	return 0;
-- 
2.34.1

Re: [PATCH v2] sound: fix uninit-value in sof_ipc4_pcm_dai_link_fixup_rate

[Péter Ujfalusi]

On 03/11/2024 13:37, Suraj Sonawane wrote:
> Fix an issue detected by the Smatch tool:
> 
> sound/soc/sof/ipc4-pcm.c: sof_ipc4_pcm_dai_link_fixup_rate()
> error: uninitialized symbol 'be_rate'.
> 
> This issue occurred because the variable 'be_rate' could remain
> uninitialized if num_input_formats is zero. In such cases, the
> loop that assigns a value to 'be_rate' would not execute,
> potentially leading to undefined behavior when rate->min and
> rate->max are set with an uninitialized 'be_rate'.
> 
> To resolve this, an additional check for num_input_formats > 0
> was added before setting rate->min and rate->max with 'be_rate'.
> This ensures that 'be_rate' is assigned only when there are valid
> input formats, preventing any use of uninitialized data.
> 
> This solution maintains defined behavior for rate->min and rate->max,
> ensuring they are only assigned when valid be_rate data is available.
> 
> Signed-off-by: Suraj Sonawane <surajsonawane0215@gmail.com>
> ---
> V1: Initialize 'be_rate' to 0.
> V2: Add conditional assignment based on num_input_formats to ensure
> be_rate is used only when assigned.
> 
>  sound/soc/sof/ipc4-pcm.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/sof/ipc4-pcm.c b/sound/soc/sof/ipc4-pcm.c
> index 4df2be3d3..d5d7ffc69 100644
> --- a/sound/soc/sof/ipc4-pcm.c
> +++ b/sound/soc/sof/ipc4-pcm.c
> @@ -633,8 +633,11 @@ static int sof_ipc4_pcm_dai_link_fixup_rate(struct snd_sof_dev *sdev,
>  			return -EINVAL;
>  		}
>  
> -		rate->min = be_rate;
> -		rate->max = rate->min;
> +		/* Set rate only if be_rate was assigned */
> +		if (num_input_formats > 0) {

By definition the copier must have at least one input and one output
format, this check is going to be always true.

> +			rate->min = be_rate;
> +			rate->max = rate->min;
> +		}
>  	}
>  
>  	return 0;

-- 
Péter

Re: [PATCH v2] sound: fix uninit-value in sof_ipc4_pcm_dai_link_fixup_rate

[Mark Brown]

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

On Mon, Nov 04, 2024 at 12:52:09PM +0200, Péter Ujfalusi wrote:
> On 03/11/2024 13:37, Suraj Sonawane wrote:

> > Fix an issue detected by the Smatch tool:
> > 
> > sound/soc/sof/ipc4-pcm.c: sof_ipc4_pcm_dai_link_fixup_rate()
> > error: uninitialized symbol 'be_rate'.
> > 
> > This issue occurred because the variable 'be_rate' could remain
> > uninitialized if num_input_formats is zero. In such cases, the
> > loop that assigns a value to 'be_rate' would not execute,
> > potentially leading to undefined behavior when rate->min and
> > rate->max are set with an uninitialized 'be_rate'.
> > 
> > To resolve this, an additional check for num_input_formats > 0
> > was added before setting rate->min and rate->max with 'be_rate'.
> > This ensures that 'be_rate' is assigned only when there are valid
> > input formats, preventing any use of uninitialized data.

> > -		rate->min = be_rate;
> > -		rate->max = rate->min;
> > +		/* Set rate only if be_rate was assigned */
> > +		if (num_input_formats > 0) {

> By definition the copier must have at least one input and one output
> format, this check is going to be always true.

Static analysis of the code can't reasonably tell that, we need
something that ensures that it doesn't detect a spuriously uninitialised
variable here.  Possibly a

	if (WARN_ON_ONCE(!num_input_formats))
		return -EINVAL;

or similar?

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

Re: [PATCH v2] sound: fix uninit-value in sof_ipc4_pcm_dai_link_fixup_rate

[Suraj Sonawane]

On 04/11/24 23:57, Mark Brown wrote:
> On Mon, Nov 04, 2024 at 12:52:09PM +0200, Péter Ujfalusi wrote:
>> On 03/11/2024 13:37, Suraj Sonawane wrote:
> 
>>> Fix an issue detected by the Smatch tool:
>>>
>>> sound/soc/sof/ipc4-pcm.c: sof_ipc4_pcm_dai_link_fixup_rate()
>>> error: uninitialized symbol 'be_rate'.
>>>
>>> This issue occurred because the variable 'be_rate' could remain
>>> uninitialized if num_input_formats is zero. In such cases, the
>>> loop that assigns a value to 'be_rate' would not execute,
>>> potentially leading to undefined behavior when rate->min and
>>> rate->max are set with an uninitialized 'be_rate'.
>>>
>>> To resolve this, an additional check for num_input_formats > 0
>>> was added before setting rate->min and rate->max with 'be_rate'.
>>> This ensures that 'be_rate' is assigned only when there are valid
>>> input formats, preventing any use of uninitialized data.
> 
>>> -		rate->min = be_rate;
>>> -		rate->max = rate->min;
>>> +		/* Set rate only if be_rate was assigned */
>>> +		if (num_input_formats > 0) {
> 
>> By definition the copier must have at least one input and one output
>> format, this check is going to be always true.
> 
> Static analysis of the code can't reasonably tell that, we need
> something that ensures that it doesn't detect a spuriously uninitialised
> variable here.  Possibly a
> 
> 	if (WARN_ON_ONCE(!num_input_formats))
> 		return -EINVAL;
> 
> or similar?

Thank you, Mark and Péter, for the guidance. I understand now that, 
while the copier should always have at least one input format, static 
analysis tools can’t detect this. Based on your suggestions, I’ve 
considered the following possible solutions to address the issue:

1. Add a WARN_ON_ONCE(!num_input_formats) check: This would issue a 
warning and return an error if num_input_formats is unexpectedly zero, 
ensuring we handle any edge cases explicitly.

2. Return an error if no input formats are available: Implementing the 
following check could provide immediate feedback if num_input_formats is 
zero:
     if (num_input_formats <= 0) {
         dev_err(sdev->dev, "No input formats available\n");
         return -EINVAL; // Return an error if there are no formats
     }

Would it be preferable to proceed with the 
WARN_ON_ONCE(!num_input_formats) approach, or is there a preferred 
alternative from the options above?

Thank you again

Re: [PATCH v2] sound: fix uninit-value in sof_ipc4_pcm_dai_link_fixup_rate

[Mark Brown]

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

On Tue, Nov 05, 2024 at 04:20:23PM +0530, Suraj Sonawane wrote:

> Thank you, Mark and Péter, for the guidance. I understand now that, while
> the copier should always have at least one input format, static analysis
> tools can’t detect this. Based on your suggestions, I’ve considered the
> following possible solutions to address the issue:

> 1. Add a WARN_ON_ONCE(!num_input_formats) check: This would issue a warning
> and return an error if num_input_formats is unexpectedly zero, ensuring we
> handle any edge cases explicitly.

> 2. Return an error if no input formats are available: Implementing the
> following check could provide immediate feedback if num_input_formats is
> zero:
>     if (num_input_formats <= 0) {
>         dev_err(sdev->dev, "No input formats available\n");
>         return -EINVAL; // Return an error if there are no formats
>     }

> Would it be preferable to proceed with the WARN_ON_ONCE(!num_input_formats)
> approach, or is there a preferred alternative from the options above?

I don't have a super strong preference between the two options.

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

Re: [PATCH v2] sound: fix uninit-value in sof_ipc4_pcm_dai_link_fixup_rate

[Suraj Sonawane]

On 06/11/24 00:37, Mark Brown wrote:
> On Tue, Nov 05, 2024 at 04:20:23PM +0530, Suraj Sonawane wrote:
> 
>> Thank you, Mark and Péter, for the guidance. I understand now that, while
>> the copier should always have at least one input format, static analysis
>> tools can’t detect this. Based on your suggestions, I’ve considered the
>> following possible solutions to address the issue:
> 
>> 1. Add a WARN_ON_ONCE(!num_input_formats) check: This would issue a warning
>> and return an error if num_input_formats is unexpectedly zero, ensuring we
>> handle any edge cases explicitly.
> 
>> 2. Return an error if no input formats are available: Implementing the
>> following check could provide immediate feedback if num_input_formats is
>> zero:
>>      if (num_input_formats <= 0) {
>>          dev_err(sdev->dev, "No input formats available\n");
>>          return -EINVAL; // Return an error if there are no formats
>>      }
> 
>> Would it be preferable to proceed with the WARN_ON_ONCE(!num_input_formats)
>> approach, or is there a preferred alternative from the options above?
> 
> I don't have a super strong preference between the two options.
Thank you for the clarification. I’ll study the best approach in more 
detail and will send the patch in a while.

Thanks again for your time and feedback!

Re: [PATCH v2] sound: fix uninit-value in sof_ipc4_pcm_dai_link_fixup_rate

[Suraj Sonawane]

On 04/11/24 16:22, Péter Ujfalusi wrote:
> 
> 
> On 03/11/2024 13:37, Suraj Sonawane wrote:
>> Fix an issue detected by the Smatch tool:
>>
>> sound/soc/sof/ipc4-pcm.c: sof_ipc4_pcm_dai_link_fixup_rate()
>> error: uninitialized symbol 'be_rate'.
>>
>> This issue occurred because the variable 'be_rate' could remain
>> uninitialized if num_input_formats is zero. In such cases, the
>> loop that assigns a value to 'be_rate' would not execute,
>> potentially leading to undefined behavior when rate->min and
>> rate->max are set with an uninitialized 'be_rate'.
>>
>> To resolve this, an additional check for num_input_formats > 0
>> was added before setting rate->min and rate->max with 'be_rate'.
>> This ensures that 'be_rate' is assigned only when there are valid
>> input formats, preventing any use of uninitialized data.
>>
>> This solution maintains defined behavior for rate->min and rate->max,
>> ensuring they are only assigned when valid be_rate data is available.
>>
>> Signed-off-by: Suraj Sonawane <surajsonawane0215@gmail.com>
>> ---
>> V1: Initialize 'be_rate' to 0.
>> V2: Add conditional assignment based on num_input_formats to ensure
>> be_rate is used only when assigned.
>>
>>   sound/soc/sof/ipc4-pcm.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/sound/soc/sof/ipc4-pcm.c b/sound/soc/sof/ipc4-pcm.c
>> index 4df2be3d3..d5d7ffc69 100644
>> --- a/sound/soc/sof/ipc4-pcm.c
>> +++ b/sound/soc/sof/ipc4-pcm.c
>> @@ -633,8 +633,11 @@ static int sof_ipc4_pcm_dai_link_fixup_rate(struct snd_sof_dev *sdev,
>>   			return -EINVAL;
>>   		}
>>   
>> -		rate->min = be_rate;
>> -		rate->max = rate->min;
>> +		/* Set rate only if be_rate was assigned */
>> +		if (num_input_formats > 0) {
> 
> By definition the copier must have at least one input and one output
> format, this check is going to be always true.
> 
>> +			rate->min = be_rate;
>> +			rate->max = rate->min;
>> +		}
>>   	}
>>   
>>   	return 0;
> 
Thank you for the clarification.