Re: [PATCH] PowerPC64 - Fix INTERNAL_[V]SYSCALL_NCS macros to notcast return val to (int)

[Ryan Arnold <rsa at us dot ibm dot com>]

On Fri, 2011-02-11 at 12:15 -0800, Roland McGrath wrote:
> > Sure, I can do this.  I originally had it in misc/ directly but it
> > didn't seem correct there either.
> 
> It is correct here if it's written cleanly in a way that applies to all
> configurations.

Here's an updated patch with the test-case moved to the wordsize-64/
directory.  I haven't had a chance to test this on x86_64 yet.

Ryan S. Arnold
IBM Linux Technology Center

2011-02-11  Ryan S. Arnold  <rsa@us.ibm.com>

	* sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h:
	(INTERNAL_VSYSCALL_NCS INTERNAL_SYSCALL_NCS): Remove erroneous (int)
	cast from r3.
	* sysdeps/wordsize-64/Makefile: New file that adds tst-writev to
	'tests' variable.
	* sysdeps/wordsize-64/tst-writev.c: Verify that writev() returns the
	valid number of written bytes in the return value, i.e., make sure the
	ret val isn't mangled by the syscall wrapper.

commit 659c765a713f89364d13a6c808939213f33970af
Author: Ryan Arnold <rsa@us.ibm.com>
Date:   Fri Feb 11 17:38:26 2011 -0600

    Remove erroneous (int) cast from PowerPC64 INTERNAL_SYSCALL macros.
    
    The PowerPC64 implementation of INTERNAL_VSYSCALL_NCS and INTERNAL_SYSCALL_NCS
    erroneously cast the return value held in r3 to (int).  Normally this wasn't a
    problem but for syscalls wrappers that require a ssize_t return, like writev()
    this bug would mangle the high half word of the return value and even
    erroneously sign-extend the high half word if the sign-bit of the low half
    word was '1'.

diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h b/sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
index aab4b72..e714c4c 100644
--- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
+++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
@@ -172,7 +172,7 @@
        : "r9", "r10", "r11", "r12",					\
          "cr0", "ctr", "lr", "memory");					\
 	  err = (long int) r0;						\
-    (int) r3;								\
+    r3;								\
   })
 
 #undef INLINE_SYSCALL
@@ -219,7 +219,7 @@
        : "r9", "r10", "r11", "r12",					\
          "cr0", "ctr", "memory");					\
 	  err = r0;  \
-    (int) r3;  \
+    r3;  \
   })
 #define INTERNAL_SYSCALL(name, err, nr, args...)			\
   INTERNAL_SYSCALL_NCS (__NR_##name, err, nr, args)
diff --git a/sysdeps/wordsize-64/Makefile b/sysdeps/wordsize-64/Makefile
new file mode 100644
index 0000000..9903f51
--- /dev/null
+++ b/sysdeps/wordsize-64/Makefile
@@ -0,0 +1,6 @@
+ifeq ($(subdir),misc)
+tests += tst-writev
+
+# Time enough for a large writev syscall to complete.
+tst-writev-ENV = TIMEOUTFACTOR="10"
+endif
diff --git a/sysdeps/wordsize-64/tst-writev.c b/sysdeps/wordsize-64/tst-writev.c
new file mode 100644
index 0000000..2eee27a
--- /dev/null
+++ b/sysdeps/wordsize-64/tst-writev.c
@@ -0,0 +1,35 @@
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/uio.h>
+
+/* The purpose of this test is to verify that the INTERNAL_[V]SYSCALL_NCS macros
+ * don't cast the return type to (int) which would erroneously sign extend the
+ * return value should the high bit of the bottom half of the word be '1'.  */
+
+#define bufsz (unsigned long)(0x1l << 31)
+
+static int
+do_test (void)
+{
+  struct iovec iv[1];
+  ssize_t ret;
+
+  /* Write a bunch of junk to /dev/null with the writev syscall in order to get
+   * a return of 0x8000000 bytes to verify that the INTERNAL_SYSCALL wrappers
+   * aren't mangling the result if the signbit of a 32-bit number is set.  */
+  (void) freopen ("/dev/null", "a", stderr);
+
+   iv[0].iov_base = malloc(bufsz);
+   iv[0].iov_len = bufsz;
+
+   /* Write it out to stderr and it'll get dumped into /dev/null.  */
+   ret = writev(2, iv, 1);
+   if (ret != 0x80000000) {
+     fprintf(stdout, "ret = %lu\n", ret);
+     return ret;
+   }
+   return 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"