iio: Fix uninitialized symbol 'ret'

[PATCH] iio: Fix uninitialized symbol 'ret'

Posted by Zicheng Qu 3 weeks, 3 days ago

Initialize the variable ret at the time of declaration to prevent it from
being returned without a defined value. Fixes smatch warning:
drivers/iio/industrialio-gts-helper.c:256 gain_to_scaletables() error:
uninitialized symbol 'ret'.

Cc: stable@vger.kernel.org # v6.6+
Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers")
Signed-off-by: Zicheng Qu <quzicheng@huawei.com>
---
 drivers/iio/industrialio-gts-helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
index 59d7615c0f56..c5dc5b51693d 100644
--- a/drivers/iio/industrialio-gts-helper.c
+++ b/drivers/iio/industrialio-gts-helper.c
@@ -167,7 +167,7 @@ static int iio_gts_gain_cmp(const void *a, const void *b)
 
 static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
 {
-	int ret, i, j, new_idx, time_idx;
+	int i, j, new_idx, time_idx, ret = 0;
 	int *all_gains;
 	size_t gain_bytes;
 
-- 
2.34.1

Re: [PATCH] iio: Fix uninitialized symbol 'ret'

Posted by Matti Vaittinen 3 weeks, 3 days ago

Hi Zicheng,

Thanks for the patch.

On 31/10/2024 03:45, Zicheng Qu wrote:
> Initialize the variable ret at the time of declaration to prevent it from
> being returned without a defined value. Fixes smatch warning:
> drivers/iio/industrialio-gts-helper.c:256 gain_to_scaletables() error:
> uninitialized symbol 'ret'.
> 
> Cc: stable@vger.kernel.org # v6.6+
> Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers")
> Signed-off-by: Zicheng Qu <quzicheng@huawei.com>
> ---
>   drivers/iio/industrialio-gts-helper.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
> index 59d7615c0f56..c5dc5b51693d 100644
> --- a/drivers/iio/industrialio-gts-helper.c
> +++ b/drivers/iio/industrialio-gts-helper.c
> @@ -167,7 +167,7 @@ static int iio_gts_gain_cmp(const void *a, const void *b)
>   
>   static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
>   {
> -	int ret, i, j, new_idx, time_idx;
> +	int i, j, new_idx, time_idx, ret = 0;
>   	int *all_gains;
>   	size_t gain_bytes;
>   

So, if I read it right, this handles a (corner) case where there is no 
times given. I am not sure how well such use has been considered because 
the point of GTS is helping out with cases where the gain and 
integration time both impact to scale.

How do you see the benefits of the gts if there is no such shared impact 
to scale? Sure the gts could still provide the 'standard table format' 
to present the gains (or times), and conversions from the register 
values to gains (or times), and perhaps the available scale table(s) - 
but I suppose it also brings a lot of unused code and some 
initialization overhead. (I have a vague feeling this was discussed with 
Jonathan during the reviews).

Reason I am asking these questions is that I wonder if the usage should 
be limited to cases where we have both gains and times? We could check 
this in the iio_gts_sanity_check(). (And, I am actually a bit surprized 
this check was not implemented).

Well, initialization fixes a potential bug here and does not really cost 
much - so big thanks to you :)

Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

Yours,
  -- Matti Vaittinen

Re: [PATCH] iio: Fix uninitialized symbol 'ret'

Posted by Zicheng Qu 3 weeks, 2 days ago

Hi Matti,

It might be better but I am not pretty sure whether have to have both 
gains and times, so I modified 'ret' in places where issues might arise, 
rather than adding a restriction in iio_gts_sanity_check().

In the corner case, there is a restriction that both num_hwgain and 
num_itime cannot be 0 simultaneously in the iio_gts_sanity_check(). 
However, in the gain_to_scaletables() , if num_itime is 1 and num_hwgain 
is 0, the 'gain_bytes' becomes 0 and 'all_gains' is not null after the 
'for' loop, which causes the subsequent 'while' or 'for' to become 
ineffective, leading to an undefined 'ret' being returned in the 'free_out'.

Yours,

-- Zicheng

On 2024/10/31 15:13, Matti Vaittinen wrote:
> Hi Zicheng,
>
> Thanks for the patch.
>
> On 31/10/2024 03:45, Zicheng Qu wrote:
>> Initialize the variable ret at the time of declaration to prevent it 
>> from
>> being returned without a defined value. Fixes smatch warning:
>> drivers/iio/industrialio-gts-helper.c:256 gain_to_scaletables() error:
>> uninitialized symbol 'ret'.
>>
>> Cc: stable@vger.kernel.org # v6.6+
>> Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers")
>> Signed-off-by: Zicheng Qu <quzicheng@huawei.com>
>> ---
>>   drivers/iio/industrialio-gts-helper.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/industrialio-gts-helper.c 
>> b/drivers/iio/industrialio-gts-helper.c
>> index 59d7615c0f56..c5dc5b51693d 100644
>> --- a/drivers/iio/industrialio-gts-helper.c
>> +++ b/drivers/iio/industrialio-gts-helper.c
>> @@ -167,7 +167,7 @@ static int iio_gts_gain_cmp(const void *a, const 
>> void *b)
>>     static int gain_to_scaletables(struct iio_gts *gts, int **gains, 
>> int **scales)
>>   {
>> -    int ret, i, j, new_idx, time_idx;
>> +    int i, j, new_idx, time_idx, ret = 0;
>>       int *all_gains;
>>       size_t gain_bytes;
>
> So, if I read it right, this handles a (corner) case where there is no 
> times given. I am not sure how well such use has been considered 
> because the point of GTS is helping out with cases where the gain and 
> integration time both impact to scale.
>
> How do you see the benefits of the gts if there is no such shared 
> impact to scale? Sure the gts could still provide the 'standard table 
> format' to present the gains (or times), and conversions from the 
> register values to gains (or times), and perhaps the available scale 
> table(s) - but I suppose it also brings a lot of unused code and some 
> initialization overhead. (I have a vague feeling this was discussed 
> with Jonathan during the reviews).
>
> Reason I am asking these questions is that I wonder if the usage 
> should be limited to cases where we have both gains and times? We 
> could check this in the iio_gts_sanity_check(). (And, I am actually a 
> bit surprized this check was not implemented).
>
> Well, initialization fixes a potential bug here and does not really 
> cost much - so big thanks to you :)
>
> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
>
> Yours,
>  -- Matti Vaittinen
>

Re: [PATCH] iio: Fix uninitialized symbol 'ret'

Posted by Matti Vaittinen 3 weeks, 1 day ago

On 01/11/2024 05:43, Zicheng Qu wrote:
> Hi Matti,
> 
> It might be better but I am not pretty sure whether have to have both 
> gains and times, so I modified 'ret' in places where issues might arise, 
> rather than adding a restriction in iio_gts_sanity_check().

Yes. Thanks for the fix.
> 
> In the corner case, there is a restriction that both num_hwgain and 
> num_itime cannot be 0 simultaneously in the iio_gts_sanity_check(). 

Right. Having both 0 would mean there was nothing at all to handle for 
the GTS.

> However, in the gain_to_scaletables() , if num_itime is 1 and num_hwgain 
> is 0, the 'gain_bytes' becomes 0 and 'all_gains' is not null after the 
> 'for' loop, which causes the subsequent 'while' or 'for' to become 
> ineffective, leading to an undefined 'ret' being returned in the 
> 'free_out'.

Yes. Thank you for the fix :)

