| Message ID | 20240221123936.3421462-1-colin.i.king@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [next] ACPI: thermal_lib: Add missing checks for errors in return code ret | expand |
On Wed, Feb 21, 2024 at 12:39:36PM +0000, Colin Ian King wrote: > Static analysis with clang scan build detected various return codes > being assigned and not checked. The calls to the trip point functions > probably should be error return checked as these can fail because > of unlikely issues such as invalid ACPI object names or ACPI value > evaluation failures. > > Cleans up clang scan warnings, such as: > drivers/acpi/thermal_lib.c:106:9: warning: 2nd function call argument > is an uninitialized value [core.CallAndMessage] > return thermal_temp(ret, temp_decik, ret_temp); > > > Fixes: 6908097aa5a7 ("ACPI: thermal_lib: Add functions returning temperature in deci-Kelvin") > Signed-off-by: Colin Ian King <colin.i.king@gmail.com> > --- > > Note: Not tested. > > --- > drivers/acpi/thermal_lib.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/acpi/thermal_lib.c b/drivers/acpi/thermal_lib.c > index 4e0519ca9739..4d27048ef075 100644 > --- a/drivers/acpi/thermal_lib.c > +++ b/drivers/acpi/thermal_lib.c > @@ -103,6 +103,9 @@ int thermal_acpi_active_trip_temp(struct acpi_device *adev, int id, int *ret_tem > int temp_decik; > int ret = acpi_active_trip_temp(adev, id, &temp_decik); > > + if (ret) > + return ret; > + > return thermal_temp(ret, temp_decik, ret_temp); ^^^ The "ret" variable is checked at the start of the thermal_temp() function which means the code works the same before and after your patch. However, the static checker is correct that we are passing uninitialized data to this function, it's just never used if "ret" is non-zero. In this case, the rule is that if the function is parsed inline then it's not a bug, but if it's not inline then it is a bug. Technically, passing uninitialized data to a function is undefined behavior in C but Linus said that didn't really match with real life and that everyone should recognize that inlines are different and update the tools accordingly. (Something like that, I don't have the email in front of me). The other thing to consider is that tools like MEMSan (?) will see non-inline function calls and we're reading from uninitialized memory and trigger a warning for that. In this code, I suspect that thermal_temp() will be inlined so it's probably a false positive in Clang. regards, dan carpenter
Btw, in real life, in 2024, the compiler is going to automatically initialize "temp_decik" to zero. So we could just do: - int temp_decik; + int temp_decik = 0; That would silence the warning without affecting anything else at all. regards, dan carpenter
On Wed, Feb 21, 2024 at 2:19 PM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > Btw, in real life, in 2024, the compiler is going to automatically > initialize "temp_decik" to zero. So we could just do: > > - int temp_decik; > + int temp_decik = 0; > > That would silence the warning without affecting anything else at all. Sounds good to me. Anyone willing to cut a patch for this?
On 22/02/2024 10:21, Rafael J. Wysocki wrote: > On Wed, Feb 21, 2024 at 2:19 PM Dan Carpenter <dan.carpenter@linaro.org> wrote: >> >> Btw, in real life, in 2024, the compiler is going to automatically >> initialize "temp_decik" to zero. So we could just do: >> >> - int temp_decik; >> + int temp_decik = 0; >> >> That would silence the warning without affecting anything else at all. > > Sounds good to me. > > Anyone willing to cut a patch for this? will do
diff --git a/drivers/acpi/thermal_lib.c b/drivers/acpi/thermal_lib.c index 4e0519ca9739..4d27048ef075 100644 --- a/drivers/acpi/thermal_lib.c +++ b/drivers/acpi/thermal_lib.c @@ -103,6 +103,9 @@ int thermal_acpi_active_trip_temp(struct acpi_device *adev, int id, int *ret_tem int temp_decik; int ret = acpi_active_trip_temp(adev, id, &temp_decik); + if (ret) + return ret; + return thermal_temp(ret, temp_decik, ret_temp); } EXPORT_SYMBOL_GPL(thermal_acpi_active_trip_temp); @@ -122,6 +125,9 @@ int thermal_acpi_passive_trip_temp(struct acpi_device *adev, int *ret_temp) int temp_decik; int ret = acpi_passive_trip_temp(adev, &temp_decik); + if (ret) + return ret; + return thermal_temp(ret, temp_decik, ret_temp); } EXPORT_SYMBOL_GPL(thermal_acpi_passive_trip_temp); @@ -142,6 +148,9 @@ int thermal_acpi_hot_trip_temp(struct acpi_device *adev, int *ret_temp) int temp_decik; int ret = acpi_hot_trip_temp(adev, &temp_decik); + if (ret) + return ret; + return thermal_temp(ret, temp_decik, ret_temp); } EXPORT_SYMBOL_GPL(thermal_acpi_hot_trip_temp); @@ -161,6 +170,9 @@ int thermal_acpi_critical_trip_temp(struct acpi_device *adev, int *ret_temp) int temp_decik; int ret = acpi_critical_trip_temp(adev, &temp_decik); + if (ret) + return ret; + return thermal_temp(ret, temp_decik, ret_temp); } EXPORT_SYMBOL_GPL(thermal_acpi_critical_trip_temp);
Static analysis with clang scan build detected various return codes being assigned and not checked. The calls to the trip point functions probably should be error return checked as these can fail because of unlikely issues such as invalid ACPI object names or ACPI value evaluation failures. Cleans up clang scan warnings, such as: drivers/acpi/thermal_lib.c:106:9: warning: 2nd function call argument is an uninitialized value [core.CallAndMessage] return thermal_temp(ret, temp_decik, ret_temp); Fixes: 6908097aa5a7 ("ACPI: thermal_lib: Add functions returning temperature in deci-Kelvin") Signed-off-by: Colin Ian King <colin.i.king@gmail.com> --- Note: Not tested. --- drivers/acpi/thermal_lib.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)