[v2] iio: Fix uninitialized variable

[PATCH v2] iio: Fix uninitialized variable

Posted by Yo-Jung (Leo) Lin 1 month, 2 weeks ago

clang found that the "offset" in bmp580_trigger_handler doesn't get
initialized before access. Add proper initialization to this variable.

Signed-off-by: Yo-Jung (Leo) Lin <0xff07@gmail.com>
---
Change in v2:
- Make value initialization immediate before its first use.
- Link to v1: https://lore.kernel.org/all/20241011093752.30685-1-0xff07@gmail.com/

---
 drivers/iio/pressure/bmp280-core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index f4df222ed0c3..682329f81886 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -2222,6 +2222,8 @@ static irqreturn_t bmp580_trigger_handler(int irq, void *p)
 		goto out;
 	}
 
+	offset = 0;
+
 	/* Pressure calculations */
 	memcpy(&data->sensor_data[offset], &data->buf[3], 3);
 
-- 
2.43.0

Re: [PATCH v2] iio: Fix uninitialized variable

Posted by Javier Carrasco 1 month, 2 weeks ago

On 11/10/2024 13:52, Yo-Jung (Leo) Lin wrote:
> clang found that the "offset" in bmp580_trigger_handler doesn't get
> initialized before access. Add proper initialization to this variable.
> 
> Signed-off-by: Yo-Jung (Leo) Lin <0xff07@gmail.com>
> ---
> Change in v2:
> - Make value initialization immediate before its first use.
> - Link to v1: https://lore.kernel.org/all/20241011093752.30685-1-0xff07@gmail.com/
> 
> ---
>  drivers/iio/pressure/bmp280-core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index f4df222ed0c3..682329f81886 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -2222,6 +2222,8 @@ static irqreturn_t bmp580_trigger_handler(int irq, void *p)
>  		goto out;
>  	}
>  
> +	offset = 0;
> +
>  	/* Pressure calculations */
>  	memcpy(&data->sensor_data[offset], &data->buf[3], 3);
>  

That was a quick reply. I would recommend you to wait a little bit while
the first version is under discussion.

I still see the offset thing a bit weird. data->sensor_data uses an
offset to avoid hard-coded numbers, but for data->buf we do exactly
that, in the very same lines.

Setting offset to 0 to access the first element i.e. no offset required,
and then adding the actual offset sizeof(s32), which could even be a
const if the first access was to sensor_data[0], looks to verbose.

These things are of course not critical, and the proposed fix is
definitely ok, but I am missing some consistency here.

Re: [PATCH v2] iio: Fix uninitialized variable

Posted by Vasileios Aoiridis 1 month, 2 weeks ago

On Fri, Oct 11, 2024 at 02:31:00PM +0200, Javier Carrasco wrote:
> On 11/10/2024 13:52, Yo-Jung (Leo) Lin wrote:
> > clang found that the "offset" in bmp580_trigger_handler doesn't get
> > initialized before access. Add proper initialization to this variable.
> > 
> > Signed-off-by: Yo-Jung (Leo) Lin <0xff07@gmail.com>
> > ---
> > Change in v2:
> > - Make value initialization immediate before its first use.
> > - Link to v1: https://lore.kernel.org/all/20241011093752.30685-1-0xff07@gmail.com/
> > 
> > ---
> >  drivers/iio/pressure/bmp280-core.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > index f4df222ed0c3..682329f81886 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -2222,6 +2222,8 @@ static irqreturn_t bmp580_trigger_handler(int irq, void *p)
> >  		goto out;
> >  	}
> >  
> > +	offset = 0;
> > +
> >  	/* Pressure calculations */
> >  	memcpy(&data->sensor_data[offset], &data->buf[3], 3);
> >  
> 
> That was a quick reply. I would recommend you to wait a little bit while
> the first version is under discussion.
> 
> I still see the offset thing a bit weird. data->sensor_data uses an
> offset to avoid hard-coded numbers, but for data->buf we do exactly
> that, in the very same lines.
> 
> Setting offset to 0 to access the first element i.e. no offset required,
> and then adding the actual offset sizeof(s32), which could even be a
> const if the first access was to sensor_data[0], looks to verbose.
> 
> These things are of course not critical, and the proposed fix is
> definitely ok, but I am missing some consistency here.

Hi everyone!

So if you check also the conversations that we had here [1] and in the
previous versions, indeed the idea behind the offset is to use it as an
self-explanatory index to a char buffer that holds in fact s32 variables.

The data->buf here holds the values that have just been read from the
sensor. If you check on the channel specification of this sensor,
you will see ".realbits = 24" in both values that the sensor returns so
hence the value 3.

I am not sure if it makes sense to use a macro here for each one of the
3's that are going to be used only one time each and in order to be more
"consistent". But I might have a wrong view on this one so feel free to
correct me!

For the initialization of the offset indeed, it was already mentioned
here [2] this morning, but on a different patch!!! I couldn't get this
error though with gcc...

Cheers,
Vasilis

[1]: https://lore.kernel.org/all/20240930202353.38203-3-vassilisamir@gmail.com/
[2]: https://lore.kernel.org/linux-iio/202410111221.YIeXHxOv-lkp@intel.com/

Re: [PATCH v2] iio: Fix uninitialized variable

Posted by Javier Carrasco 1 month, 2 weeks ago

