[PATCH for-4.15 0/3] tools/libxl: -Og fixes for libxl__domain_get_device_model_uid()

[PATCH for-4.15 0/3] tools/libxl: -Og fixes for libxl__domain_get_device_model_uid()

[Andrew Cooper]

Split out of previous series, and split up for clarity.

Andrew Cooper (3):
  tools/libxl: Split out and err paths in libxl__domain_get_device_model_uid()
  tools/libxl: Simplfy the out path in libxl__domain_get_device_model_uid()
  tools/libxl: Rework invariants in libxl__domain_get_device_model_uid()

 tools/libs/light/libxl_dm.c | 58 ++++++++++++++++++++++++---------------------
 1 file changed, 31 insertions(+), 27 deletions(-)

-- 
2.11.0

[PATCH 1/3] tools/libxl: Split out and err paths in libxl__domain_get_device_model_uid()

[Andrew Cooper]

All paths with nonzero rc head to err.  All paths with a zero rc stay heading
towards out.

The comment discussing invariants is now arguably stale - it will be fixed in
one coherent manner when further fixes are in place.

No functional change.

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 | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
index db4cec6a76..30b3242e57 100644
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -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;
@@ -188,7 +188,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 +196,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,7 +222,7 @@ 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);
@@ -240,6 +240,7 @@ 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:
     /* First, do a root check if appropriate */
@@ -257,6 +258,7 @@ static int libxl__domain_get_device_model_uid(libxl__gc *gc,
             state->dm_kill_uid = GCSPRINTF("%ld", (long)intended_uid);
     }
 
+ err:
     return rc;
 }
 
-- 
2.11.0

[PATCH 2/3] tools/libxl: Simplfy the out path in libxl__domain_get_device_model_uid()

[Andrew Cooper]

All paths heading towards `out` have rc = 0.  Assert this property.

The intended_uid check is an error path.  Use the err label rather than
falling into subsequent success logic.

With the above two changes, the two `if (!rc)` checks can be dropped.

Now, both remaining tests start with `if (user ...)`.  Combine the two blocks.

