[PATCH for-4.15 00/10] tools: Support to use abi-dumper on libraries

[PATCH for-4.15 00/10] tools: Support to use abi-dumper on libraries

[Andrew Cooper]

The first 6 patches are fixes to build with the -Og optimisation level, which
is an expectation of abi-dumper and a good idea generally.  There are 2
definite bugfixes, and 4 of more questionable usefulness.  All fixes are
simple.

The next patch switches to -Og by default.  This is potentially risky - I've
fixed up all build failures that Gitlab CI can spot, but I don't guarentee
that I've fixed all of them.  However, it only affects debug builds - release
builds are unchanged, and we're before -RC1 so have plenty of time to react to
any fallout.

The final 3 patches arrange for abi-dumper to run if it is available in the
build environment.  This is strictly optional, has no effect if abi-dumper
isn't in the build environment, and writes out one extra file if present.


With this tooling place, we can now add support to OSSTest to check for ABI
breakges in builds.

Andrew Cooper (10):
  tools/xl: Fix exit code for `xl vkbattach`
  tools/libxg: Fix uninitialised variable in write_x86_cpu_policy_records()
  tools/libxg: Fix uninitialised variable in meminit()
  tools/libxl: Fix uninitialised variable in libxl__domain_get_device_model_uid()
  tools/libxl: Fix uninitialised variable in libxl__write_stub_dmargs()
  stubdom/xenstored: Fix uninitialised variables in lu_read_state()
  tools: Use -Og for debug builds when available
  tools: Check for abi-dumper in ./configure
  tools/libs: Add rule to generate headers.lst
  tools/libs: Write out an ABI analysis when abi-dumper is available

 config/Tools.mk.in                  |  1 +
 tools/Rules.mk                      |  5 +++--
 tools/configure                     | 41 +++++++++++++++++++++++++++++++++++++
 tools/configure.ac                  |  1 +
 tools/libs/.gitignore               |  1 +
 tools/libs/guest/xg_dom_arm.c       |  2 +-
 tools/libs/guest/xg_sr_common_x86.c |  6 ++++++
 tools/libs/libs.mk                  | 18 +++++++++++++++-
 tools/libs/light/libxl_dm.c         |  6 ++----
 tools/xenstore/xenstored_control.c  |  2 +-
 tools/xl/xl_vkb.c                   |  3 ++-
 11 files changed, 76 insertions(+), 10 deletions(-)
 create mode 100644 tools/libs/.gitignore

-- 
2.11.0

[PATCH 01/10] tools/xl: Fix exit code for `xl vkbattach`

[Andrew Cooper]

Various version of gcc, when compiling with -Og, complain:

  xl_vkb.c: In function 'main_vkbattach':
  xl_vkb.c:79:12: error: 'rc' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     79 |     return rc;
        |            ^~

The dryrun_only path really does leave rc uninitalised.  Introduce a done
label for success paths to use.

Fixes: a15166af7c3 ("xl: add vkb config parser and CLI")
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/xl/xl_vkb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/xl/xl_vkb.c b/tools/xl/xl_vkb.c
index f6ed9e05ee..728ac9470b 100644
--- a/tools/xl/xl_vkb.c
+++ b/tools/xl/xl_vkb.c
@@ -64,7 +64,7 @@ int main_vkbattach(int argc, char **argv)
         char *json = libxl_device_vkb_to_json(ctx, &vkb);
         printf("vkb: %s\n", json);
         free(json);
-        goto out;
+        goto done;
     }
 
     if (libxl_device_vkb_add(ctx, domid, &vkb, 0)) {
@@ -72,6 +72,7 @@ int main_vkbattach(int argc, char **argv)
         rc = ERROR_FAIL; goto out;
     }
 
+done:
     rc = 0;
 
 out:
-- 
2.11.0

[PATCH 02/10] tools/libxg: Fix uninitialised variable in write_x86_cpu_policy_records()

[Andrew Cooper]

Various version of gcc, when compiling with -Og, complain:

  xg_sr_common_x86.c: In function 'write_x86_cpu_policy_records':
  xg_sr_common_x86.c:92:12: error: 'rc' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     92 |     return rc;
        |            ^~

The complaint is legitimate, and can occur with unexpected behaviour of two
related hypercalls in combination with a libc which permits zero-length
malloc()s.

Have an explicit rc = 0 on the success path, and make the MSRs record error
handling consistent with the CPUID record before it.

Fixes: f6b2b8ec53d ("libxc/save: Write X86_{CPUID,MSR}_DATA records")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
---
 tools/libs/guest/xg_sr_common_x86.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/libs/guest/xg_sr_common_x86.c b/tools/libs/guest/xg_sr_common_x86.c
index 6f12483907..3168c5485f 100644
--- a/tools/libs/guest/xg_sr_common_x86.c
+++ b/tools/libs/guest/xg_sr_common_x86.c
@@ -83,7 +83,13 @@ int write_x86_cpu_policy_records(struct xc_sr_context *ctx)
 
     msrs.length = nr_msrs * sizeof(xen_msr_entry_t);
     if ( msrs.length )
+    {
         rc = write_record(ctx, &msrs);
+        if ( rc )
+            goto out;
+    }
+
+    rc = 0;
 
  out:
     free(cpuid.data);
-- 
2.11.0

[PATCH 03/10] tools/libxg: Fix uninitialised variable in meminit()

[Andrew Cooper]

Various version of gcc, when compiling with -Og, complain:

  xg_dom_arm.c: In function 'meminit':
  xg_dom_arm.c:420:19: error: 'p2m_size' may be used uninitialized in this function [-Werror=maybe-uninitialized]
    420 |     dom->p2m_size = p2m_size;
        |     ~~~~~~~~~~~~~~^~~~~~~~~~

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>

Julien/Stefano: I can't work out how this variable is supposed to work, and
the fact that it isn't a straight accumulation across the RAM banks looks
suspect.
---
 tools/libs/guest/xg_dom_arm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libs/guest/xg_dom_arm.c b/tools/libs/guest/xg_dom_arm.c
index 94948d2b20..f1b8d06f75 100644
--- a/tools/libs/guest/xg_dom_arm.c
+++ b/tools/libs/guest/xg_dom_arm.c
@@ -373,7 +373,7 @@ static int meminit(struct xc_dom_image *dom)
     const uint64_t modsize = dtb_size + ramdisk_size;
     const uint64_t ram128mb = bankbase[0] + (128<<20);
 
-    xen_pfn_t p2m_size;
+    xen_pfn_t p2m_size = 0;
     uint64_t bank0end;
 
     assert(dom->rambase_pfn << XC_PAGE_SHIFT == bankbase[0]);
-- 
2.11.0

[PATCH 04/10] tools/libxl: Fix uninitialised variable 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 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

[PATCH 05/10] tools/libxl: Fix uninitialised variable in libxl__write_stub_dmargs()

[Andrew Cooper]

Various version of gcc, when compiling with -Og, complain:

  libxl_dm.c: In function ‘libxl__write_stub_dmargs’:
  libxl_dm.c:2166:16: error: ‘dmargs’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
               rc = libxl__xs_write_checked(gc, t, path, dmargs);
               ~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

