[Qemu-devel] [PATCH for-1.6 0/2] Fix clang warnings in linux-user signal code

[Qemu-devel] [PATCH for-1.6 0/2] Fix clang warnings in linux-user signal code

[Peter Maydell]

These two patches fix some clang warnings about use of uninitialized
data in linux-user's signal related code for PPC and ARM. The issue
in both cases is the same: a code path taken in case of failure was
doing 'unlock_user_struct()' with parameters which hadn't yet been
set up.

I've marked this as for-1.6 because the patches are simple and I
think it's nice to get rid of warnings. However, they're not critical
for 1.6:
 * the existing code will be OK because unlock_user_struct() is
   a no-op unless DEBUG_REMAP is defined
 * clang isn't our primary compiler on Linux
 * configure (kind of inadvertently) disables -Werror for clang
 * there are other warnings not yet fixed anyhow (most notably
   all the places which use 'dprintf' as a debug macro despite
   that being the name of a POSIX specified function)

Peter Maydell (2):
  linux-user/signal.c: PPC: Silence clang uninitialized-use warning
  linux-user/signal.c: Avoid using uninitialized data in ARM sigreturn

 linux-user/signal.c |   38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH for-1.6 1/2] linux-user/signal.c: PPC: Silence clang uninitialized-use warning

[Peter Maydell]

Silence a clang warning in a PPC signal return function:

/home/petmay01/linaro/qemu-from-laptop/qemu/linux-user/signal.c:4611:9: error: variable 'sr_addr' is used
      uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
    if (!lock_user_struct(VERIFY_READ, sc, sc_addr, 1))
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/petmay01/linaro/qemu-from-laptop/qemu/linux-user/signal.c:4636:28: note: uninitialized use occurs here
    unlock_user_struct(sr, sr_addr, 1);
                           ^~~~~~~
/home/petmay01/linaro/qemu-from-laptop/qemu/linux-user/qemu.h:442:27: note: expanded from macro 'unlock_user_struct'
    unlock_user(host_ptr, guest_addr, (copy) ? sizeof(*host_ptr) : 0)
                          ^

This happens when we unlock a user struct which we never
attempted to lock. Strictly, clang is actually wrong here -- it
hasn't been able to spot that unlock_user_struct() doesn't use
its second argument if the first is NULL. However it doesn't
seem too unreasonable to demand that we pass in initialized
values to it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/signal.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index a5e8906..d63777d 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -4603,7 +4603,7 @@ long do_sigreturn(CPUPPCState *env)
 {
     struct target_sigcontext *sc = NULL;
     struct target_mcontext *sr = NULL;
-    target_ulong sr_addr, sc_addr;
+    target_ulong sr_addr = 0, sc_addr;
     sigset_t blocked;
     target_sigset_t set;
 
-- 
1.7.9.5

[Qemu-devel] [PATCH for-1.6 2/2] linux-user/signal.c: Avoid using uninitialized data in ARM sigreturn

[Peter Maydell]

Rephrase code used in ARM sigreturn functions to avoid using
uninitialized variables. This fixes one genuine problem ('frame'
would not be initialized if we took the error-exit path because
our stackpointer was misaligned) and one which is clang being
alarmist (frame_addr wouldn't be initialized, though this is
harmless since unlock_user_struct ignores its second argument
in these cases; however since we don't generally make use of
this not-really-documented effect it's better avoided).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/signal.c |   36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index d63777d..23d65da 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -1552,7 +1552,7 @@ restore_sigcontext(CPUARMState *env, struct target_sigcontext *sc)
 static long do_sigreturn_v1(CPUARMState *env)
 {
         abi_ulong frame_addr;
-	struct sigframe_v1 *frame;
+        struct sigframe_v1 *frame = NULL;
 	target_sigset_t set;
         sigset_t host_set;
         int i;
@@ -1562,10 +1562,11 @@ static long do_sigreturn_v1(CPUARMState *env)
 	 * then 'sp' should be word aligned here.  If it's
 	 * not, then the user is trying to mess with us.
 	 */
-	if (env->regs[13] & 7)
-		goto badframe;
-
         frame_addr = env->regs[13];
+        if (frame_addr & 7) {
+            goto badframe;
+        }
+
 	if (!lock_user_struct(VERIFY_READ, frame, frame_addr, 1))
                 goto badframe;
 
@@ -1693,17 +1694,18 @@ static int do_sigframe_return_v2(CPUARMState *env, target_ulong frame_addr,
 static long do_sigreturn_v2(CPUARMState *env)
 {
         abi_ulong frame_addr;
-	struct sigframe_v2 *frame;
+        struct sigframe_v2 *frame = NULL;
 
 	/*
 	 * Since we stacked the signal on a 64-bit boundary,
 	 * then 'sp' should be word aligned here.  If it's
 	 * not, then the user is trying to mess with us.
 	 */
-	if (env->regs[13] & 7)
-		goto badframe;
-
         frame_addr = env->regs[13];
+        if (frame_addr & 7) {
+            goto badframe;
+        }
+
 	if (!lock_user_struct(VERIFY_READ, frame, frame_addr, 1))
                 goto badframe;
 
@@ -1731,7 +1733,7 @@ long do_sigreturn(CPUARMState *env)
 static long do_rt_sigreturn_v1(CPUARMState *env)
 {
         abi_ulong frame_addr;
-	struct rt_sigframe_v1 *frame;
+        struct rt_sigframe_v1 *frame = NULL;
         sigset_t host_set;
 
 	/*
@@ -1739,10 +1741,11 @@ static long do_rt_sigreturn_v1(CPUARMState *env)
 	 * then 'sp' should be word aligned here.  If it's
 	 * not, then the user is trying to mess with us.
 	 */
-	if (env->regs[13] & 7)
-		goto badframe;
-
         frame_addr = env->regs[13];
+        if (frame_addr & 7) {
+            goto badframe;
+        }
+
 	if (!lock_user_struct(VERIFY_READ, frame, frame_addr, 1))
                 goto badframe;
 
@@ -1772,17 +1775,18 @@ badframe:
 static long do_rt_sigreturn_v2(CPUARMState *env)
 {
         abi_ulong frame_addr;
-	struct rt_sigframe_v2 *frame;
+        struct rt_sigframe_v2 *frame = NULL;
 
 	/*
 	 * Since we stacked the signal on a 64-bit boundary,
 	 * then 'sp' should be word aligned here.  If it's
 	 * not, then the user is trying to mess with us.
 	 */
-	if (env->regs[13] & 7)
-		goto badframe;
-
         frame_addr = env->regs[13];
+        if (frame_addr & 7) {
+            goto badframe;
+        }
+
 	if (!lock_user_struct(VERIFY_READ, frame, frame_addr, 1))
                 goto badframe;
 
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH for-1.6 0/2] Fix clang warnings in linux-user signal code

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori