diff mbox series

[04/10] tools/libxl: Fix uninitialised variable in libxl__domain_get_device_model_uid()

Message ID 20210212153953.4582-5-andrew.cooper3@citrix.com (mailing list archive)
State New
Headers show
Series tools: Support to use abi-dumper on libraries | expand

Commit Message

Andrew Cooper Feb. 12, 2021, 3:39 p.m. UTC
Various version of gcc, when compiling with -Og, complain:

  libxl_dm.c: In function 'libxl__domain_get_device_model_uid':
  libxl_dm.c:256:12: error: 'kill_by_uid' may be used uninitialized in this function [-Werror=maybe-uninitialized]
    256 |         if (kill_by_uid)
        |            ^

The logic is sufficiently complicated I can't figure out if the complain is
legitimate or not.  There is exactly one path wanting kill_by_uid set to true,
so default it to false and drop the existing workaround for this problem at
other optimisation levels.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libs/light/libxl_dm.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Ian Jackson Feb. 16, 2021, 4:07 p.m. UTC | #1
Andrew Cooper writes ("[PATCH 04/10] tools/libxl: Fix uninitialised variable in libxl__domain_get_device_model_uid()"):
> The logic is sufficiently complicated I can't figure out if the complain is
> legitimate or not.  There is exactly one path wanting kill_by_uid set to true,
> so default it to false and drop the existing workaround for this problem at
> other optimisation levels.

The place where it's used is here:

    if (!rc && user) {
            state->dm_runas = user;
                    if (kill_by_uid)
                                state->dm_kill_uid = GCSPRINTF("%ld",...
        
This is gated by !rc.  So for this to be used uninitialised, we'd have
to get here with rc==0 but uninitialised kill_by_uid.

The label `out` is preceded by a nonzero assignment to rc.

All the `goto out` are preceded by either (i) nonzero assignment to
rc, or (ii) assignment to kill_by_uid and setting rc=0.

So the compiler is wrong.

If only we had sum types.

In the absence of sum types I suggest the following restructuring:
Change all the `rc = ERROR...; goto out;` to `goto err` and make `goto
out` be the success path only.

Ian.
diff mbox series

Patch

diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
index 291dee9b3f..1ca21e4b81 100644
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -128,7 +128,7 @@  static int libxl__domain_get_device_model_uid(libxl__gc *gc,
     int rc;
     char *user;
     uid_t intended_uid = -1;
-    bool kill_by_uid;
+    bool kill_by_uid = false;
 
     /* Only qemu-upstream can run as a different uid */
     if (b_info->device_model_version != LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN)
@@ -176,7 +176,6 @@  static int libxl__domain_get_device_model_uid(libxl__gc *gc,
         LOGD(DEBUG, guest_domid,
              "dm_restrict disabled, starting QEMU as root");
         user = NULL; /* Should already be null, but just in case */
-        kill_by_uid = false; /* Keep older versions of gcc happy */
         rc = 0;
         goto out;
     }
@@ -227,7 +226,6 @@  static int libxl__domain_get_device_model_uid(libxl__gc *gc,
         LOGD(WARN, guest_domid, "Could not find user %s, falling back to %s",
              LIBXL_QEMU_USER_RANGE_BASE, LIBXL_QEMU_USER_SHARED);
         intended_uid = user_base->pw_uid;
-        kill_by_uid = false;
         rc = 0;
         goto out;
     }