It can't, but only because of how the is_linux_stubdom checks line up.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
index 1ca21e4b81..7bbb8792ea 100644
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -2099,7 +2099,7 @@ static int libxl__write_stub_dmargs(libxl__gc *gc,
 {
     struct xs_permissions roperm[2];
     xs_transaction_t t = XBT_NULL;
-    char *dmargs;
+    char *dmargs = NULL;
     int rc;
 
     roperm[0].id = 0;
-- 
2.11.0

[PATCH 06/10] stubdom/xenstored: Fix uninitialised variables in lu_read_state()

[Andrew Cooper]

Various version of gcc, when compiling with -Og, complain:

  xenstored_control.c: In function ‘lu_read_state’:
  xenstored_control.c:540:11: error: ‘state.size’ is used uninitialized in this
  function [-Werror=uninitialized]
    if (state.size == 0)
        ~~~~~^~~~~
  xenstored_control.c:543:6: error: ‘state.buf’ may be used uninitialized in
  this function [-Werror=maybe-uninitialized]
    pre = state.buf;
    ~~~~^~~~~~~~~~~
  xenstored_control.c:550:23: error: ‘state.buf’ may be used uninitialized in
  this function [-Werror=maybe-uninitialized]
     (void *)head - state.buf < state.size;
                    ~~~~~^~~~
  xenstored_control.c:550:35: error: ‘state.size’ may be used uninitialized in
  this function [-Werror=maybe-uninitialized]
     (void *)head - state.buf < state.size;
                                ~~~~~^~~~~

Interestingly, this is only in the stubdom build.  I can't identify any
relevant differences vs the regular tools build.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
CC: Juergen Gross <jgross@suse.com>
---
 tools/xenstore/xenstored_control.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c
index 1f733e0a04..f10beaf85e 100644
--- a/tools/xenstore/xenstored_control.c
+++ b/tools/xenstore/xenstored_control.c
@@ -530,7 +530,7 @@ static const char *lu_dump_state(const void *ctx, struct connection *conn)
 
 void lu_read_state(void)
 {
-	struct lu_dump_state state;
+	struct lu_dump_state state = {};
 	struct xs_state_record_header *head;
 	void *ctx = talloc_new(NULL); /* Work context for subfunctions. */
 	struct xs_state_preamble *pre;
-- 
2.11.0

[PATCH 07/10] tools: Use -Og for debug builds when available

[Andrew Cooper]

The recommended optimisation level for debugging is -Og, and is what tools
such as gdb prefer.  In practice, it equates to -01 with a few specific
optimisations turned off.

abi-dumper in particular wants the libraries it inspects in this form.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
CC: Juergen Gross <jgross@suse.com>
---
 tools/Rules.mk | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/Rules.mk b/tools/Rules.mk
index f61da81f4a..2907ed2d39 100644
--- a/tools/Rules.mk
+++ b/tools/Rules.mk
@@ -106,8 +106,9 @@ endif
 CFLAGS_libxenlight += $(CFLAGS_libxenctrl)
 
 ifeq ($(debug),y)
-# Disable optimizations
-CFLAGS += -O0 -fno-omit-frame-pointer
+# Use -Og if available, -O0 otherwise
+dbg_opt_level := $(call cc-option,$(CC),-Og,-O0)
+CFLAGS += $(dbg_opt_level) -fno-omit-frame-pointer
 # But allow an override to -O0 in case Python enforces -D_FORTIFY_SOURCE=<n>.
 PY_CFLAGS += $(PY_NOOPT_CFLAGS)
 else
-- 
2.11.0

[PATCH 08/10] tools: Check for abi-dumper in ./configure

[Andrew Cooper]

This will be optional.  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: Juergen Gross <jgross@suse.com>
---
 config/Tools.mk.in |  1 +
 tools/configure    | 41 +++++++++++++++++++++++++++++++++++++++++
 tools/configure.ac |  1 +
 3 files changed, 43 insertions(+)

diff --git a/config/Tools.mk.in b/config/Tools.mk.in
index 48bd9ab731..d47936686b 100644
--- a/config/Tools.mk.in
+++ b/config/Tools.mk.in
@@ -19,6 +19,7 @@ BCC                 := @BCC@
 IASL                := @IASL@
 AWK                 := @AWK@
 FETCHER             := @FETCHER@
+ABI_DUMPER          := @ABI_DUMPER@
 
 # Extra folder for libs/includes
 PREPEND_INCLUDES    := @PREPEND_INCLUDES@
diff --git a/tools/configure b/tools/configure
index e63ca3797d..bb5acf9d43 100755
--- a/tools/configure
+++ b/tools/configure
@@ -682,6 +682,7 @@ OCAMLOPT
 OCAMLLIB
 OCAMLVERSION
 OCAMLC
+ABI_DUMPER
 INSTALL_DATA
 INSTALL_SCRIPT
 INSTALL_PROGRAM
@@ -5442,6 +5443,46 @@ $as_echo "no" >&6; }
 fi
 
 
+# Extract the first word of "abi-dumper", so it can be a program name with args.
+set dummy abi-dumper; ac_word=$2
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for $ac_word" >&5
+$as_echo_n "checking for $ac_word... " >&6; }
+if ${ac_cv_path_ABI_DUMPER+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  case $ABI_DUMPER in
+  [\\/]* | ?:[\\/]*)
+  ac_cv_path_ABI_DUMPER="$ABI_DUMPER" # Let the user override the test with a path.
+  ;;
+  *)
+  as_save_IFS=$IFS; IFS=$PATH_SEPARATOR
+for as_dir in $PATH
+do
+  IFS=$as_save_IFS
+  test -z "$as_dir" && as_dir=.
+    for ac_exec_ext in '' $ac_executable_extensions; do
+  if as_fn_executable_p "$as_dir/$ac_word$ac_exec_ext"; then
+    ac_cv_path_ABI_DUMPER="$as_dir/$ac_word$ac_exec_ext"
+    $as_echo "$as_me:${as_lineno-$LINENO}: found $as_dir/$ac_word$ac_exec_ext" >&5
+    break 2
+  fi
+done
+  done
+IFS=$as_save_IFS
+
+  ;;
+esac
+fi
+ABI_DUMPER=$ac_cv_path_ABI_DUMPER
+if test -n "$ABI_DUMPER"; then
+  { $as_echo "$as_me:${as_lineno-$LINENO}: result: $ABI_DUMPER" >&5
+$as_echo "$ABI_DUMPER" >&6; }
+else
+  { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
+$as_echo "no" >&6; }
+fi
+
+
 # Extract the first word of "perl", so it can be a program name with args.
 set dummy perl; ac_word=$2
 { $as_echo "$as_me:${as_lineno-$LINENO}: checking for $ac_word" >&5
diff --git a/tools/configure.ac b/tools/configure.ac
index 6b611deb13..636e7077be 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -310,6 +310,7 @@ AC_PROG_CC
 AC_PROG_MAKE_SET
 AC_PROG_INSTALL
 AC_PATH_PROG([FLEX], [flex])
+AC_PATH_PROG([ABI_DUMPER], [abi-dumper])
 AX_PATH_PROG_OR_FAIL([PERL], [perl])
 AX_PATH_PROG_OR_FAIL([AWK], [awk])
 
-- 
2.11.0

[PATCH 09/10] tools/libs: Add rule to generate headers.lst

[Andrew Cooper]

abi-dumper needs a list of the public header files for shared objects, and
only accepts this in the form of a file.

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: Juergen Gross <jgross@suse.com>
---
 tools/libs/.gitignore | 1 +
 tools/libs/libs.mk    | 9 ++++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)
 create mode 100644 tools/libs/.gitignore

