src/interface/interface_backend_udev.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
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.
© 2016 - 2024 Red Hat, Inc.