No functional change, but far simpler logic to follow.

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 | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
index 30b3242e57..7843c283ca 100644
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -243,16 +243,17 @@ static int libxl__domain_get_device_model_uid(libxl__gc *gc,
     goto err;
 
 out:
-    /* First, do a root check if appropriate */
-    if (!rc) {
-        if (user && intended_uid == 0) {
+    assert(rc == 0);
+
+    if (user) {
+        /* First, do a root check if appropriate */
+        if (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) {
+        /* Then do the final set. */
         state->dm_runas = user;
         if (kill_by_uid)
             state->dm_kill_uid = GCSPRINTF("%ld", (long)intended_uid);
-- 
2.11.0

[PATCH 3/3] tools/libxl: Rework invariants in libxl__domain_get_device_model_uid()

[Andrew Cooper]

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.

Two paths unconditionally set user before checking for associated errors.
This interferes with the expected use of uninitialised-variable heuristics to
force compile failures for ill-formed exit paths.

Use b_info->device_model_user and LIBXL_QEMU_USER_SHARED as appliable, and
only set user on the goto out paths.

All goto out paths now are comprised of the form:
  user = NULL;
  rc = 0;

or:
  user = non-NULL;
  indended_uid = ...;
  kill_by_uid = ...;
  rc = 0;

As a consequence, indended_uid can drop its default of -1, the dm_restrict can
drop its now-stale "just in case" comment and the redundant setting of
kill_by_uid to work around this issue at other optimisation levels.

Finally, rewrite the comment about invariants, indicating the split between
the out and err lables, and associated rc values.  Additionally, reword the
"is {not,} set" terminology to "is {non-,}NULL" to be more precise.

No funcational change.

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 | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
index 7843c283ca..8a7e084d89 100644
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -127,7 +127,7 @@ static int libxl__domain_get_device_model_uid(libxl__gc *gc,
     struct passwd *user_base, user_pwbuf;
     int rc;
     char *user;
-    uid_t intended_uid = -1;
+    uid_t intended_uid;
     bool kill_by_uid;
 
     /* Only qemu-upstream can run as a different uid */
@@ -135,33 +135,34 @@ 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,
+     * rc = 0) or `err` (failure, rc != 0) 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.
-     * - if user is set, then intended_uid must be set to a UID matching
+     * - if rc is an error code, all settings are ignored.
+     * - if rc is 0, user may be NULL or non-NULL.
+     * - if user is non-NULL, then intended_uid must be set to a UID matching
      *   the username `user`, and kill_by_uid must be set to the appropriate
      *   value.  intended_uid will be checked for root (0).
      */
-    
+
     /*
      * If device_model_user is present, set `-runas` even if
      * dm_restrict isn't in use
      */
-    user = b_info->device_model_user;
-    if (user) {
-        rc = userlookup_helper_getpwnam(gc, user, &user_pwbuf, &user_base);
+    if (b_info->device_model_user) {
+        rc = userlookup_helper_getpwnam(gc, b_info->device_model_user,
+                                        &user_pwbuf, &user_base);
         if (rc)
             goto err;
 
         if (!user_base) {
             LOGD(ERROR, guest_domid, "Couldn't find device_model_user %s",
-                 user);
+                 b_info->device_model_user);
             rc = ERROR_INVAL;
             goto err;
         }
 
+        user = b_info->device_model_user;
         intended_uid = user_base->pw_uid;
         kill_by_uid = true;
         rc = 0;
@@ -175,8 +176,7 @@ static int libxl__domain_get_device_model_uid(libxl__gc *gc,
     if (!libxl_defbool_val(b_info->dm_restrict)) {
         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 */
+        user = NULL;
         rc = 0;
         goto out;
     }
@@ -219,13 +219,14 @@ static int libxl__domain_get_device_model_uid(libxl__gc *gc,
      * QEMU_USER_SHARED.  NB for QEMU_USER_SHARED, all QEMU will run
      * as the same UID, we can't kill by uid; therefore don't set uid.
      */
-    user = LIBXL_QEMU_USER_SHARED;
-    rc = userlookup_helper_getpwnam(gc, user, &user_pwbuf, &user_base);
+    rc = userlookup_helper_getpwnam(gc, LIBXL_QEMU_USER_SHARED,
+                                    &user_pwbuf, &user_base);
     if (rc)
         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);
+        user = LIBXL_QEMU_USER_SHARED;
         intended_uid = user_base->pw_uid;
         kill_by_uid = false;
         rc = 0;
-- 
2.11.0

Re: [PATCH 3/3] tools/libxl: Rework invariants in libxl__domain_get_device_model_uid()

[Ian Jackson]

Andrew Cooper writes ("[PATCH 3/3] tools/libxl: Rework invariants 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)
>         |            ^

Thanks for working on this.  I have reviewed your changes and I see
where you are coming from.  The situation is not very nice, mostly
because we don't have proper sum types in C.

I'm sorry to say that with my release manager hat on I think it is too
late for this kind of reorganisation for 4.15, especially just to work
around an overzealous compiler warning.

I think we can fix the compiler warning simply by setting the
`kill_by_uid` variable on more of the exit paths.  This approach was
already taken in this code for one of the paths.

I would prefer that approach at this stage of the release.

Sorry,
Ian.

Re: [PATCH 3/3] tools/libxl: Rework invariants in libxl__domain_get_device_model_uid()

[Andrew Cooper]

On 17/02/2021 17:50, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH 3/3] tools/libxl: Rework invariants 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)
>>         |            ^
> Thanks for working on this.  I have reviewed your changes and I see
> where you are coming from.  The situation is not very nice, mostly
> because we don't have proper sum types in C.
>
> I'm sorry to say that with my release manager hat on I think it is too
> late for this kind of reorganisation for 4.15, especially just to work
> around an overzealous compiler warning.
>
> I think we can fix the compiler warning simply by setting the
> `kill_by_uid` variable on more of the exit paths.  This approach was
> already taken in this code for one of the paths.
>
> I would prefer that approach at this stage of the release.

Well - I have explained why I'm not happy with that approach, but you
are the maintainer and RM.

I will trade you a minimal patch for formal R-b's so the time invested
so far fixing this mess isn't wasted when 4.16 opens.

~Andrew