iio: Fix uninitialized variable

PATCH Superseded Yo-Jung (Leo) Lin 5 participants 9 messages Oct 11, 2024 – Oct 12, 2024 IIO

Share & tools

Yo-Jung (Leo) Lin

author

2024-10-11 09:40 UTC #0 lore raw

PATCH 1/1 iio: Fix uninitialized variable

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 <[email protected]>

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

drivers/iio/pressure/bmp280-core.c +1 -1

index f4df222ed0c3..7d4151826b86 100644
@@ -2210,7 +2210,7 @@ static irqreturn_t bmp580_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct bmp280_data *data = iio_priv(indio_dev);
-	int ret, offset;
+	int ret, offset = 0;
 
 	guard(mutex)(&data->lock);

Signature · 1 line

2.43.0

Andy Shevchenko

2024-10-11 10:54 UTC #1 lore raw

Quoted 2 lines · Yo-Jung (Leo) Lin

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

...

drivers/iio/pressure/bmp280-core.c static irqreturn_t bmp580_trigger_handler(int irq, void *p) · line 2213

Quoted 3 lines

drivers/iio/pressure/bmp280-core.c +1 -1

 	struct bmp280_data *data = iio_priv(indio_dev);
-	int ret, offset;
+	int ret, offset = 0;

Can it be done closer to the actual user of it?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Javier Carrasco

2024-10-11 11:03 UTC #2 lore raw

Quoted 4 lines · Andy Shevchenko

Quoted 2 lines · Yo-Jung (Leo) Lin
clang found that the "offset" in bmp580_trigger_handler doesn't get initialized before access. Add proper initialization to this variable.

Quoted 9 lines

...
drivers/iio/pressure/bmp280-core.c static irqreturn_t bmp580_trigger_handler(int irq, void *p) · line 2213
Quoted 3 lines
drivers/iio/pressure/bmp280-core.c +1 -1
 	struct bmp280_data *data = iio_priv(indio_dev);
-	int ret, offset;
+	int ret, offset = 0;
Can it be done closer to the actual user of it?

Actually, offset could be initialized to sizeof(32), and only used for the temperature calculations.

+ int ret, offset = sizeof(s32);

The first memcpy would use 0 as index, as it did before.

Best regards,
Javier Carrasco

Yo-Jung (Leo) Lin

author

2024-10-11 11:54 UTC #3 lore raw

PATCH v2 1/1 iio: Fix uninitialized variable

In reply to Yo-Jung (Leo) Lin

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 <[email protected]>

---
Change in v2:
- Make value initialization immediate before its first use.
- Link to v1: https://lore.kernel.org/all/[email protected]/

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

drivers/iio/pressure/bmp280-core.c +2 -0

index f4df222ed0c3..682329f81886 100644
@@ -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);

Signature · 1 line

2.43.0

Javier Carrasco

2024-10-11 12:31 UTC #4 lore raw

Quoted 18 lines · Yo-Jung (Leo) Lin

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 <[email protected]>
---
Change in v2:
- Make value initialization immediate before its first use.
- Link to v1: https://lore.kernel.org/all/[email protected]/
---
 drivers/iio/pressure/bmp280-core.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/iio/pressure/bmp280-core.c +0 -0
index f4df222ed0c3..682329f81886 100644

drivers/iio/pressure/bmp280-core.c static irqreturn_t bmp580_trigger_handler(int irq, void *p) · line 2229

Quoted 8 lines

drivers/iio/pressure/bmp280-core.c +2 -0

@@ -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.

Yo-Jung Lin

author

2024-10-11 15:01 UTC #5 lore raw

Hi Javier,

Quoted 23 lines · Javier Carrasco

Quoted 21 lines · Yo-Jung (Leo) Lin
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 <[email protected]>
---
Change in v2:
- Make value initialization immediate before its first use.
- Link to v1: https://lore.kernel.org/all/[email protected]/
---
 drivers/iio/pressure/bmp280-core.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/iio/pressure/bmp280-core.c +0 -0
index f4df222ed0c3..682329f81886 100644
@@ -2222,6 +2222,8 @@ static irqreturn_t bmp580_trigger_handler(int irq, void *p)
              goto out;
      }
 

Quoted 8 lines

drivers/iio/pressure/bmp280-core.c static irqreturn_t bmp580_trigger_handler(int irq, void *p) · line 2229
Quoted 5 lines
drivers/iio/pressure/bmp280-core.c +2 -0
+     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.

Quoted 8 lines

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.

Quoted 3 lines

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

Best,
Leo

Vasileios Aoiridis

2024-10-11 18:32 UTC #6 lore raw

In reply to Javier Carrasco

Quoted 35 lines · Javier Carrasco

Quoted 18 lines · Yo-Jung (Leo) Lin
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 <[email protected]>
---
Change in v2:
- Make value initialization immediate before its first use.
- Link to v1: https://lore.kernel.org/all/[email protected]/
---
 drivers/iio/pressure/bmp280-core.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/iio/pressure/bmp280-core.c +0 -0
index f4df222ed0c3..682329f81886 100644
drivers/iio/pressure/bmp280-core.c static irqreturn_t bmp580_trigger_handler(int irq, void *p) · line 2229
Quoted 8 lines
drivers/iio/pressure/bmp280-core.c +2 -0
@@ -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.

Quoted 6 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/[email protected]/
[2]: https://lore.kernel.org/linux-iio/[email protected]/

Javier Carrasco

2024-10-11 19:32 UTC #7 lore raw

...

Quoted 12 lines

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.

Quoted 5 lines

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!

Quoted 7 lines

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

Jonathan Cameron

maintainer

2024-10-12 10:50 UTC #8 lore raw

Quoted 15 lines · On Fri, 11 Oct 2024 21:32:38 +0200 Javier Carrasco…

...
Quoted 12 lines
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.

Quoted 4 lines

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

Quoted 23 lines

Quoted 6 lines
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!
Quoted 7 lines
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