diff --git a/tools/libs/.gitignore b/tools/libs/.gitignore
new file mode 100644
index 0000000000..4a13126144
--- /dev/null
+++ b/tools/libs/.gitignore
@@ -0,0 +1 @@
+*/headers.lst
diff --git a/tools/libs/libs.mk b/tools/libs/libs.mk
index 0b3381755a..ac68996ab2 100644
--- a/tools/libs/libs.mk
+++ b/tools/libs/libs.mk
@@ -76,6 +76,10 @@ endif
 
 headers.chk: $(AUTOINCS)
 
+headers.lst: FORCE
+	@{ $(foreach h,$(LIBHEADERS),echo $(h);) } > $@.tmp
+	@$(call move-if-changed,$@.tmp,$@)
+
 libxen$(LIBNAME).map:
 	echo 'VERS_$(MAJOR).$(MINOR) { global: *; };' >$@
 
@@ -118,9 +122,12 @@ TAGS:
 clean:
 	rm -rf *.rpm $(LIB) *~ $(DEPS_RM) $(LIB_OBJS) $(PIC_OBJS)
 	rm -f lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR) lib$(LIB_FILE_NAME).so.$(MAJOR)
-	rm -f headers.chk
+	rm -f headers.chk headers.lst
 	rm -f $(PKG_CONFIG)
 	rm -f _paths.h
 
 .PHONY: distclean
 distclean: clean
+
+.PHONY: FORCE
+FORCE:
-- 
2.11.0

[PATCH 10/10] tools/libs: Write out an ABI analysis when abi-dumper is available

[Andrew Cooper]

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
CC: Juergen Gross <jgross@suse.com>
---
 tools/libs/libs.mk | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/libs/libs.mk b/tools/libs/libs.mk
index ac68996ab2..2a4ce8a90c 100644
--- a/tools/libs/libs.mk
+++ b/tools/libs/libs.mk
@@ -49,6 +49,8 @@ PKG_CONFIG_LOCAL := $(PKG_CONFIG_DIR)/$(PKG_CONFIG)
 LIBHEADER ?= $(LIB_FILE_NAME).h
 LIBHEADERS = $(foreach h, $(LIBHEADER), $(XEN_INCLUDE)/$(h))
 
+PKG_ABI := lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR)-$(XEN_COMPILE_ARCH)-abi.dump
+
 $(PKG_CONFIG_LOCAL): PKG_CONFIG_PREFIX = $(XEN_ROOT)
 $(PKG_CONFIG_LOCAL): PKG_CONFIG_INCDIR = $(XEN_INCLUDE)
 $(PKG_CONFIG_LOCAL): PKG_CONFIG_LIBDIR = $(CURDIR)
@@ -94,6 +96,13 @@ lib$(LIB_FILE_NAME).so.$(MAJOR): lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR)
 lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR): $(PIC_OBJS) libxen$(LIBNAME).map
 	$(CC) $(LDFLAGS) $(PTHREAD_LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,lib$(LIB_FILE_NAME).so.$(MAJOR) $(SHLIB_LDFLAGS) -o $@ $(PIC_OBJS) $(LDUSELIBS) $(APPEND_LDFLAGS)
 
+# If abi-dumper is available, write out the ABI analysis
+ifneq ($(ABI_DUMPER),)
+libs: $(PKG_ABI)
+$(PKG_ABI): lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR) headers.lst
+	abi-dumper $< -o $@ -public-headers headers.lst -lver $(MAJOR).$(MINOR)
+endif
+
 .PHONY: install
 install: build
 	$(INSTALL_DIR) $(DESTDIR)$(libdir)
-- 
2.11.0

Re: [PATCH 02/10] tools/libxg: Fix uninitialised variable in write_x86_cpu_policy_records()

[Jan Beulich]

On 12.02.2021 16:39, Andrew Cooper wrote:
> Various version of gcc, when compiling with -Og, complain:
> 
>   xg_sr_common_x86.c: In function 'write_x86_cpu_policy_records':
>   xg_sr_common_x86.c:92:12: error: 'rc' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>      92 |     return rc;
>         |            ^~
> 
> The complaint is legitimate, and can occur with unexpected behaviour of two
> related hypercalls in combination with a libc which permits zero-length
> malloc()s.
> 
> Have an explicit rc = 0 on the success path, and make the MSRs record error
> handling consistent with the CPUID record before it.
> 
> Fixes: f6b2b8ec53d ("libxc/save: Write X86_{CPUID,MSR}_DATA records")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH 03/10] tools/libxg: Fix uninitialised variable in meminit()

[Julien Grall]

Hi Andrew,

On 12/02/2021 15:39, Andrew Cooper wrote:
> Various version of gcc, when compiling with -Og, complain:
> 
>    xg_dom_arm.c: In function 'meminit':
>    xg_dom_arm.c:420:19: error: 'p2m_size' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>      420 |     dom->p2m_size = p2m_size;
>          |     ~~~~~~~~~~~~~~^~~~~~~~~~
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

This was reported nearly 3 years ago (see [1]) and it is pretty sad this 
was never merged :(.

> ---
> CC: Ian Jackson <iwj@xenproject.org>
> CC: Wei Liu <wl@xen.org>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> 
> Julien/Stefano: I can't work out how this variable is supposed to work, and
> the fact that it isn't a straight accumulation across the RAM banks looks
> suspect.

It looks buggy, but the P2M is never used on Arm. In fact, you sent a 
patch a year ago to drop it (see [2]). It would be nice to revive it.

> ---
>   tools/libs/guest/xg_dom_arm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/libs/guest/xg_dom_arm.c b/tools/libs/guest/xg_dom_arm.c
> index 94948d2b20..f1b8d06f75 100644
> --- a/tools/libs/guest/xg_dom_arm.c
> +++ b/tools/libs/guest/xg_dom_arm.c
> @@ -373,7 +373,7 @@ static int meminit(struct xc_dom_image *dom)
>       const uint64_t modsize = dtb_size + ramdisk_size;
>       const uint64_t ram128mb = bankbase[0] + (128<<20);
>   
> -    xen_pfn_t p2m_size;
> +    xen_pfn_t p2m_size = 0;
>       uint64_t bank0end;
>   
>       assert(dom->rambase_pfn << XC_PAGE_SHIFT == bankbase[0]);
>

If your original series is too risky for 4.15, I would consider to 
remote p2m_size completely and always 0 dom->p2m_size.

Cheers,

[1] 
https://lore.kernel.org/xen-devel/20180314123203.30646-1-wei.liu2@citrix.com/
[2] 
https://patchwork.kernel.org/project/xen-devel/patch/20191217201550.15864-3-andrew.cooper3@citrix.com/

-- 
Julien Grall

Re: [PATCH 07/10] tools: Use -Og for debug builds when available

[Jan Beulich]

On 12.02.2021 16:39, Andrew Cooper wrote:
> The recommended optimisation level for debugging is -Og, and is what tools
> such as gdb prefer.  In practice, it equates to -01 with a few specific
> optimisations turned off.
> 
> abi-dumper in particular wants the libraries it inspects in this form.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> --- a/tools/Rules.mk
> +++ b/tools/Rules.mk
> @@ -106,8 +106,9 @@ endif
>  CFLAGS_libxenlight += $(CFLAGS_libxenctrl)
>  
>  ifeq ($(debug),y)
> -# Disable optimizations
> -CFLAGS += -O0 -fno-omit-frame-pointer
> +# Use -Og if available, -O0 otherwise
> +dbg_opt_level := $(call cc-option,$(CC),-Og,-O0)
> +CFLAGS += $(dbg_opt_level) -fno-omit-frame-pointer

I wonder if we shouldn't do something similar for the hypervisor,
where we use -O1 for debug builds right now. At least when
DEBUG_INFO is also enabled, -Og may be better.

Jan

Re: [PATCH 06/10] stubdom/xenstored: Fix uninitialised variables in lu_read_state()

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1447 bytes --]

