[Qemu-devel] [PATCH] linux-user: Fix conversion of sigevent argument to timer_create

* [Qemu-devel] [PATCH] linux-user: Fix conversion of sigevent argument to timer_create
@ 2014-08-09 14:42 Peter Maydell
  2014-08-12  7:37 ` Riku Voipio
  0 siblings, 1 reply; 2+ messages in thread
From: Peter Maydell @ 2014-08-09 14:42 UTC (permalink / raw)
  To: qemu-devel; +Cc: Riku Voipio, Erik de Castro Lopo, patches

There were a number of bugs in the conversion of the sigevent
argument to timer_create from target to host format:
 * signal number not converted from target to host
 * thread ID not copied across
 * sigev_value not copied across
 * we never unlocked the struct when we were done

Between them, these problems meant that SIGEV_THREAD_ID
timers (and the glibc-implemented SIGEV_THREAD timers which
depend on them) didn't work.

Fix these problems and clean up the code a little by pulling
the struct conversion out into its own function, in line with
how we convert various other structs. This allows the test
program in bug LP:1042388 to run.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Riku: this is going to conflict with and supersede Erik's oneliner
patch which adds the unlock_user_struct() (I'm afraid I forgot about
that when I was writing this and independently re-fixed the bug),
so it might be simplest to drop that from the linux-user queue if
you apply this instead. Otherwise if that patch makes it to master
I'll rebase and resend this one at that point.

 linux-user/syscall.c | 38 ++++++++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index a50229d..4ce7455 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -4914,6 +4914,32 @@ static inline abi_long host_to_target_itimerspec(abi_ulong target_addr,
     return 0;
 }
 
+static inline abi_long target_to_host_sigevent(struct sigevent *host_sevp,
+                                               abi_ulong target_addr)
+{
+    struct target_sigevent *target_sevp;
+
+    if (!lock_user_struct(VERIFY_READ, target_sevp, target_addr, 1)) {
+        return -TARGET_EFAULT;
+    }
+
+    /* This union is awkward on 64 bit systems because it has a 32 bit
+     * integer and a pointer in it; we follow the conversion approach
+     * used for handling sigval types in signal.c so the guest should get
+     * the correct value back even if we did a 64 bit byteswap and it's
+     * using the 32 bit integer.
+     */
+    host_sevp->sigev_value.sival_ptr =
+        (void *)(uintptr_t)tswapal(target_sevp->sigev_value.sival_ptr);
+    host_sevp->sigev_signo =
+        target_to_host_signal(tswap32(target_sevp->sigev_signo));
+    host_sevp->sigev_notify = tswap32(target_sevp->sigev_notify);
+    host_sevp->_sigev_un._tid = tswap32(target_sevp->_sigev_un._tid);
+
+    unlock_user_struct(target_sevp, target_addr, 1);
+    return 0;
+}
+
 #if defined(TARGET_NR_stat64) || defined(TARGET_NR_newfstatat)
 static inline abi_long host_to_target_stat64(void *cpu_env,
                                              abi_ulong target_addr,
@@ -9413,7 +9439,6 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         /* args: clockid_t clockid, struct sigevent *sevp, timer_t *timerid */
 
         struct sigevent host_sevp = { {0}, }, *phost_sevp = NULL;
-        struct target_sigevent *ptarget_sevp;
         struct target_timer_t *ptarget_timer;
 
         int clkid = arg1;
@@ -9425,14 +9450,11 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
             timer_t *phtimer = g_posix_timers  + timer_index;
 
             if (arg2) {
-                if (!lock_user_struct(VERIFY_READ, ptarget_sevp, arg2, 1)) {
-                    goto efault;
-                }
-
-                host_sevp.sigev_signo = tswap32(ptarget_sevp->sigev_signo);
-                host_sevp.sigev_notify = tswap32(ptarget_sevp->sigev_notify);
-
                 phost_sevp = &host_sevp;
+                ret = target_to_host_sigevent(phost_sevp, arg2);
+                if (ret != 0) {
+                    break;
+                }
             }
 
             ret = get_errno(timer_create(clkid, phost_sevp, phtimer));
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 2+ messages in thread