From c5d028fc913de84a781bd61084bf7ae2182fd48e Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Thu, 10 Jan 2013 14:42:14 -0800 Subject: [PATCH] Only have one copy of the kernel_sigset_t hack, and add more tests. Change-Id: I377522fcba6fb4b5fd2754ab15b091014bd7c16f --- libc/Android.mk | 3 +- libc/bionic/pthread.c | 52 --------------------- libc/bionic/pthread_sigmask.cpp | 62 ++++++++++++++++++++++++++ libc/bionic/signalfd.cpp | 26 +++-------- libc/bionic/{sigwait.c => sigwait.cpp} | 62 +++++++------------------- libc/private/kernel_sigset_t.h | 52 +++++++++++++++++++++ tests/pthread_test.cpp | 28 ++++++++++++ tests/signal_test.cpp | 24 ++++++++++ 8 files changed, 189 insertions(+), 120 deletions(-) create mode 100644 libc/bionic/pthread_sigmask.cpp rename libc/bionic/{sigwait.c => sigwait.cpp} (50%) create mode 100644 libc/private/kernel_sigset_t.h diff --git a/libc/Android.mk b/libc/Android.mk index 45297b5af..360bdc1cf 100644 --- a/libc/Android.mk +++ b/libc/Android.mk @@ -222,7 +222,6 @@ libc_common_src_files := \ bionic/signame.c \ bionic/sigsetmask.c \ bionic/sigsuspend.c \ - bionic/sigwait.c \ bionic/sleep.c \ bionic/statfs.c \ bionic/strcoll.c \ @@ -281,10 +280,12 @@ libc_bionic_src_files := \ bionic/__memcpy_chk.cpp \ bionic/__memmove_chk.cpp \ bionic/__memset_chk.cpp \ + bionic/pthread_sigmask.cpp \ bionic/raise.cpp \ bionic/__set_errno.cpp \ bionic/setlocale.cpp \ bionic/signalfd.cpp \ + bionic/sigwait.cpp \ bionic/__strcat_chk.cpp \ bionic/__strcpy_chk.cpp \ bionic/strerror.cpp \ diff --git a/libc/bionic/pthread.c b/libc/bionic/pthread.c index b685f2dcd..92ee4aa76 100644 --- a/libc/bionic/pthread.c +++ b/libc/bionic/pthread.c @@ -2099,58 +2099,6 @@ int pthread_kill(pthread_t tid, int sig) return ret; } -/* Despite the fact that our kernel headers define sigset_t explicitly - * as a 32-bit integer, the kernel system call really expects a 64-bit - * bitmap for the signal set, or more exactly an array of two-32-bit - * values (see $KERNEL/arch/$ARCH/include/asm/signal.h for details). - * - * Unfortunately, we cannot fix the sigset_t definition without breaking - * the C library ABI, so perform a little runtime translation here. - */ -typedef union { - sigset_t bionic; - uint32_t kernel[2]; -} kernel_sigset_t; - -/* this is a private syscall stub */ -extern int __rt_sigprocmask(int, const kernel_sigset_t *, kernel_sigset_t *, size_t); - -int pthread_sigmask(int how, const sigset_t *set, sigset_t *oset) -{ - /* pthread_sigmask must return the error code, but the syscall - * will set errno instead and return 0/-1 - */ - int ret, old_errno = errno; - - /* We must convert *set into a kernel_sigset_t */ - kernel_sigset_t in_set, *in_set_ptr; - kernel_sigset_t out_set; - - in_set.kernel[0] = in_set.kernel[1] = 0; - out_set.kernel[0] = out_set.kernel[1] = 0; - - /* 'in_set_ptr' is the second parameter to __rt_sigprocmask. It must be NULL - * if 'set' is NULL to ensure correct semantics (which in this case would - * be to ignore 'how' and return the current signal set into 'oset'. - */ - if (set == NULL) { - in_set_ptr = NULL; - } else { - in_set.bionic = *set; - in_set_ptr = &in_set; - } - - ret = __rt_sigprocmask(how, in_set_ptr, &out_set, sizeof(kernel_sigset_t)); - if (ret < 0) - ret = errno; - - if (oset) - *oset = out_set.bionic; - - errno = old_errno; - return ret; -} - int pthread_getcpuclockid(pthread_t tid, clockid_t *clockid) { diff --git a/libc/bionic/pthread_sigmask.cpp b/libc/bionic/pthread_sigmask.cpp new file mode 100644 index 000000000..9c9dc3ed8 --- /dev/null +++ b/libc/bionic/pthread_sigmask.cpp @@ -0,0 +1,62 @@ +/* + * Copyright (C) 2008 The Android Open Source Project + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in + * the documentation and/or other materials provided with the + * distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE + * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, + * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS + * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED + * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +#include +#include +#include + +#include + +extern "C" int __rt_sigprocmask(int, const kernel_sigset_t*, kernel_sigset_t*, size_t); + +int pthread_sigmask(int how, const sigset_t* iset, sigset_t* oset) { + int old_errno = errno; + + // 'in_set_ptr' is the second parameter to __rt_sigprocmask. It must be NULL + // if 'set' is NULL to ensure correct semantics (which in this case would + // be to ignore 'how' and return the current signal set into 'oset'). + kernel_sigset_t in_set; + kernel_sigset_t* in_set_ptr = NULL; + if (iset != NULL) { + in_set.set(iset); + in_set_ptr = &in_set; + } + + kernel_sigset_t out_set; + int result = __rt_sigprocmask(how, in_set_ptr, &out_set, sizeof(out_set)); + if (result < 0) { + result = errno; + } + + if (oset != NULL) { + *oset = out_set.bionic; + } + + errno = old_errno; + return result; +} diff --git a/libc/bionic/signalfd.cpp b/libc/bionic/signalfd.cpp index 51fae8361..b7e647412 100644 --- a/libc/bionic/signalfd.cpp +++ b/libc/bionic/signalfd.cpp @@ -28,27 +28,11 @@ #include -/* Despite the fact that our kernel headers define sigset_t explicitly - * as a 32-bit integer, the kernel system call really expects a 64-bit - * bitmap for the signal set, or more exactly an array of two-32-bit - * values (see $KERNEL/arch/$ARCH/include/asm/signal.h for details). - * - * Unfortunately, we cannot fix the sigset_t definition without breaking - * the C library ABI, so perform a little runtime translation here. - */ -typedef union { - sigset_t bionic; - uint32_t kernel[2]; -} kernel_sigset_t; +#include -extern "C" int signalfd4(int fd, kernel_sigset_t *mask, size_t sizemask, int flags); +extern "C" int signalfd4(int fd, kernel_sigset_t* mask, size_t sizemask, int flags); -int signalfd(int fd, const sigset_t *mask, int flags) -{ - kernel_sigset_t in_set; - in_set.kernel[0] = in_set.kernel[1] = 0; - - in_set.bionic = *mask; - - return signalfd4(fd, &in_set, sizeof(in_set), flags); +int signalfd(int fd, const sigset_t* mask, int flags) { + kernel_sigset_t in_set(mask); + return signalfd4(fd, &in_set, sizeof(in_set), flags); } diff --git a/libc/bionic/sigwait.c b/libc/bionic/sigwait.cpp similarity index 50% rename from libc/bionic/sigwait.c rename to libc/bionic/sigwait.cpp index 1e90c41df..1546fd6e9 100644 --- a/libc/bionic/sigwait.c +++ b/libc/bionic/sigwait.cpp @@ -25,59 +25,29 @@ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF * SUCH DAMAGE. */ + #include #include #include #include -int __rt_sigtimedwait(const sigset_t *uthese, siginfo_t *uinfo, const struct timespec *uts, size_t sigsetsize); +#include -/* ok, this is really subtle: defines sigset_t differently - * when you're in the kernel or in the C library. - * - * in the kernel, this is an array of 2 32-bit unsigned longs - * in the C library, this is a single 32-bit unsigned long - * - * moreover, the kernel implementation of rt_sigtimedwait doesn't - * accept anything except kernel-sized signal sets (probably a bug !) - * - * we thus need to create a fake kernel sigset !! - */ +extern "C" int __rt_sigtimedwait(const sigset_t* uthese, siginfo_t* uinfo, const struct timespec* uts, size_t sigsetsize); -int sigwait(const sigset_t *set, int *sig) -{ - int ret; -#ifdef __mips__ - /* use a union to get rid of aliasing warnings. On MIPS sigset_t is 128 bits */ - union { - sigset_t kernel_sigset; - sigset_t dummy_sigset; - } u; - u.dummy_sigset = *set; -#else - /* use a union to get rid of aliasing warnings */ - union { - unsigned long kernel_sigset[2]; - sigset_t dummy_sigset; - } u; - - u.kernel_sigset[0] = *set; - u.kernel_sigset[1] = 0; /* no real-time signals supported ? */ -#endif - for (;;) - { - /* __rt_sigtimedwait can return EAGAIN or EINTR, we need to loop - * around them since sigwait is only allowed to return EINVAL - */ - ret = __rt_sigtimedwait ( &u.dummy_sigset, NULL, NULL, sizeof(u.kernel_sigset)); - if (ret >= 0) - break; - - if (errno != EAGAIN && errno != EINTR) - return errno; +int sigwait(const sigset_t* set, int* sig) { + kernel_sigset_t sigset(set); + while (true) { + // __rt_sigtimedwait can return EAGAIN or EINTR, we need to loop + // around them since sigwait is only allowed to return EINVAL. + int result = __rt_sigtimedwait(sigset.get(), NULL, NULL, sizeof(sigset)); + if (result >= 0) { + *sig = result; + return 0; } - *sig = ret; - return 0; + if (errno != EAGAIN && errno != EINTR) { + return errno; + } + } } - diff --git a/libc/private/kernel_sigset_t.h b/libc/private/kernel_sigset_t.h new file mode 100644 index 000000000..733a84256 --- /dev/null +++ b/libc/private/kernel_sigset_t.h @@ -0,0 +1,52 @@ +/* + * Copyright (C) 2013 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef LIBC_PRIVATE_KERNEL_SIGSET_T_H_ +#define LIBC_PRIVATE_KERNEL_SIGSET_T_H_ + +// Our sigset_t is wrong for ARM and x86. It's 32-bit but the kernel expects 64 bits. +// This means we can't support real-time signals correctly until we can change the ABI. +// In the meantime, we can use this union to pass an appropriately-sized block of memory +// to the kernel, at the cost of not being able to refer to real-time signals. +union kernel_sigset_t { + kernel_sigset_t() { + clear(); + } + + kernel_sigset_t(const sigset_t* value) { + clear(); + set(value); + } + + void clear() { + memset(this, 0, sizeof(*this)); + } + + void set(const sigset_t* value) { + bionic = *value; + } + + sigset_t* get() { + return &bionic; + } + + sigset_t bionic; +#ifndef __mips__ + uint32_t kernel[2]; +#endif +}; + +#endif diff --git a/tests/pthread_test.cpp b/tests/pthread_test.cpp index da945b4df..3e144dbaa 100644 --- a/tests/pthread_test.cpp +++ b/tests/pthread_test.cpp @@ -126,3 +126,31 @@ TEST(pthread_DeathTest, pthread_bug_37410) { EXPECT_EXIT(TestBug37410(), ::testing::ExitedWithCode(0), ""); } #endif + +static void* SignalHandlerFn(void* arg) { + sigset_t wait_set; + sigfillset(&wait_set); + return reinterpret_cast(sigwait(&wait_set, reinterpret_cast(arg))); +} + +TEST(pthread, pthread_sigmask) { + // Block SIGUSR1. + sigset_t set; + sigemptyset(&set); + sigaddset(&set, SIGUSR1); + ASSERT_EQ(0, pthread_sigmask(SIG_BLOCK, &set, NULL)); + + // Spawn a thread that calls sigwait and tells us what it received. + pthread_t signal_thread; + int received_signal = -1; + ASSERT_EQ(0, pthread_create(&signal_thread, NULL, SignalHandlerFn, &received_signal)); + + // Send that thread SIGUSR1. + pthread_kill(signal_thread, SIGUSR1); + + // See what it got. + void* join_result; + ASSERT_EQ(0, pthread_join(signal_thread, &join_result)); + ASSERT_EQ(SIGUSR1, received_signal); + ASSERT_EQ(0, reinterpret_cast(join_result)); +} diff --git a/tests/signal_test.cpp b/tests/signal_test.cpp index 129256890..b100372c9 100644 --- a/tests/signal_test.cpp +++ b/tests/signal_test.cpp @@ -101,3 +101,27 @@ TEST(signal, raise_invalid) { ASSERT_EQ(-1, raise(-1)); ASSERT_EQ(EINVAL, errno); } + +static void HandleSIGALRM(int signal_number) { + ASSERT_EQ(SIGALRM, signal_number); +} + +TEST(signal, sigwait) { + struct sigaction action; + sigemptyset(&action.sa_mask); + action.sa_flags = 0; + action.sa_handler = HandleSIGALRM; + sigaction(SIGALRM, &action, NULL); + + sigset_t wait_set; + sigemptyset(&wait_set); + sigaddset(&wait_set, SIGALRM); + + alarm(1); + + int received_signal; + errno = 0; + ASSERT_EQ(0, sigwait(&wait_set, &received_signal)); + ASSERT_EQ(0, errno); + ASSERT_EQ(SIGALRM, received_signal); +}