...

> 
> Hi everyone!
> 
> So if you check also the conversations that we had here [1] and in the
> previous versions, indeed the idea behind the offset is to use it as an
> self-explanatory index to a char buffer that holds in fact s32 variables.
> 
> The data->buf here holds the values that have just been read from the
> sensor. If you check on the channel specification of this sensor,
> you will see ".realbits = 24" in both values that the sensor returns so
> hence the value 3.
> 

So you are using 3 = 24 bits, but s32 not as 4 bytes? the whole thing
would have turned into sensor_data[0], sensor_data[4], and no variables
implied, correct? But I am discussing too much for something that in the
end is more or less the same, I am fine with this proposal.

> I am not sure if it makes sense to use a macro here for each one of the
> 3's that are going to be used only one time each and in order to be more
> "consistent". But I might have a wrong view on this one so feel free to
> correct me!
> 
> For the initialization of the offset indeed, it was already mentioned
> here [2] this morning, but on a different patch!!! I couldn't get this
> error though with gcc...
> 
> Cheers,
> Vasilis
> 

At least for the things I do in the kernel, clang catches more issues
than gcc. Sometimes even gcc will not complain, and clang will fail to
compile (e.g. a goto before a cleanup attribute).

And if you run smatch before sending the series, you might discover a
couple of extra "surprises".

Best regards,
Javier Carrasco

Re: [PATCH v2] iio: Fix uninitialized variable

Posted by Jonathan Cameron 1 month, 2 weeks ago

On Fri, 11 Oct 2024 21:32:38 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> ...
> 
> > 
> > Hi everyone!
> > 
> > So if you check also the conversations that we had here [1] and in the
> > previous versions, indeed the idea behind the offset is to use it as an
> > self-explanatory index to a char buffer that holds in fact s32 variables.
> > 
> > The data->buf here holds the values that have just been read from the
> > sensor. If you check on the channel specification of this sensor,
> > you will see ".realbits = 24" in both values that the sensor returns so
> > hence the value 3.
> >   
> 
> So you are using 3 = 24 bits, but s32 not as 4 bytes? the whole thing
> would have turned into sensor_data[0], sensor_data[4], and no variables
> implied, correct? But I am discussing too much for something that in the
> end is more or less the same, I am fine with this proposal.
Applied the fix.  Thanks,

Jonathan


> 
> > I am not sure if it makes sense to use a macro here for each one of the
> > 3's that are going to be used only one time each and in order to be more
> > "consistent". But I might have a wrong view on this one so feel free to
> > correct me!
> > 
> > For the initialization of the offset indeed, it was already mentioned
> > here [2] this morning, but on a different patch!!! I couldn't get this
> > error though with gcc...
> > 
> > Cheers,
> > Vasilis
> >   
> 
> At least for the things I do in the kernel, clang catches more issues
> than gcc. Sometimes even gcc will not complain, and clang will fail to
> compile (e.g. a goto before a cleanup attribute).
> 
> And if you run smatch before sending the series, you might discover a
> couple of extra "surprises".
> 
> Best regards,
> Javier Carrasco

Re: [PATCH v2] iio: Fix uninitialized variable

Posted by Yo-Jung Lin 1 month, 2 weeks ago

Hi Javier,

On Fri, Oct 11, 2024 at 8:31 PM Javier Carrasco
<javier.carrasco.cruz@gmail.com> wrote:
>
> On 11/10/2024 13:52, Yo-Jung (Leo) Lin wrote:
> > clang found that the "offset" in bmp580_trigger_handler doesn't get
> > initialized before access. Add proper initialization to this variable.
> >
> > Signed-off-by: Yo-Jung (Leo) Lin <0xff07@gmail.com>
> > ---
> > Change in v2:
> > - Make value initialization immediate before its first use.
> > - Link to v1: https://lore.kernel.org/all/20241011093752.30685-1-0xff07@gmail.com/
> >
> > ---
> >  drivers/iio/pressure/bmp280-core.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > index f4df222ed0c3..682329f81886 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -2222,6 +2222,8 @@ static irqreturn_t bmp580_trigger_handler(int irq, void *p)
> >               goto out;
> >       }
> >
> > +     offset = 0;
> > +
> >       /* Pressure calculations */
> >       memcpy(&data->sensor_data[offset], &data->buf[3], 3);
> >
>
> That was a quick reply. I would recommend you to wait a little bit while
> the first version is under discussion.

It was my bad not waiting for enough feedback to finalize another
patch. I’ll definitely do better next time.

I feel that initializing it as sizeof(s32) in the beginning and not
using it until the second memcpy() only widens the gap between value
initialization and its first access, which doesn’t address
Shevchenko’s concern.

>
> I still see the offset thing a bit weird. data->sensor_data uses an
> offset to avoid hard-coded numbers, but for data->buf we do exactly
> that, in the very same lines.
>
> Setting offset to 0 to access the first element i.e. no offset required,
> and then adding the actual offset sizeof(s32), which could even be a
> const if the first access was to sensor_data[0], looks to verbose.

While that is also true, it’ll take a more general refactoring to make
it driver-wise consistent. Maybe that refactoring should be on top of
a fix, instead of being part of a fix.

>
> These things are of course not critical, and the proposed fix is
> definitely ok, but I am missing some consistency here.

Best,
Leo