Re: [libvirt PATCH 06/11] udevGetIntSysfsAttr: Return -1 for missing attributes
On Thu, 2021-01-28 at 11:44 +0100, Peter Krempa wrote:
On Thu, Jan 28, 2021 at 11:24:36 +0100, Tim Wiederhake wrote:
> If "udevGetDeviceSysfsAttr()" returns NULL,
"udevGetIntSysfsAttr"
> would return "0", indicating success, without writing to
"value".
>
> This was found by clang-tidy's
> "clang-analyzer-core.UndefinedBinaryOperatorResult" check in
> function "udevProcessCCW", flagging a read on the potentially
> uninitialized variable "online".
>
> Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
> ---
> src/node_device/node_device_udev.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/src/node_device/node_device_udev.c
> b/src/node_device/node_device_udev.c
> index 55a2731681..d5a12bab0e 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -254,7 +254,10 @@ udevGetIntSysfsAttr(struct udev_device
> *udev_device,
>
> str = udevGetDeviceSysfsAttr(udev_device, attr_name);
>
> - if (str && virStrToLong_i(str, NULL, base, value) < 0) {
> + if (!str)
> + return -1;
In this case an error wouldn't be reported any more.
> +
> + if (virStrToLong_i(str, NULL, base, value) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to convert '%s' to int"), str);
while here it was
I believe this is not correct. Here is how the code looked before:
(1) str = udevGetDeviceSysfsAttr(udev_device, attr_name);
(2) if (str && virStrToLong_i(str, NULL, base, value) < 0) {
(3) virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to convert '%s' to int"), str);
return -1;
}
return 0;
If "udevGetDeviceSysfsAttr" returns NULL in line (1), the (non-
existant) "false" branch of line (2) is taken, i.e. the virReportError
in line (3) is not executed.
In the new version of the code:
(4) str = udevGetDeviceSysfsAttr(udev_device, attr_name);
(5) if (!str)
return -1;
if (virStrToLong_i(str, NULL, base, value) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to convert '%s' to int"), str);
return -1;
}
return 0;
If "udevGetDeviceSysfsAttr" returns NULL in line (4), this is caught in
line (5), still not reporting an error message, but returning with an
appropriate return value. Reporting the error is performed higher up in
the call chain, e.g.:
* udevAddOneDevice ← reports the issue in "cleanup" block
* udevGetDeviceDetails
* udevProcessCCW
* udevGetIntSysfsAttr ← now returns -1 for missing attributes
Cheers,
Tim