On 12.02.21 16:39, Andrew Cooper wrote:
> Various version of gcc, when compiling with -Og, complain:
> 
>    xenstored_control.c: In function ‘lu_read_state’:
>    xenstored_control.c:540:11: error: ‘state.size’ is used uninitialized in this
>    function [-Werror=uninitialized]
>      if (state.size == 0)
>          ~~~~~^~~~~
>    xenstored_control.c:543:6: error: ‘state.buf’ may be used uninitialized in
>    this function [-Werror=maybe-uninitialized]
>      pre = state.buf;
>      ~~~~^~~~~~~~~~~
>    xenstored_control.c:550:23: error: ‘state.buf’ may be used uninitialized in
>    this function [-Werror=maybe-uninitialized]
>       (void *)head - state.buf < state.size;
>                      ~~~~~^~~~
>    xenstored_control.c:550:35: error: ‘state.size’ may be used uninitialized in
>    this function [-Werror=maybe-uninitialized]
>       (void *)head - state.buf < state.size;
>                                  ~~~~~^~~~~
> 
> Interestingly, this is only in the stubdom build.  I can't identify any
> relevant differences vs the regular tools build.

But me. :-)

lu_get_dump_state() is empty for the stubdom case (this will change when
LU is implemented for stubdom, too). In the daemon case this function is
setting all the fields which are relevant.

> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 07/10] tools: Use -Og for debug builds when available

[Andrew Cooper]

On 12/02/2021 16:04, Jan Beulich wrote:
> On 12.02.2021 16:39, Andrew Cooper wrote:
>> The recommended optimisation level for debugging is -Og, and is what tools
>> such as gdb prefer.  In practice, it equates to -01 with a few specific
>> optimisations turned off.
>>
>> abi-dumper in particular wants the libraries it inspects in this form.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks,

>
>> --- a/tools/Rules.mk
>> +++ b/tools/Rules.mk
>> @@ -106,8 +106,9 @@ endif
>>  CFLAGS_libxenlight += $(CFLAGS_libxenctrl)
>>  
>>  ifeq ($(debug),y)
>> -# Disable optimizations
>> -CFLAGS += -O0 -fno-omit-frame-pointer
>> +# Use -Og if available, -O0 otherwise
>> +dbg_opt_level := $(call cc-option,$(CC),-Og,-O0)
>> +CFLAGS += $(dbg_opt_level) -fno-omit-frame-pointer
> I wonder if we shouldn't do something similar for the hypervisor,
> where we use -O1 for debug builds right now. At least when
> DEBUG_INFO is also enabled, -Og may be better.

I also made that work... its rather more invasive in terms of changes -
all for "maybe uninitialised" warnings.

$ git diff e2bab84984^ --stat
 xen/Makefile                    | 3 ++-
 xen/arch/arm/domain_build.c     | 2 +-
 xen/arch/x86/irq.c              | 2 +-
 xen/arch/x86/mm/shadow/common.c | 2 +-
 xen/arch/x86/pv/shim.c          | 6 +++---
 xen/arch/x86/sysctl.c           | 4 ++--
 xen/common/efi/boot.c           | 2 +-
 7 files changed, 11 insertions(+), 10 deletions(-)

is what is required to make Gitlab happy.  I was planning to defer it to
4.16 at this point.

~Andrew

Re: [PATCH 10/10] tools/libs: Write out an ABI analysis when abi-dumper is available

[Jan Beulich]

On 12.02.2021 16:39, Andrew Cooper wrote:
> --- a/tools/libs/libs.mk
> +++ b/tools/libs/libs.mk
> @@ -49,6 +49,8 @@ PKG_CONFIG_LOCAL := $(PKG_CONFIG_DIR)/$(PKG_CONFIG)
>  LIBHEADER ?= $(LIB_FILE_NAME).h
>  LIBHEADERS = $(foreach h, $(LIBHEADER), $(XEN_INCLUDE)/$(h))
>  
> +PKG_ABI := lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR)-$(XEN_COMPILE_ARCH)-abi.dump

Don't you mean $(XEN_TARGET_ARCH) here?

Jan

Re: [PATCH 07/10] tools: Use -Og for debug builds when available

[Jan Beulich]

On 12.02.2021 17:09, Andrew Cooper wrote:
> On 12/02/2021 16:04, Jan Beulich wrote:
>> On 12.02.2021 16:39, Andrew Cooper wrote:
>>> --- a/tools/Rules.mk
>>> +++ b/tools/Rules.mk
>>> @@ -106,8 +106,9 @@ endif
>>>  CFLAGS_libxenlight += $(CFLAGS_libxenctrl)
>>>  
>>>  ifeq ($(debug),y)
>>> -# Disable optimizations
>>> -CFLAGS += -O0 -fno-omit-frame-pointer
>>> +# Use -Og if available, -O0 otherwise
>>> +dbg_opt_level := $(call cc-option,$(CC),-Og,-O0)
>>> +CFLAGS += $(dbg_opt_level) -fno-omit-frame-pointer
>> I wonder if we shouldn't do something similar for the hypervisor,
>> where we use -O1 for debug builds right now. At least when
>> DEBUG_INFO is also enabled, -Og may be better.
> 
> I also made that work... its rather more invasive in terms of changes -
> all for "maybe uninitialised" warnings.
> 
> $ git diff e2bab84984^ --stat
>  xen/Makefile                    | 3 ++-
>  xen/arch/arm/domain_build.c     | 2 +-
>  xen/arch/x86/irq.c              | 2 +-
>  xen/arch/x86/mm/shadow/common.c | 2 +-
>  xen/arch/x86/pv/shim.c          | 6 +++---
>  xen/arch/x86/sysctl.c           | 4 ++--
>  xen/common/efi/boot.c           | 2 +-
>  7 files changed, 11 insertions(+), 10 deletions(-)
> 
> is what is required to make Gitlab happy.

Oh, good to know. Thanks!

>  I was planning to defer it to 4.16 at this point.

Of course.

Jan

Re: [PATCH 06/10] stubdom/xenstored: Fix uninitialised variables in lu_read_state()

[Andrew Cooper]

On 12/02/2021 16:08, Jürgen Groß wrote:
> On 12.02.21 16:39, Andrew Cooper wrote:
>> Various version of gcc, when compiling with -Og, complain:
>>
>>    xenstored_control.c: In function ‘lu_read_state’:
>>    xenstored_control.c:540:11: error: ‘state.size’ is used
>> uninitialized in this
>>    function [-Werror=uninitialized]
>>      if (state.size == 0)
>>          ~~~~~^~~~~
>>    xenstored_control.c:543:6: error: ‘state.buf’ may be used
>> uninitialized in
>>    this function [-Werror=maybe-uninitialized]
>>      pre = state.buf;
>>      ~~~~^~~~~~~~~~~
>>    xenstored_control.c:550:23: error: ‘state.buf’ may be used
>> uninitialized in
>>    this function [-Werror=maybe-uninitialized]
>>       (void *)head - state.buf < state.size;
>>                      ~~~~~^~~~
>>    xenstored_control.c:550:35: error: ‘state.size’ may be used
>> uninitialized in
>>    this function [-Werror=maybe-uninitialized]
>>       (void *)head - state.buf < state.size;
>>                                  ~~~~~^~~~~
>>
>> Interestingly, this is only in the stubdom build.  I can't identify any
>> relevant differences vs the regular tools build.
>
> But me. :-)
>
> lu_get_dump_state() is empty for the stubdom case (this will change when
> LU is implemented for stubdom, too). In the daemon case this function is
> setting all the fields which are relevant.

