[PATCH] interface: fix udev_device_get_sysattr_value return value check

Reviewing the code I found that return value of function udev_device_get_sysattr_value() is dereferenced without a check. udev_device_get_sysattr_value() may return NULL by number of reasons. Signed-off-by: Dmitry Frolov <frolov@swemel.ru> --- src/interface/interface_backend_udev.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index a0485ddd21..c820b3ccdf 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -355,10 +355,11 @@ udevConnectListAllInterfaces(virConnectPtr conn, g_autoptr(virInterfaceDef) def = NULL; path = udev_list_entry_get_name(dev_entry); - dev = udev_device_new_from_syspath(udev, path); + if (!(dev = udev_device_new_from_syspath(udev, path))) + continue; name = udev_device_get_sysname(dev); macaddr = udev_device_get_sysattr_value(dev, "address"); - status = STREQ(udev_device_get_sysattr_value(dev, "operstate"), "up"); + status = STREQ(NULLSTR(udev_device_get_sysattr_value(dev, "operstate")), "up"); def = udevGetMinimalDefForDevice(dev); if (!virConnectListAllInterfacesCheckACL(conn, def)) { @@ -964,9 +965,9 @@ udevGetIfaceDef(struct udev *udev, const char *name) /* MTU */ mtu_str = udev_device_get_sysattr_value(dev, "mtu"); - if (virStrToLong_ui(mtu_str, NULL, 10, &mtu) < 0) { + if (!mtu_str || virStrToLong_ui(mtu_str, NULL, 10, &mtu) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not parse MTU value '%1$s'"), mtu_str); + _("Could not parse MTU value '%1$s'"), NULLSTR(mtu_str)); goto error; } ifacedef->mtu = mtu; @@ -1089,7 +1090,7 @@ udevInterfaceIsActive(virInterfacePtr ifinfo) goto cleanup; /* Check if it's active or not */ - status = STREQ(udev_device_get_sysattr_value(dev, "operstate"), "up"); + status = STREQ(NULLSTR(udev_device_get_sysattr_value(dev, "operstate")), "up"); udev_device_unref(dev); -- 2.34.1

On Fri, Sep 08, 2023 at 13:06:43 +0300, Dmitry Frolov wrote:
Reviewing the code I found that return value of function udev_device_get_sysattr_value() is dereferenced without a check. udev_device_get_sysattr_value() may return NULL by number of reasons.
Signed-off-by: Dmitry Frolov <frolov@swemel.ru> --- src/interface/interface_backend_udev.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index a0485ddd21..c820b3ccdf 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -355,10 +355,11 @@ udevConnectListAllInterfaces(virConnectPtr conn, g_autoptr(virInterfaceDef) def = NULL;
path = udev_list_entry_get_name(dev_entry); - dev = udev_device_new_from_syspath(udev, path); + if (!(dev = udev_device_new_from_syspath(udev, path))) + continue;
IMO this warants at least a VIR_DEBUG stating that the returned 'dev' struct is NULL and thus we're skipping a whole interface.
name = udev_device_get_sysname(dev);
This is also documented as returning NULL on error, but virGetInterface actually checks it before use.
macaddr = udev_device_get_sysattr_value(dev, "address"); - status = STREQ(udev_device_get_sysattr_value(dev, "operstate"), "up"); + status = STREQ(NULLSTR(udev_device_get_sysattr_value(dev, "operstate")), "up");
For comparing strings which can be NULL we have STREQ_NULLABLE and STRNEQ_NULLABLE.
participants (2)
-
Dmitry Frolov
-
Peter Krempa