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

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

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 <frolov@xxxxxxxxx>

Reviewed-by: Martin Kletzander <mkletzan@xxxxxxxxxx>

> ---
> 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

Attachment: signature.asc
Description: PGP signature