Yours,
     -- Matti

Re: [PATCH] iio: Fix uninitialized symbol 'ret'

Posted by Jonathan Cameron 3 weeks, 3 days ago

On Thu, 31 Oct 2024 09:13:16 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Hi Zicheng,
> 
> Thanks for the patch.
> 
> On 31/10/2024 03:45, Zicheng Qu wrote:
> > Initialize the variable ret at the time of declaration to prevent it from
> > being returned without a defined value. Fixes smatch warning:
> > drivers/iio/industrialio-gts-helper.c:256 gain_to_scaletables() error:
> > uninitialized symbol 'ret'.
> > 
> > Cc: stable@vger.kernel.org # v6.6+
> > Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers")
> > Signed-off-by: Zicheng Qu <quzicheng@huawei.com>
> > ---
> >   drivers/iio/industrialio-gts-helper.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
> > index 59d7615c0f56..c5dc5b51693d 100644
> > --- a/drivers/iio/industrialio-gts-helper.c
> > +++ b/drivers/iio/industrialio-gts-helper.c
> > @@ -167,7 +167,7 @@ static int iio_gts_gain_cmp(const void *a, const void *b)
> >   
> >   static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
> >   {
> > -	int ret, i, j, new_idx, time_idx;
> > +	int i, j, new_idx, time_idx, ret = 0;
> >   	int *all_gains;
> >   	size_t gain_bytes;
> >     
> 
> So, if I read it right, this handles a (corner) case where there is no 
> times given. I am not sure how well such use has been considered because 
> the point of GTS is helping out with cases where the gain and 
> integration time both impact to scale.
> 
> How do you see the benefits of the gts if there is no such shared impact 
> to scale? Sure the gts could still provide the 'standard table format' 
> to present the gains (or times), and conversions from the register 
> values to gains (or times), and perhaps the available scale table(s) - 
> but I suppose it also brings a lot of unused code and some 
> initialization overhead. (I have a vague feeling this was discussed with 
> Jonathan during the reviews).
> 
> Reason I am asking these questions is that I wonder if the usage should 
> be limited to cases where we have both gains and times? We could check 
> this in the iio_gts_sanity_check(). (And, I am actually a bit surprized 
> this check was not implemented).
> 
> Well, initialization fixes a potential bug here and does not really cost 
> much - so big thanks to you :)
> 
> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
Indeed I'm not convinced this is a a bug that can be hit, but it is
obviously good hardening so applied to the fixes togreg branch of iio.git.
Note I'd like a follow up to use __free() + early returns in this function.
Will reduce complexity and that last line will become a return 0;