So I spotted that.  This instance of lu_read_state() is already within
the ifdefary, so doesn't get to see the empty stub (I think).

Also, I'd expect the compiler to complain at -O2 if it spotted that code.

>
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> Reviewed-by: Juergen Gross <jgross@suse.com>

Thanks,

~Andrew

Re: [PATCH 10/10] tools/libs: Write out an ABI analysis when abi-dumper is available

[Andrew Cooper]

On 12/02/2021 16:12, Jan Beulich wrote:
> On 12.02.2021 16:39, Andrew Cooper wrote:
>> --- a/tools/libs/libs.mk
>> +++ b/tools/libs/libs.mk
>> @@ -49,6 +49,8 @@ PKG_CONFIG_LOCAL := $(PKG_CONFIG_DIR)/$(PKG_CONFIG)
>>  LIBHEADER ?= $(LIB_FILE_NAME).h
>>  LIBHEADERS = $(foreach h, $(LIBHEADER), $(XEN_INCLUDE)/$(h))
>>  
>> +PKG_ABI := lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR)-$(XEN_COMPILE_ARCH)-abi.dump
> Don't you mean $(XEN_TARGET_ARCH) here?

Yes, I do.  Will fix up.

Thanks,

~Andrew

[PATCH v1.1 10/10] tools/libs: Write out an ABI analysis when abi-dumper is available

[Andrew Cooper]

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
CC: Juergen Gross <jgross@suse.com>

v2:
 * Swap XEN_COMPILE_ARCH for XEN_TARGET_ARCH
 * Swap abi-dumper for $(ABI_DUMPER)
---
 tools/libs/libs.mk | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/libs/libs.mk b/tools/libs/libs.mk
index ac68996ab2..a82d1783cc 100644
--- a/tools/libs/libs.mk
+++ b/tools/libs/libs.mk
@@ -49,6 +49,8 @@ PKG_CONFIG_LOCAL := $(PKG_CONFIG_DIR)/$(PKG_CONFIG)
 LIBHEADER ?= $(LIB_FILE_NAME).h
 LIBHEADERS = $(foreach h, $(LIBHEADER), $(XEN_INCLUDE)/$(h))
 
+PKG_ABI := lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR)-$(XEN_TARGET_ARCH)-abi.dump
+
 $(PKG_CONFIG_LOCAL): PKG_CONFIG_PREFIX = $(XEN_ROOT)
 $(PKG_CONFIG_LOCAL): PKG_CONFIG_INCDIR = $(XEN_INCLUDE)
 $(PKG_CONFIG_LOCAL): PKG_CONFIG_LIBDIR = $(CURDIR)
@@ -94,6 +96,13 @@ lib$(LIB_FILE_NAME).so.$(MAJOR): lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR)
 lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR): $(PIC_OBJS) libxen$(LIBNAME).map
 	$(CC) $(LDFLAGS) $(PTHREAD_LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,lib$(LIB_FILE_NAME).so.$(MAJOR) $(SHLIB_LDFLAGS) -o $@ $(PIC_OBJS) $(LDUSELIBS) $(APPEND_LDFLAGS)
 
+# If abi-dumper is available, write out the ABI analysis
+ifneq ($(ABI_DUMPER),)
+libs: $(PKG_ABI)
+$(PKG_ABI): lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR) headers.lst
+	$(ABI_DUMPER) $< -o $@ -public-headers headers.lst -lver $(MAJOR).$(MINOR)
+endif
+
 .PHONY: install
 install: build
 	$(INSTALL_DIR) $(DESTDIR)$(libdir)
-- 
2.11.0

Re: [PATCH 03/10] tools/libxg: Fix uninitialised variable in meminit()

[Andrew Cooper]

On 12/02/2021 15:55, Julien Grall wrote:
> Hi Andrew,
>
> On 12/02/2021 15:39, Andrew Cooper wrote:
>> Various version of gcc, when compiling with -Og, complain:
>>
>>    xg_dom_arm.c: In function 'meminit':
>>    xg_dom_arm.c:420:19: error: 'p2m_size' may be used uninitialized
>> in this function [-Werror=maybe-uninitialized]
>>      420 |     dom->p2m_size = p2m_size;
>>          |     ~~~~~~~~~~~~~~^~~~~~~~~~
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> This was reported nearly 3 years ago (see [1]) and it is pretty sad
> this was never merged :(.

:( We've got far too many patches which fall through the cracks like this.

>
>> ---
>> CC: Ian Jackson <iwj@xenproject.org>
>> CC: Wei Liu <wl@xen.org>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien@xen.org>
>>
>> Julien/Stefano: I can't work out how this variable is supposed to
>> work, and
>> the fact that it isn't a straight accumulation across the RAM banks
>> looks
>> suspect.
>
> It looks buggy, but the P2M is never used on Arm. In fact, you sent a
> patch a year ago to drop it (see [2]). It would be nice to revive it.


That series was committed more than a year ago - ee21f10d70^..97e34ad22d
- and tbh, I'd forgotten about it.

In light of that, I think I'll just delete the p2m_size references
here.  It's easy to prove correctness via inspection, and removes a
dubious construct entirely.

~Andrew

[PATCH v1.1 03/10] tools/libxg: Drop stale p2m logic from ARM's meminit()

[Andrew Cooper]

Various version of gcc, when compiling with -Og, complain:

  xg_dom_arm.c: In function 'meminit':
  xg_dom_arm.c:420:19: error: 'p2m_size' may be used uninitialized in this function [-Werror=maybe-uninitialized]
    420 |     dom->p2m_size = p2m_size;
        |     ~~~~~~~~~~~~~~^~~~~~~~~~

This is actually entirely stale code since ee21f10d70^..97e34ad22d which
removed the 1:1 identity p2m for translated domains.

Drop the write of d->p2m_size, and the p2m_size local variable.  Reposition
the p2m_size field in struct xc_dom_image and correct some stale
documentation.

This change really ought to have been part of the original cleanup series.

No actual change to how ARM domains are constructed.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>

v2:
 * Delete stale p2m_size infrastructure.
---
 tools/include/xenguest.h      | 5 ++---
 tools/libs/guest/xg_dom_arm.c | 5 -----
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index 775cf34c04..217022b6e7 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -145,6 +145,7 @@ struct xc_dom_image {
      * eventually copied into guest context.
      */
     xen_pfn_t *pv_p2m;
+    xen_pfn_t p2m_size;         /* number of pfns covered by pv_p2m */
 
     /* physical memory
      *
@@ -154,12 +155,10 @@ struct xc_dom_image {
      *
      * An ARM guest has GUEST_RAM_BANKS regions of RAM, with
      * rambank_size[i] pages in each. The lowest RAM address
-     * (corresponding to the base of the p2m arrays above) is stored
-     * in rambase_pfn.
+     * is stored in rambase_pfn.
      */
     xen_pfn_t rambase_pfn;
     xen_pfn_t total_pages;
-    xen_pfn_t p2m_size;         /* number of pfns covered by p2m */
     struct xc_dom_phys *phys_pages;
 #if defined (__arm__) || defined(__aarch64__)
     xen_pfn_t rambank_size[GUEST_RAM_BANKS];
diff --git a/tools/libs/guest/xg_dom_arm.c b/tools/libs/guest/xg_dom_arm.c
index 94948d2b20..b4c24f15fb 100644
--- a/tools/libs/guest/xg_dom_arm.c
+++ b/tools/libs/guest/xg_dom_arm.c
@@ -373,7 +373,6 @@ static int meminit(struct xc_dom_image *dom)
     const uint64_t modsize = dtb_size + ramdisk_size;
     const uint64_t ram128mb = bankbase[0] + (128<<20);
 
-    xen_pfn_t p2m_size;
     uint64_t bank0end;
 
     assert(dom->rambase_pfn << XC_PAGE_SHIFT == bankbase[0]);
@@ -409,16 +408,12 @@ static int meminit(struct xc_dom_image *dom)
 
         ramsize -= banksize;
 
-        p2m_size = ( bankbase[i] + banksize - bankbase[0] ) >> XC_PAGE_SHIFT;
-
         dom->rambank_size[i] = banksize >> XC_PAGE_SHIFT;
     }
 
     assert(dom->rambank_size[0] != 0);
     assert(ramsize == 0); /* Too much RAM is rejected above */
 
-    dom->p2m_size = p2m_size;
-
     /* setup initial p2m and allocate guest memory */
     for ( i = 0; i < GUEST_RAM_BANKS && dom->rambank_size[i]; i++ )
     {
-- 
2.11.0

Re: [PATCH 06/10] stubdom/xenstored: Fix uninitialised variables in lu_read_state()

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1812 bytes --]

On 12.02.21 18:01, Andrew Cooper wrote:
> On 12/02/2021 16:08, Jürgen Groß wrote:
>> On 12.02.21 16:39, Andrew Cooper wrote:
>>> Various version of gcc, when compiling with -Og, complain:
>>>
>>>     xenstored_control.c: In function ‘lu_read_state’:
>>>     xenstored_control.c:540:11: error: ‘state.size’ is used
>>> uninitialized in this
>>>     function [-Werror=uninitialized]
>>>       if (state.size == 0)
>>>           ~~~~~^~~~~
>>>     xenstored_control.c:543:6: error: ‘state.buf’ may be used
>>> uninitialized in
>>>     this function [-Werror=maybe-uninitialized]
>>>       pre = state.buf;
>>>       ~~~~^~~~~~~~~~~
>>>     xenstored_control.c:550:23: error: ‘state.buf’ may be used
>>> uninitialized in
>>>     this function [-Werror=maybe-uninitialized]
>>>        (void *)head - state.buf < state.size;
>>>                       ~~~~~^~~~
>>>     xenstored_control.c:550:35: error: ‘state.size’ may be used
>>> uninitialized in
>>>     this function [-Werror=maybe-uninitialized]
>>>        (void *)head - state.buf < state.size;
>>>                                   ~~~~~^~~~~
>>>
>>> Interestingly, this is only in the stubdom build.  I can't identify any
>>> relevant differences vs the regular tools build.
>>
>> But me. :-)
>>
>> lu_get_dump_state() is empty for the stubdom case (this will change when
>> LU is implemented for stubdom, too). In the daemon case this function is
>> setting all the fields which are relevant.
> 
> So I spotted that.  This instance of lu_read_state() is already within
> the ifdefary, so doesn't get to see the empty stub (I think).

There is only one instance of lu_read_state().


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 06/10] stubdom/xenstored: Fix uninitialised variables in lu_read_state()

