Re: [PATCH v3] interface: fix udev_device_get_sysattr_value return value check

[Martin Kletzander]

On Tue, Sep 12, 2023 at 03:56:47PM +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.

v2: VIR_DEBUG added, replaced STREQ(NULLSTR()) with STREQ_NULLABLE()
v3: More checks added, to skip earlier. More verbose VIR_DEBUG.

Signed-off-by: Dmitry Frolov <fro...@swemel.ru>

Reviewed-by: Martin Kletzander <mklet...@redhat.com>

---
src/interface/interface_backend_udev.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/src/interface/interface_backend_udev.c 
b/src/interface/interface_backend_udev.c
index a0485ddd21..fb6799ed94 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -23,6 +23,7 @@
#include <dirent.h>
#include <libudev.h>

+#include "virlog.h"
#include "virerror.h"
#include "virfile.h"
#include "datatypes.h"
@@ -40,6 +41,8 @@

#define VIR_FROM_THIS VIR_FROM_INTERFACE

+VIR_LOG_INIT("interface.interface_backend_udev");
+
struct udev_iface_driver {
    struct udev *udev;
    /* pid file FD, ensures two copies of the driver can't use the same root */
@@ -354,11 +357,20 @@ udevConnectListAllInterfaces(virConnectPtr conn,
        const char *macaddr;
        g_autoptr(virInterfaceDef) def = NULL;

-        path = udev_list_entry_get_name(dev_entry);
-        dev = udev_device_new_from_syspath(udev, path);
-        name = udev_device_get_sysname(dev);
+        if (!(path = udev_list_entry_get_name(dev_entry))) {
+            VIR_DEBUG("Skipping interface, path == NULL");
+            continue;
+        }
+        if (!(dev = udev_device_new_from_syspath(udev, path))) {
+            VIR_DEBUG("Skipping interface '%s', dev == NULL", path);
+            continue;
+        }
+        if (!(name = udev_device_get_sysname(dev))) {
+            VIR_DEBUG("Skipping interface '%s', name == NULL", path);
+            continue;
+        }
        macaddr = udev_device_get_sysattr_value(dev, "address");
-        status = STREQ(udev_device_get_sysattr_value(dev, "operstate"), "up");
+        status = STREQ_NULLABLE(udev_device_get_sysattr_value(dev, "operstate"), 
"up");

        def = udevGetMinimalDefForDevice(dev);
        if (!virConnectListAllInterfacesCheckACL(conn, def)) {
@@ -964,9 +976,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 +1101,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_NULLABLE(udev_device_get_sysattr_value(dev, "operstate"), 
"up");

    udev_device_unref(dev);

--
2.34.1

signature.asc
Description: PGP signature