Message ID | 20210212153953.4582-5-andrew.cooper3@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | tools: Support to use abi-dumper on libraries | expand |
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 --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; }
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(-)