> 
> Yours,
>   -- Matti Vaittinen
>

Re: [PATCH] iio: Fix uninitialized symbol 'ret'

Posted by Matti Vaittinen 3 weeks, 1 day ago

On 31/10/2024 23:47, Jonathan Cameron wrote:
> On Thu, 31 Oct 2024 09:13:16 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> Hi Zicheng,
>>
>> Thanks for the patch.
>>
>> On 31/10/2024 03:45, Zicheng Qu wrote:
>>> Initialize the variable ret at the time of declaration to prevent it from
>>> being returned without a defined value. Fixes smatch warning:
>>> drivers/iio/industrialio-gts-helper.c:256 gain_to_scaletables() error:
>>> uninitialized symbol 'ret'.
>>>
>>> Cc: stable@vger.kernel.org # v6.6+
>>> Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers")
>>> Signed-off-by: Zicheng Qu <quzicheng@huawei.com>
>>> ---
>>>    drivers/iio/industrialio-gts-helper.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
>>> index 59d7615c0f56..c5dc5b51693d 100644
>>> --- a/drivers/iio/industrialio-gts-helper.c
>>> +++ b/drivers/iio/industrialio-gts-helper.c
>>> @@ -167,7 +167,7 @@ static int iio_gts_gain_cmp(const void *a, const void *b)
>>>    
>>>    static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
>>>    {
>>> -	int ret, i, j, new_idx, time_idx;
>>> +	int i, j, new_idx, time_idx, ret = 0;
>>>    	int *all_gains;
>>>    	size_t gain_bytes;
>>>      
>>
>> So, if I read it right, this handles a (corner) case where there is no
>> times given. I am not sure how well such use has been considered because
>> the point of GTS is helping out with cases where the gain and
>> integration time both impact to scale.
>>
>> How do you see the benefits of the gts if there is no such shared impact
>> to scale? Sure the gts could still provide the 'standard table format'
>> to present the gains (or times), and conversions from the register
>> values to gains (or times), and perhaps the available scale table(s) -
>> but I suppose it also brings a lot of unused code and some
>> initialization overhead. (I have a vague feeling this was discussed with
>> Jonathan during the reviews).
>>
>> Reason I am asking these questions is that I wonder if the usage should
>> be limited to cases where we have both gains and times? We could check
>> this in the iio_gts_sanity_check(). (And, I am actually a bit surprized
>> this check was not implemented).
>>
>> Well, initialization fixes a potential bug here and does not really cost
>> much - so big thanks to you :)
>>
>> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
> Indeed I'm not convinced this is a a bug that can be hit, but it is
> obviously good hardening so applied to the fixes togreg branch of iio.git.
> Note I'd like a follow up to use __free() + early returns in this function.
> Will reduce complexity and that last line will become a return 0;

I suppose it is time for me to adapt to use the __cleanup based 
helpers... I'll add this to my TODO-list :)

Yours,
	-- Matti