diff mbox series

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

Message ID 20210217164251.11005-4-andrew.cooper3@citrix.com (mailing list archive)
State New
Headers show
Series tools/libxl: -Og fixes for libxl__domain_get_device_model_uid() | expand

Commit Message

Andrew Cooper Feb. 17, 2021, 4:42 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 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(-)

Comments

Ian Jackson Feb. 17, 2021, 5:50 p.m. UTC | #1
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.
Andrew Cooper Feb. 17, 2021, 6:13 p.m. UTC | #2
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
diff mbox series

Patch

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;