This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] PowerPC64 - Fix INTERNAL_[V]SYSCALL_NCS macros to notcast return val to (int)
On Fri, 2011-02-11 at 16:52 -0800, Roland McGrath wrote:
> The test does not meet GNU formatting standards in the common trivial ways
> that are obvious and easy to get right if you only look. It lacks the
> normal glibc copyright header and file comment.
Corrected the formatting. Regarding the copyright and file comment, the
precedent in the tst-* files I looked at is that this information isn't
present. Perhaps this isn't a reason to not have it in this testcase?
I included it in the new version.
> It uses two different literals of different sorts for the same constant,
> which is just nutty. The clean way to write that is INT32_MAX+1, and then
> of course use the same macro or variable in both places.
Fixed, and it requires a (ssize_t) cast to prevent overflow warnings as
well.
> Using freopen there is just gratuitously strange. There is no reason not
> to simply use plain open and write to the returned fd.
Fixed.
> It assumes you can malloc 2GB in one chunk, which is not necessarily going
> to work for everybody. You can test the same thing by allocating one
> buffer of 2GB/UIO_MAXIOV and filling UIO_MAXIOV iovecs with the same
> pointer.
Fixed, though I have a question, what's the difference between
UIO_MAXIOV and IOV_MAX? POSIX specifies using IOV_MAX in the writev
documentation but I used UIO_MAXIOV as you indicated.
I've come up with:
/* writev should report that it has written EXPECTED number of bytes. */
#define EXPECTED ((ssize_t)INT32_MAX+1)
/* How big does each buffer have to be to fit EXPECTED into UIO_MAXIOV iovec
* structs? We do this because not all systems will support an EXPECTED size
* malloc. */
const ssize_t bufsz = (EXPECTED / (UIO_MAXIOV - 1));
/* The last buffer is only partially full. */
const ssize_t bufrem = (EXPECTED % (UIO_MAXIOV - 1));
> It uses fprintf where printf is the natural thing to use.
> The correct format string for ssize_t values is %zd.
Fixed.
Thanks for the review Roland. I was a bit too hasty turning a bug
report testcase into a GLIBC testcase.
Ryan
2011-02-13 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 be803dae40e734719fbac4d2aac59140e8f5aa08
Author: Ryan S. Arnold <rsa@us.ibm.com>
Date: Sun Feb 13 16:27:10 2011 -0600
Remove erroneous (int) cast from PowerPC64 INTERNAL_SYSCALL macros.
The PowerPC64 implementation of INTERNAL_[V]SYSCALL_NCS erroneously casts 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'. This patch
also adds a testcase which proves correct behavior for wordsize-64.
diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h b/sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
index aab4b72..3e57685 100644
--- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
+++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
@@ -1,5 +1,5 @@
-/* Copyright (C) 1992,1997,1998,1999,2000,2001,2002,2003,2004,2005,2006
- Free Software Foundation, Inc.
+/* Copyright (C) 1992,1997,1998,1999,2000,2001,2002,2003,2004,2005,2006,2011
+ Free Software Foundation, Inc.
This file is part of the GNU C Library.
The GNU C Library is free software; you can redistribute it and/or
@@ -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..3197cde
--- /dev/null
+++ b/sysdeps/wordsize-64/tst-writev.c
@@ -0,0 +1,84 @@
+/* Copyright (C) 2011 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+ Contributed by Ryan S. Arnold <rsa@us.ibm.com>, 2011.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, write to the Free
+ Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
+ 02111-1307 USA. */
+
+/* The purpose of this test is to verify that the INTERNAL_[V]SYSCALL_NCS macros
+ * on 64-bit platforms 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'. */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/uio.h>
+
+/* writev should report that it has written EXPECTED number of bytes. */
+#define EXPECTED ((ssize_t)INT32_MAX+1)
+
+/* How big does each buffer have to be to fit EXPECTED into UIO_MAXIOV iovec
+ * structs? We do this because not all systems will support an EXPECTED size
+ * malloc. */
+const ssize_t bufsz = (EXPECTED / (UIO_MAXIOV - 1));
+
+/* The last buffer is only partially full. */
+const ssize_t bufrem = (EXPECTED % (UIO_MAXIOV - 1));
+
+static int
+do_test (void)
+{
+ FILE *fp;
+ struct iovec iv[UIO_MAXIOV];
+ ssize_t ret;
+ int i;
+
+ /* We'll write a bunch of junk to /dev/null with the writev syscall in order
+ * to get a return of INT32_MAX+1 bytes to verify that the INTERNAL_SYSCALL
+ * wrappers aren't mangling the result if the signbit of a 32-bit number is
+ * set. */
+ if ((fp=fopen ("/dev/null", "a")) == NULL)
+ {
+ printf("Unable to open /dev/null for appending.\n");
+ return -1;
+ }
+
+ iv[0].iov_base = malloc(bufsz);
+ iv[0].iov_len = bufsz;
+ for (i=1;i<(UIO_MAXIOV-1);i++)
+ {
+ /* We don't care what the data is so reuse the allocation from iv[0]; */
+ iv[i].iov_base = iv[0].iov_base;
+ iv[i].iov_len = iv[0].iov_len;
+ }
+
+ /* The last buffer may only be partially needed. */
+ iv[UIO_MAXIOV-1].iov_base = iv[0].iov_base;
+ iv[UIO_MAXIOV-1].iov_len = bufrem;
+
+ /* Write it to /dev/null. */
+ ret = writev(fileno(fp), iv, UIO_MAXIOV);
+ free(iv[0].iov_base);
+ fclose(fp);
+ if (ret != EXPECTED)
+ {
+ printf("writev() return value: %zd != EXPECTED: %zd\n", ret,EXPECTED);
+ return ret;
+ }
+ return 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"