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 Tue, 2011-02-15 at 13:15 -0800, Roland McGrath wrote:
> There are some extraneous parens in several places, but that doesn't matter
> much. The test fails to check for malloc failure. If it does fail, writev
> will either fail with EFAULT or crash anyway, so that is no critical. The
> return value from main on failure should be just 1 or 2 or something, not
> what might be an arbitrary large number.
I added a malloc failure test.
I've removed the extraneous parens.
The failure return value is now '1' rather than an arbitrary large
number.
> Otherwise the test looks good to me now.
Great.
> I don't know if the actual fix in the powerpc code might have some other
> ramifications and someone else would have to say about that. Since the
> other syscall macros return a long int value, I assume it's probably fine.
The VSYSCALL macros only seem to be used for time based functions which
return int and will get an implicit cast anway.
All of the generated syscalls from syscalls.list files are ok since
these macros don't use the INLINE_SYSCALL macros to generate code.
Everything that's not generated that returns an int is going to get an
implicit cast anyway.
I suspect the following may have the same problem as writev():
readv()
preadv()
pread() and pread64()
pwrite() and pwrite64()
msgrcv()
shmat()
ptrace() under PTRACE_PEEK*
Ryan S. Arnold
IBM Linux Technology Center
2011-02-16 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: New file that verifies that
writev() returns the valid number of written bytes in the return
value, i.e., it makes sure the ret val isn't mangled by the syscall
macros.
commit
Author: Ryan Arnold <rsa@us.ibm.com>
Date: Wed Feb 16 09:38:03 2011 -0600
Remove erroneous (int) cast from PowerPC64 INTERNAL_[V]SYSCALL_NCS 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..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..e9bda20
--- /dev/null
+++ b/sysdeps/wordsize-64/tst-writev.c
@@ -0,0 +1,108 @@
+/* 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. */
+
+#include <stdio.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <sys/uio.h>
+
+#include <errno.h>
+
+/* 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'. */
+
+#if 0
+/* Used to test the non power-of-2 code path. */
+#undef IOV_MAX
+#define IOV_MAX 1000
+#endif
+
+/* writev() should report that it has written EXPECTED number of bytes. */
+#define EXPECTED ((size_t) INT32_MAX + 1)
+
+static int
+do_test (void)
+{
+ int fd;
+ struct iovec iv[IOV_MAX];
+ ssize_t ret;
+ int i;
+ /* POSIX doesn't guarantee that IOV_MAX is pow of 2 but we're optimistic. */
+ size_t bufsz = EXPECTED / IOV_MAX;
+ size_t bufrem = EXPECTED % IOV_MAX;
+
+ /* If there's a remainder then IOV_MAX probably isn't a power of 2 and we
+ need to make bufsz bigger so that the last iovec, iv[IOV_MAX-1], is free
+ for the remainder. */
+ if (bufrem)
+ {
+ bufsz = bufsz + 1;
+ bufrem = EXPECTED - (bufsz * (IOV_MAX - 1));
+ }
+
+ /* We writev to /dev/null since we're just testing writev's return value. */
+ if ((fd = open ("/dev/null", O_WRONLY)) == -1)
+ {
+ printf ("Unable to open /dev/null for appending.\n");
+ return -1;
+ }
+
+ if ((iv[0].iov_base = malloc (bufsz)) == NULL)
+ {
+ printf ("malloc (%zu) failed.\n", bufsz);
+ close (fd);
+ return -1;
+ }
+ iv[0].iov_len = bufsz;
+
+ /* We optimistically presume that there isn't a remainder and set all iovec
+ instances to the same base and len as the first instance. */
+ for (i = 1; i < IOV_MAX; 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;
+ }
+
+ /* If there is a remainder then we correct the last iov_len. */
+ if (bufrem)
+ iv[IOV_MAX - 1].iov_len = bufrem;
+
+ /* Write 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. */
+ ret = writev (fd, iv, IOV_MAX);
+
+ free (iv[0].iov_base);
+ close (fd);
+
+ if (ret != (ssize_t) EXPECTED)
+ {
+ printf ("writev() return value: %zd != EXPECTED: %zd\n", ret, EXPECTED);
+ return 1;
+ }
+
+ return 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"