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

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;
     }
-- 
2.11.0

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.

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 very tangled.  Split the out and err labels apart, using out for
success cases only.

assert() that rc is 0 in the success case.  This allows for the removal of the
`if (!rc)` nesting in the out path, which reduces the cyclomatic complexity,
which is the root cause of false positive maybe-uninitialised warnings.

This also allows for dropping of the two paths setting kill_by_uid to false to
work around this warning 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>

v2:
 * Split the out and err paths.
---
 tools/libs/light/libxl_dm.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
index 291dee9b3f..dbd2ab607d 100644
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -135,8 +135,8 @@ static int libxl__domain_get_device_model_uid(libxl__gc *gc,
         return 0;
 
     /*
-     * From this point onward, all paths should go through the `out`
-     * label.  The invariants should be:
+     * From this point onward, all paths should go through the `out` (success)
+     * or `err` (failure) labels.  The invariants should be:
      * - rc may be 0, or an error code.
      * - if rc is an error code, user and intended_uid are ignored.
      * - if rc is 0, user may be set or not set.
@@ -153,13 +153,13 @@ static int libxl__domain_get_device_model_uid(libxl__gc *gc,
     if (user) {
         rc = userlookup_helper_getpwnam(gc, user, &user_pwbuf, &user_base);
         if (rc)
-            goto out;
+            goto err;
 
         if (!user_base) {
             LOGD(ERROR, guest_domid, "Couldn't find device_model_user %s",
                  user);
             rc = ERROR_INVAL;
-            goto out;
+            goto err;
         }
 
         intended_uid = user_base->pw_uid;
@@ -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;
     }
@@ -188,7 +187,7 @@ static int libxl__domain_get_device_model_uid(libxl__gc *gc,
     rc = userlookup_helper_getpwnam(gc, LIBXL_QEMU_USER_RANGE_BASE,
                                          &user_pwbuf, &user_base);
     if (rc)
-        goto out;
+        goto err;
     if (user_base) {
         struct passwd *user_clash, user_clash_pwbuf;
 
@@ -196,14 +195,14 @@ static int libxl__domain_get_device_model_uid(libxl__gc *gc,
         rc = userlookup_helper_getpwuid(gc, intended_uid,
                                          &user_clash_pwbuf, &user_clash);
         if (rc)
-            goto out;
+            goto err;
         if (user_clash) {
             LOGD(ERROR, guest_domid,
                  "wanted to use uid %ld (%s + %d) but that is user %s !",
                  (long)intended_uid, LIBXL_QEMU_USER_RANGE_BASE,
                  guest_domid, user_clash->pw_name);
             rc = ERROR_INVAL;
-            goto out;
+            goto err;
         }
 
         LOGD(DEBUG, guest_domid, "using uid %ld", (long)intended_uid);
@@ -222,12 +221,11 @@ static int libxl__domain_get_device_model_uid(libxl__gc *gc,
     user = LIBXL_QEMU_USER_SHARED;
     rc = userlookup_helper_getpwnam(gc, user, &user_pwbuf, &user_base);
     if (rc)
-        goto out;
+        goto err;
     if (user_base) {
         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;
     }
@@ -240,23 +238,26 @@ static int libxl__domain_get_device_model_uid(libxl__gc *gc,
          "Could not find user %s or range base pseudo-user %s, cannot restrict",
          LIBXL_QEMU_USER_SHARED, LIBXL_QEMU_USER_RANGE_BASE);
     rc = ERROR_INVAL;
+    goto err;
 
 out:
+    assert(rc == 0);
+
     /* First, do a root check if appropriate */
-    if (!rc) {
-        if (user && intended_uid == 0) {
-            LOGD(ERROR, guest_domid, "intended_uid is 0 (root)!");
-            rc = ERROR_INVAL;
-        }
+    if (user && intended_uid == 0) {
+        LOGD(ERROR, guest_domid, "intended_uid is 0 (root)!");
+        rc = ERROR_INVAL;
+        goto err;
     }
 
     /* Then do the final set, if still appropriate */
-    if (!rc && user) {
+    if (user) {
         state->dm_runas = user;
         if (kill_by_uid)
             state->dm_kill_uid = GCSPRINTF("%ld", (long)intended_uid);
     }
 
+ err:
     return rc;
 }
 
-- 
2.11.0

Andrew Cooper writes ("[PATCH v2 04/10] tools/libxl: Fix uninitialised variable in libxl__domain_get_device_model_uid()"):
> 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)
...
> @@ -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;

Uhhhhh.  I think this and the other one seem to be stray hunks which
each introduce a new uninitialised variable bug ?

Isn.

Ian Jackson writes ("Re: [PATCH v2 04/10] tools/libxl: Fix uninitialised variable in libxl__domain_get_device_model_uid()"):
> Uhhhhh.  I think this and the other one seem to be stray hunks which
> each introduce a new uninitialised variable bug ?

I now think (following irc discussion) that I was wrong about this.

Use of intended_uid is conditional on user being set.  This is very
confusing.  This code is simulating a sum type.  If only we had sum
types.

Reviewed-by: Ian Jackson <iwj@xenproject.org>

I think, given how confusing this is, that I would like another
careful review before I give my release-ack.

Ian.