[Andrew Cooper]

On 13/02/2021 09:36, Jürgen Groß wrote:
> On 12.02.21 18:01, Andrew Cooper wrote:
>> On 12/02/2021 16:08, Jürgen Groß wrote:
>>> On 12.02.21 16:39, Andrew Cooper wrote:
>>>> Various version of gcc, when compiling with -Og, complain:
>>>>
>>>>     xenstored_control.c: In function ‘lu_read_state’:
>>>>     xenstored_control.c:540:11: error: ‘state.size’ is used
>>>> uninitialized in this
>>>>     function [-Werror=uninitialized]
>>>>       if (state.size == 0)
>>>>           ~~~~~^~~~~
>>>>     xenstored_control.c:543:6: error: ‘state.buf’ may be used
>>>> uninitialized in
>>>>     this function [-Werror=maybe-uninitialized]
>>>>       pre = state.buf;
>>>>       ~~~~^~~~~~~~~~~
>>>>     xenstored_control.c:550:23: error: ‘state.buf’ may be used
>>>> uninitialized in
>>>>     this function [-Werror=maybe-uninitialized]
>>>>        (void *)head - state.buf < state.size;
>>>>                       ~~~~~^~~~
>>>>     xenstored_control.c:550:35: error: ‘state.size’ may be used
>>>> uninitialized in
>>>>     this function [-Werror=maybe-uninitialized]
>>>>        (void *)head - state.buf < state.size;
>>>>                                   ~~~~~^~~~~
>>>>
>>>> Interestingly, this is only in the stubdom build.  I can't identify
>>>> any
>>>> relevant differences vs the regular tools build.
>>>
>>> But me. :-)
>>>
>>> lu_get_dump_state() is empty for the stubdom case (this will change
>>> when
>>> LU is implemented for stubdom, too). In the daemon case this
>>> function is
>>> setting all the fields which are relevant.
>>
>> So I spotted that.  This instance of lu_read_state() is already within
>> the ifdefary, so doesn't get to see the empty stub (I think).
>
> There is only one instance of lu_read_state().

Ahh - I got the NO_LIVE_UPDATE and __MINIOS__ ifdefary inverted.  (It is
very confusing to follow).

I'll reword the commit message, but leave the fix the same.

~Andrew

[PATCH v1.1 06/10] stubdom/xenstored: Fix uninitialised variables in lu_read_state()

[Andrew Cooper]

Various version of gcc, when compiling with -Og, complain:

  xenstored_control.c: In function ‘lu_read_state’:
  xenstored_control.c:540:11: error: ‘state.size’ is used uninitialized in this
  function [-Werror=uninitialized]
    if (state.size == 0)
        ~~~~~^~~~~
  xenstored_control.c:543:6: error: ‘state.buf’ may be used uninitialized in
  this function [-Werror=maybe-uninitialized]
    pre = state.buf;
    ~~~~^~~~~~~~~~~
  xenstored_control.c:550:23: error: ‘state.buf’ may be used uninitialized in
  this function [-Werror=maybe-uninitialized]
     (void *)head - state.buf < state.size;
                    ~~~~~^~~~
  xenstored_control.c:550:35: error: ‘state.size’ may be used uninitialized in
  this function [-Werror=maybe-uninitialized]
     (void *)head - state.buf < state.size;
                                ~~~~~^~~~~

for the stubdom build.  This is because lu_get_dump_state() is a no-op stub in
MiniOS, and state really is operated on uninitialised.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
CC: Juergen Gross <jgross@suse.com>

v2:
 * Reword commit message.
---
 tools/xenstore/xenstored_control.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c
