[PATCH][next] ACPI: thermal_lib: Add missing checks for errors in return code ret

[PATCH][next] ACPI: thermal_lib: Add missing checks for errors in return code ret

[Colin Ian King]

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);
 }
 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);
-- 
2.39.2

Re: [PATCH][next] ACPI: thermal_lib: Add missing checks for errors in return code ret

[Dan Carpenter]

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

Re: [PATCH][next] ACPI: thermal_lib: Add missing checks for errors in return code ret

[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

Re: [PATCH][next] ACPI: thermal_lib: Add missing checks for errors in return code ret

[Rafael J. Wysocki]

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?

Re: [PATCH][next] ACPI: thermal_lib: Add missing checks for errors in return code ret

[Colin King (gmail)]

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