index 1f733e0a04..f10beaf85e 100644
--- a/tools/xenstore/xenstored_control.c
+++ b/tools/xenstore/xenstored_control.c
@@ -530,7 +530,7 @@ static const char *lu_dump_state(const void *ctx, struct connection *conn)
 
 void lu_read_state(void)
 {
-	struct lu_dump_state state;
+	struct lu_dump_state state = {};
 	struct xs_state_record_header *head;
 	void *ctx = talloc_new(NULL); /* Work context for subfunctions. */
 	struct xs_state_preamble *pre;
-- 
2.11.0

Re: [PATCH 01/10] tools/xl: Fix exit code for `xl vkbattach`

[Ian Jackson]

Andrew Cooper writes ("[PATCH 01/10] tools/xl: Fix exit code for `xl vkbattach`"):
> Various version of gcc, when compiling with -Og, complain:
> 
>   xl_vkb.c: In function 'main_vkbattach':
>   xl_vkb.c:79:12: error: 'rc' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>      79 |     return rc;
>         |            ^~
> 
> The dryrun_only path really does leave rc uninitalised.  Introduce a done
> label for success paths to use.
> 
> Fixes: a15166af7c3 ("xl: add vkb config parser and CLI")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH 02/10] tools/libxg: Fix uninitialised variable in write_x86_cpu_policy_records()

[Ian Jackson]

Andrew Cooper writes ("[PATCH 02/10] tools/libxg: Fix uninitialised variable in write_x86_cpu_policy_records()"):
> Fixes: f6b2b8ec53d ("libxc/save: Write X86_{CPUID,MSR}_DATA records")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v1.1 03/10] tools/libxg: Drop stale p2m logic from ARM's meminit()

[Ian Jackson]

Andrew Cooper writes ("[PATCH v1.1 03/10] tools/libxg: Drop stale p2m logic from ARM's meminit()"):
> Various version of gcc, when compiling with -Og, complain:
> 
>   xg_dom_arm.c: In function 'meminit':
>   xg_dom_arm.c:420:19: error: 'p2m_size' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>     420 |     dom->p2m_size = p2m_size;
>         |     ~~~~~~~~~~~~~~^~~~~~~~~~
> 
> This is actually entirely stale code since ee21f10d70^..97e34ad22d which
> removed the 1:1 identity p2m for translated domains.
> 
> Drop the write of d->p2m_size, and the p2m_size local variable.  Reposition
> the p2m_size field in struct xc_dom_image and correct some stale
> documentation.
> 
> This change really ought to have been part of the original cleanup series.
> 
> No actual change to how ARM domains are constructed.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

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

[Ian Jackson]

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.

Re: [PATCH 05/10] tools/libxl: Fix uninitialised variable in libxl__write_stub_dmargs()

[Ian Jackson]

Andrew Cooper writes ("[PATCH 05/10] tools/libxl: Fix uninitialised variable in libxl__write_stub_dmargs()"):
> Various version of gcc, when compiling with -Og, complain:
> 
>   libxl_dm.c: In function ‘libxl__write_stub_dmargs’:
>   libxl_dm.c:2166:16: error: ‘dmargs’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>                rc = libxl__xs_write_checked(gc, t, path, dmargs);
>                ~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> It can't, but only because of how the is_linux_stubdom checks line up.

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

Re: [PATCH 06/10] stubdom/xenstored: Fix uninitialised variables in lu_read_state()

[Ian Jackson]

Andrew Cooper writes ("[PATCH 06/10] stubdom/xenstored: Fix uninitialised variables in lu_read_state()"):
> Various version of gcc, when compiling with -Og, complain:
> 
>   xenstored_control.c: In function ‘lu_read_state’:
>   xenstored_control.c:540:11: error: ‘state.size’ is used uninitialized in this
>   function [-Werror=uninitialized]
>     if (state.size == 0)
>         ~~~~~^~~~~
>   xenstored_control.c:543:6: error: ‘state.buf’ may be used uninitialized in
>   this function [-Werror=maybe-uninitialized]
>     pre = state.buf;
>     ~~~~^~~~~~~~~~~
>   xenstored_control.c:550:23: error: ‘state.buf’ may be used uninitialized in
>   this function [-Werror=maybe-uninitialized]
>      (void *)head - state.buf < state.size;
>                     ~~~~~^~~~
>   xenstored_control.c:550:35: error: ‘state.size’ may be used uninitialized in
>   this function [-Werror=maybe-uninitialized]
>      (void *)head - state.buf < state.size;
>                                 ~~~~~^~~~~
> 
> Interestingly, this is only in the stubdom build.  I can't identify any
> relevant differences vs the regular tools build.

  #ifdef __MINIOS__
  static void lu_get_dump_state(struct lu_dump_state *state)
  {
  }

So the compiler is right to complain that

  lu_get_dump_state(&state);
  if (state.size == 0)
          barf_perror("No state found after live-update");

this will use state.size uninitialised.

It's probably just luck that this works at all, if it does,
anywhere...

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

Ian.

Re: [PATCH 07/10] tools: Use -Og for debug builds when available

[Ian Jackson]

Andrew Cooper writes ("[PATCH 07/10] tools: Use -Og for debug builds when available"):
> The recommended optimisation level for debugging is -Og, and is what tools
> such as gdb prefer.  In practice, it equates to -01 with a few specific
> optimisations turned off.
> 
> abi-dumper in particular wants the libraries it inspects in this form.

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

I would prefer to have this in 4.15 now than to backport it later...

Ian.

Re: [PATCH 08/10] tools: Check for abi-dumper in ./configure

[Ian Jackson]

Andrew Cooper writes ("[PATCH 08/10] tools: Check for abi-dumper in ./configure"):
> This will be optional.  No functional change.

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

Re: [PATCH 09/10] tools/libs: Add rule to generate headers.lst

[Ian Jackson]

Andrew Cooper writes ("[PATCH 09/10] tools/libs: Add rule to generate headers.lst"):
> abi-dumper needs a list of the public header files for shared objects, and
> only accepts this in the form of a file.

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

because it's not run by default, but...

> +headers.lst: FORCE
> +	@{ $(foreach h,$(LIBHEADERS),echo $(h);) } > $@.tmp

Missing set -e.  If the disk fills up temporarily you might get a
partial file here...

> +	@$(call move-if-changed,$@.tmp,$@)

Ian.

Re: [PATCH 10/10] tools/libs: Write out an ABI analysis when abi-dumper is available

[Ian Jackson]

Andrew Cooper writes ("[PATCH 10/10] tools/libs: Write out an ABI analysis when abi-dumper is available"):
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH 01/10] tools/xl: Fix exit code for `xl vkbattach`

[Ian Jackson]

Andrew Cooper writes ("[PATCH 01/10] tools/xl: Fix exit code for `xl vkbattach`"):
> Various version of gcc, when compiling with -Og, complain:
> 
>   xl_vkb.c: In function 'main_vkbattach':
>   xl_vkb.c:79:12: error: 'rc' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>      79 |     return rc;
>         |            ^~
> 
> The dryrun_only path really does leave rc uninitalised.  Introduce a done
> label for success paths to use.
> 
> Fixes: a15166af7c3 ("xl: add vkb config parser and CLI")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH 05/10] tools/libxl: Fix uninitialised variable in libxl__write_stub_dmargs()

[Ian Jackson]

Andrew Cooper writes ("[PATCH 05/10] tools/libxl: Fix uninitialised variable in libxl__write_stub_dmargs()"):
> Various version of gcc, when compiling with -Og, complain:
> 
>   libxl_dm.c: In function ‘libxl__write_stub_dmargs’:
>   libxl_dm.c:2166:16: error: ‘dmargs’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>                rc = libxl__xs_write_checked(gc, t, path, dmargs);
>                ~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

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

Re: [PATCH 07/10] tools: Use -Og for debug builds when available

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH 07/10] tools: Use -Og for debug builds when available"):
> On 12.02.2021 16:39, Andrew Cooper wrote:
> > The recommended optimisation level for debugging is -Og, and is what tools
> > such as gdb prefer.  In practice, it equates to -01 with a few specific
> > optimisations turned off.
> > 
> > abi-dumper in particular wants the libraries it inspects in this form.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

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

Re: [PATCH 08/10] tools: Check for abi-dumper in ./configure

[Ian Jackson]

Andrew Cooper writes ("[PATCH 08/10] tools: Check for abi-dumper in ./configure"):
> This will be optional.  No functional change.

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

Re: [PATCH v1.1 03/10] tools/libxg: Drop stale p2m logic from ARM's meminit()

[Julien Grall]

Hi Andrew,

On 12/02/2021 20:01, Andrew Cooper wrote:
> Various version of gcc, when compiling with -Og, complain:
> 
>    xg_dom_arm.c: In function 'meminit':
>    xg_dom_arm.c:420:19: error: 'p2m_size' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>      420 |     dom->p2m_size = p2m_size;
>          |     ~~~~~~~~~~~~~~^~~~~~~~~~
> 
> This is actually entirely stale code since ee21f10d70^..97e34ad22d which
> removed the 1:1 identity p2m for translated domains.
> 
> Drop the write of d->p2m_size, and the p2m_size local variable.  Reposition
> the p2m_size field in struct xc_dom_image and correct some stale
> documentation.
> 
> This change really ought to have been part of the original cleanup series.
> 
> No actual change to how ARM domains are constructed.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

> ---
> CC: Ian Jackson <iwj@xenproject.org>
> CC: Wei Liu <wl@xen.org>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> 
> v2:
>   * Delete stale p2m_size infrastructure.
> ---
>   tools/include/xenguest.h      | 5 ++---
>   tools/libs/guest/xg_dom_arm.c | 5 -----
>   2 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
> index 775cf34c04..217022b6e7 100644
> --- a/tools/include/xenguest.h
> +++ b/tools/include/xenguest.h
> @@ -145,6 +145,7 @@ struct xc_dom_image {
>        * eventually copied into guest context.
>        */
>       xen_pfn_t *pv_p2m;
> +    xen_pfn_t p2m_size;         /* number of pfns covered by pv_p2m */
>   
>       /* physical memory
>        *
> @@ -154,12 +155,10 @@ struct xc_dom_image {
>        *
>        * An ARM guest has GUEST_RAM_BANKS regions of RAM, with
>        * rambank_size[i] pages in each. The lowest RAM address
> -     * (corresponding to the base of the p2m arrays above) is stored
> -     * in rambase_pfn.
> +     * is stored in rambase_pfn.
>        */
>       xen_pfn_t rambase_pfn;
>       xen_pfn_t total_pages;
> -    xen_pfn_t p2m_size;         /* number of pfns covered by p2m */
>       struct xc_dom_phys *phys_pages;
>   #if defined (__arm__) || defined(__aarch64__)
>       xen_pfn_t rambank_size[GUEST_RAM_BANKS];
> diff --git a/tools/libs/guest/xg_dom_arm.c b/tools/libs/guest/xg_dom_arm.c
> index 94948d2b20..b4c24f15fb 100644
> --- a/tools/libs/guest/xg_dom_arm.c
> +++ b/tools/libs/guest/xg_dom_arm.c
> @@ -373,7 +373,6 @@ static int meminit(struct xc_dom_image *dom)
>       const uint64_t modsize = dtb_size + ramdisk_size;
>       const uint64_t ram128mb = bankbase[0] + (128<<20);
>   
> -    xen_pfn_t p2m_size;
>       uint64_t bank0end;
>   
>       assert(dom->rambase_pfn << XC_PAGE_SHIFT == bankbase[0]);
> @@ -409,16 +408,12 @@ static int meminit(struct xc_dom_image *dom)
>   
>           ramsize -= banksize;
>   
> -        p2m_size = ( bankbase[i] + banksize - bankbase[0] ) >> XC_PAGE_SHIFT;
> -
>           dom->rambank_size[i] = banksize >> XC_PAGE_SHIFT;
>       }
>   
>       assert(dom->rambank_size[0] != 0);
>       assert(ramsize == 0); /* Too much RAM is rejected above */
>   
> -    dom->p2m_size = p2m_size;
> -
>       /* setup initial p2m and allocate guest memory */
>       for ( i = 0; i < GUEST_RAM_BANKS && dom->rambank_size[i]; i++ )
>       {
> 

-- 
Julien Grall

Re: [PATCH 10/10] tools/libs: Write out an ABI analysis when abi-dumper is available

[Ian Jackson]

Andrew Cooper writes ("[PATCH 10/10] tools/libs: Write out an ABI analysis when abi-dumper is available"):
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
...
> +# If abi-dumper is available, write out the ABI analysis
> +ifneq ($(ABI_DUMPER),)
> +libs: $(PKG_ABI)
> +$(PKG_ABI): lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR) headers.lst
> +	abi-dumper $< -o $@ -public-headers headers.lst -lver $(MAJOR).$(MINOR)
> +endif

Kind of annoying that we don't have a variable for
   lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR)

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

[PATCH v2 09/10] tools/libs: Add rule to generate headers.lst

[Andrew Cooper]

abi-dumper needs a list of the public header files for shared objects, and
only accepts this in the form of a file.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Release-Acked-by: Ian Jackson <iwj@xenproject.org>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
CC: Juergen Gross <jgross@suse.com>

v2:
 * Use set -e to avoid truncated content on transient errors
---
 tools/libs/.gitignore | 1 +
 tools/libs/libs.mk    | 9 ++++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)
 create mode 100644 tools/libs/.gitignore

diff --git a/tools/libs/.gitignore b/tools/libs/.gitignore
new file mode 100644
index 0000000000..4a13126144
--- /dev/null
+++ b/tools/libs/.gitignore
@@ -0,0 +1 @@
+*/headers.lst
diff --git a/tools/libs/libs.mk b/tools/libs/libs.mk
index 0b3381755a..7970627ac8 100644
--- a/tools/libs/libs.mk
+++ b/tools/libs/libs.mk
@@ -76,6 +76,10 @@ endif
 
 headers.chk: $(AUTOINCS)
 
+headers.lst: FORCE
+	@{ set -e; $(foreach h,$(LIBHEADERS),echo $(h);) } > $@.tmp
+	@$(call move-if-changed,$@.tmp,$@)
+
 libxen$(LIBNAME).map:
 	echo 'VERS_$(MAJOR).$(MINOR) { global: *; };' >$@
 
@@ -118,9 +122,12 @@ TAGS:
 clean:
 	rm -rf *.rpm $(LIB) *~ $(DEPS_RM) $(LIB_OBJS) $(PIC_OBJS)
 	rm -f lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR) lib$(LIB_FILE_NAME).so.$(MAJOR)
-	rm -f headers.chk
+	rm -f headers.chk headers.lst
 	rm -f $(PKG_CONFIG)
 	rm -f _paths.h
 
 .PHONY: distclean
 distclean: clean
+
+.PHONY: FORCE
+FORCE:
-- 
2.11.0

Re: [PATCH v2 09/10] tools/libs: Add rule to generate headers.lst

[Ian Jackson]

Andrew Cooper writes ("[PATCH v2 09/10] tools/libs: Add rule to generate headers.lst"):
> abi-dumper needs a list of the public header files for shared objects, and
> only accepts this in the form of a file.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Release-Acked-by: Ian Jackson <iwj@xenproject.org>

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

[PATCH v2 04/10] tools/libxl: Fix uninitialised variable 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.  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

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

[Ian Jackson]

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.

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

[Ian Jackson]

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.