Better handling of sigset_t on LP32.

The main motivation here is that the sigprocmask in pthread_exit wasn't
actually blocking the real-time signals, and debuggerd (amongst other
things) is using them. I wasn't able to write a test that actually won
that race but I did write an equivalent one for posix_spawn.

This also fixes all the uses of sigset_t where the sigset_t isn't
exposed to the outside (which we can't easily fix because it would be
an ABI change).

Bug: https://issuetracker.google.com/72291624
Test: ran tests
Change-Id: Ib6eebebc5a7b0150079f1cb79593247917dcf750
This commit is contained in:
Elliott Hughes 2018-01-24 18:54:38 -08:00
parent 73871ad09b
commit 4b1c6e7385
18 changed files with 212 additions and 139 deletions

View file

@ -32,6 +32,8 @@
#include <sys/syscall.h>
#include <unistd.h>
#include "private/kernel_sigset_t.h"
// We call tgkill(2) directly instead of raise (or even the libc tgkill wrapper), to reduce the
// number of uninteresting stack frames at the top of a crash.
static inline __always_inline void inline_tgkill(pid_t pid, pid_t tid, int sig) {
@ -60,10 +62,10 @@ void abort() {
// Don't block SIGABRT to give any signal handler a chance; we ignore
// any errors -- X311J doesn't allow abort to return anyway.
sigset_t mask;
sigfillset(&mask);
sigdelset(&mask, SIGABRT);
sigprocmask(SIG_SETMASK, &mask, NULL);
kernel_sigset_t mask;
mask.fill();
mask.clear(SIGABRT);
__rt_sigprocmask(SIG_SETMASK, &mask, nullptr, sizeof(mask));
inline_tgkill(pid, tid, SIGABRT);
@ -74,7 +76,7 @@ void abort() {
sa.sa_flags = SA_RESTART;
sigemptyset(&sa.sa_mask);
sigaction(SIGABRT, &sa, &sa);
sigprocmask(SIG_SETMASK, &mask, NULL);
__rt_sigprocmask(SIG_SETMASK, &mask, nullptr, sizeof(mask));
inline_tgkill(pid, tid, SIGABRT);

View file

@ -30,13 +30,8 @@
#include "private/kernel_sigset_t.h"
extern "C" int __rt_sigprocmask(int, const kernel_sigset_t*, kernel_sigset_t*, size_t);
extern "C" int __rt_sigsuspend(const kernel_sigset_t*, size_t);
int pause() {
kernel_sigset_t mask;
if (__rt_sigprocmask(SIG_SETMASK, NULL, &mask, sizeof(mask)) == -1) {
return -1;
}
if (__rt_sigprocmask(SIG_SETMASK, nullptr, &mask, sizeof(mask)) == -1) return -1;
return __rt_sigsuspend(&mask, sizeof(mask));
}

View file

@ -74,8 +74,7 @@ static __kernel_timer_t to_kernel_timer_id(timer_t timer) {
static void* __timer_thread_start(void* arg) {
PosixTimer* timer = reinterpret_cast<PosixTimer*>(arg);
kernel_sigset_t sigset;
sigaddset(sigset.get(), TIMER_SIGNAL);
kernel_sigset_t sigset{TIMER_SIGNAL};
while (true) {
// Wait for a signal...
@ -150,14 +149,13 @@ int timer_create(clockid_t clock_id, sigevent* evp, timer_t* timer_id) {
// We start the thread with TIMER_SIGNAL blocked by blocking the signal here and letting it
// inherit. If it tried to block the signal itself, there would be a race.
kernel_sigset_t sigset;
sigaddset(sigset.get(), TIMER_SIGNAL);
kernel_sigset_t sigset{TIMER_SIGNAL};
kernel_sigset_t old_sigset;
pthread_sigmask(SIG_BLOCK, sigset.get(), old_sigset.get());
__rt_sigprocmask(SIG_BLOCK, &sigset, &old_sigset, sizeof(sigset));
int rc = pthread_create(&timer->callback_thread, &thread_attributes, __timer_thread_start, timer);
pthread_sigmask(SIG_SETMASK, old_sigset.get(), NULL);
__rt_sigprocmask(SIG_SETMASK, &old_sigset, nullptr, sizeof(sigset));
if (rc != 0) {
free(timer);

View file

@ -34,6 +34,7 @@
#include <sys/mman.h>
#include "private/bionic_defs.h"
#include "private/ScopedSignalBlocker.h"
#include "pthread_internal.h"
extern "C" __noreturn void _exit_with_stack_teardown(void*, size_t);
@ -63,6 +64,12 @@ void __pthread_cleanup_pop(__pthread_cleanup_t* c, int execute) {
}
}
static void __pthread_unmap_tls(pthread_internal_t* thread) {
// Unmap the bionic TLS, including guard pages.
void* allocation = reinterpret_cast<char*>(thread->bionic_tls) - PTHREAD_GUARD_SIZE;
munmap(allocation, BIONIC_TLS_SIZE + 2 * PTHREAD_GUARD_SIZE);
}
__BIONIC_WEAK_FOR_NATIVE_BRIDGE
void pthread_exit(void* return_value) {
// Call dtors for thread_local objects first.
@ -96,10 +103,6 @@ void pthread_exit(void* return_value) {
thread->alternate_signal_stack = NULL;
}
// Unmap the bionic TLS, including guard pages.
void* allocation = reinterpret_cast<char*>(thread->bionic_tls) - PTHREAD_GUARD_SIZE;
munmap(allocation, BIONIC_TLS_SIZE + 2 * PTHREAD_GUARD_SIZE);
ThreadJoinState old_state = THREAD_NOT_JOINED;
while (old_state == THREAD_NOT_JOINED &&
!atomic_compare_exchange_weak(&thread->join_state, &old_state, THREAD_EXITED_NOT_JOINED)) {
@ -120,16 +123,15 @@ void pthread_exit(void* return_value) {
// That's not something we can do in C.
// We don't want to take a signal after we've unmapped the stack.
// That's one last thing we can handle in C.
sigset_t mask;
sigfillset(&mask);
sigprocmask(SIG_SETMASK, &mask, NULL);
// That's one last thing we can do before dropping to assembler.
ScopedSignalBlocker ssb;
__pthread_unmap_tls(thread);
_exit_with_stack_teardown(thread->attr.stack_base, thread->mmap_size);
}
}
// No need to free mapped space. Either there was no space mapped, or it is left for
// the pthread_join caller to clean up.
__pthread_unmap_tls(thread);
__exit(0);
}

View file

@ -25,26 +25,18 @@
* OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE.
*/
#include <signal.h>
/* this function is called from the ARM assembly setjmp fragments */
int
sigblock(int mask)
{
int n;
int sigblock(int mask) {
union {
int the_mask;
sigset_t the_sigset;
int mask;
sigset_t set;
} in, out;
sigemptyset(&in.the_sigset);
in.the_mask = mask;
sigemptyset(&in.set);
in.mask = mask;
n = sigprocmask(SIG_BLOCK, &in.the_sigset, &out.the_sigset);
if (n)
return n;
return out.the_mask;
if (sigprocmask(SIG_BLOCK, &in.set, &out.set) == -1) return -1;
return out.mask;
}

View file

@ -28,9 +28,11 @@
#include <signal.h>
#include "private/kernel_sigset_t.h"
int sighold(int sig) {
sigset_t set;
if (sigemptyset(&set) == -1) return -1;
if (sigaddset(&set, sig) == -1) return -1;
return sigprocmask(SIG_BLOCK, &set, nullptr);
kernel_sigset_t set;
set.clear();
if (!set.set(sig)) return -1;
return __rt_sigprocmask(SIG_BLOCK, &set, nullptr, sizeof(set));
}

View file

@ -28,9 +28,12 @@
#include <signal.h>
#include "private/kernel_sigset_t.h"
int sigpause(int sig) {
sigset_t set;
if (sigprocmask(SIG_SETMASK, nullptr, &set) == -1) return -1;
if (sigdelset(&set, sig) == -1) return -1;
return sigsuspend(&set);
kernel_sigset_t set;
set.clear();
if (__rt_sigprocmask(SIG_SETMASK, nullptr, &set, sizeof(set)) == -1) return -1;
if (!set.clear(sig)) return -1;
return __rt_sigsuspend(&set, sizeof(set));
}

View file

@ -30,8 +30,6 @@
#include "private/kernel_sigset_t.h"
extern "C" int __rt_sigpending(const kernel_sigset_t*, size_t);
int sigpending(sigset_t* bionic_set) {
kernel_sigset_t set;
int result = __rt_sigpending(&set, sizeof(set));

View file

@ -32,8 +32,6 @@
#include "private/kernel_sigset_t.h"
extern "C" int __rt_sigprocmask(int, const kernel_sigset_t*, kernel_sigset_t*, size_t);
int sigprocmask(int how, const sigset_t* bionic_new_set, sigset_t* bionic_old_set) {
kernel_sigset_t new_set;
kernel_sigset_t* new_set_ptr = NULL;

View file

@ -28,9 +28,11 @@
#include <signal.h>
#include "private/kernel_sigset_t.h"
int sigrelse(int sig) {
sigset_t set;
if (sigemptyset(&set) == -1) return -1;
if (sigaddset(&set, sig) == -1) return -1;
return sigprocmask(SIG_UNBLOCK, &set, nullptr);
kernel_sigset_t set;
set.clear();
if (!set.set(sig)) return -1;
return __rt_sigprocmask(SIG_UNBLOCK, &set, nullptr, sizeof(set));
}

View file

@ -29,6 +29,8 @@
#include <signal.h>
#include <string.h>
#include "private/kernel_sigset_t.h"
sighandler_t sigset(int sig, sighandler_t disp) {
struct sigaction new_sa;
if (disp != SIG_HOLD) {
@ -38,19 +40,16 @@ sighandler_t sigset(int sig, sighandler_t disp) {
}
struct sigaction old_sa;
if (sigaction(sig, disp == SIG_HOLD ? nullptr : &new_sa, &old_sa) == -1) {
if (sigaction(sig, (disp == SIG_HOLD) ? nullptr : &new_sa, &old_sa) == -1) {
return SIG_ERR;
}
sigset_t new_proc_mask;
sigemptyset(&new_proc_mask);
sigaddset(&new_proc_mask, sig);
sigset_t old_proc_mask;
if (sigprocmask(disp == SIG_HOLD ? SIG_BLOCK : SIG_UNBLOCK,
&new_proc_mask, &old_proc_mask) == -1) {
kernel_sigset_t new_mask{sig};
kernel_sigset_t old_mask;
if (__rt_sigprocmask(disp == SIG_HOLD ? SIG_BLOCK : SIG_UNBLOCK, &new_mask, &old_mask,
sizeof(new_mask)) == -1) {
return SIG_ERR;
}
return sigismember(&old_proc_mask, sig) ? SIG_HOLD : old_sa.sa_handler;
return old_mask.is_set(sig) ? SIG_HOLD : old_sa.sa_handler;
}

View file

@ -25,26 +25,18 @@
* OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE.
*/
#include <signal.h>
/* called from setjmp assembly fragment */
int
sigsetmask(int mask)
{
int n;
int sigsetmask(int mask) {
union {
int the_mask;
sigset_t the_sigset;
int mask;
sigset_t set;
} in, out;
sigemptyset(&in.the_sigset);
in.the_mask = mask;
sigemptyset(&in.set);
in.mask = mask;
n = sigprocmask(SIG_SETMASK, &in.the_sigset, &out.the_sigset);
if (n)
return n;
return out.the_mask;
if (sigprocmask(SIG_SETMASK, &in.set, &out.set) == -1) return -1;
return out.mask;
}

View file

@ -30,8 +30,6 @@
#include "private/kernel_sigset_t.h"
extern "C" int __rt_sigsuspend(const kernel_sigset_t*, size_t);
int sigsuspend(const sigset_t* bionic_set) {
kernel_sigset_t set(bionic_set);
return __rt_sigsuspend(&set, sizeof(set));

View file

@ -20,13 +20,14 @@
#include <signal.h>
#include "bionic_macros.h"
#include "kernel_sigset_t.h"
class ScopedSignalBlocker {
public:
explicit ScopedSignalBlocker() {
sigset_t set;
sigfillset(&set);
sigprocmask(SIG_BLOCK, &set, &old_set_);
kernel_sigset_t set;
set.fill();
__rt_sigprocmask(SIG_SETMASK, &set, &old_set_, sizeof(set));
}
~ScopedSignalBlocker() {
@ -34,11 +35,11 @@ class ScopedSignalBlocker {
}
void reset() {
sigprocmask(SIG_SETMASK, &old_set_, nullptr);
__rt_sigprocmask(SIG_SETMASK, &old_set_, nullptr, sizeof(old_set_));
}
private:
sigset_t old_set_;
kernel_sigset_t old_set_;
DISALLOW_COPY_AND_ASSIGN(ScopedSignalBlocker);
};

View file

@ -17,18 +17,27 @@
#ifndef LIBC_PRIVATE_KERNEL_SIGSET_T_H_
#define LIBC_PRIVATE_KERNEL_SIGSET_T_H_
#include <errno.h>
#include <signal.h>
#include <async_safe/log.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.
// This means we can't support real-time signals correctly without breaking 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.
// to the kernel, at the cost of not being able to refer to real-time signals when
// initializing from a sigset_t on LP32.
union kernel_sigset_t {
public:
kernel_sigset_t() {
clear();
}
kernel_sigset_t(const sigset_t* value) {
explicit kernel_sigset_t(int signal_number) {
clear();
if (!set(signal_number)) async_safe_fatal("kernel_sigset_t(%d)", signal_number);
}
explicit kernel_sigset_t(const sigset_t* value) {
clear();
set(value);
}
@ -37,7 +46,32 @@ union kernel_sigset_t {
__builtin_memset(this, 0, sizeof(*this));
}
bool clear(int signal_number) {
int bit = bit_of(signal_number);
if (bit == -1) return false;
bits[bit / LONG_BIT] &= ~(1UL << (bit % LONG_BIT));
return true;
}
void fill() {
__builtin_memset(this, 0xff, sizeof(*this));
}
bool is_set(int signal_number) {
int bit = bit_of(signal_number);
if (bit == -1) return false;
return ((bits[bit / LONG_BIT] >> (bit % LONG_BIT)) & 1) == 1;
}
bool set(int signal_number) {
int bit = bit_of(signal_number);
if (bit == -1) return false;
bits[bit / LONG_BIT] |= 1UL << (bit % LONG_BIT);
return true;
}
void set(const sigset_t* value) {
clear();
bionic = *value;
}
@ -46,9 +80,21 @@ union kernel_sigset_t {
}
sigset_t bionic;
#ifndef __mips__
uint32_t kernel[2];
#endif
unsigned long bits[_KERNEL__NSIG/LONG_BIT];
private:
int bit_of(int signal_number) {
int bit = signal_number - 1; // Signal numbers start at 1, but bit positions start at 0.
if (bit < 0 || bit >= static_cast<int>(8*sizeof(*this))) {
errno = EINVAL;
return -1;
}
return bit;
}
};
extern "C" int __rt_sigpending(const kernel_sigset_t*, size_t);
extern "C" int __rt_sigprocmask(int, const kernel_sigset_t*, kernel_sigset_t*, size_t);
extern "C" int __rt_sigsuspend(const kernel_sigset_t*, size_t);
#endif

View file

@ -53,13 +53,13 @@ class ScopedSignalHandler {
const int signal_number_;
};
class ScopedSignalMask {
class SignalMaskRestorer {
public:
ScopedSignalMask() {
SignalMaskRestorer() {
sigprocmask(SIG_SETMASK, nullptr, &old_mask_);
}
~ScopedSignalMask() {
~SignalMaskRestorer() {
sigprocmask(SIG_SETMASK, &old_mask_, nullptr);
}

View file

@ -464,33 +464,45 @@ TEST(signal, sigrelse_EINVAL) {
ASSERT_EQ(EINVAL, errno);
}
TEST(signal, sighold_sigpause_sigrelse) {
static int sigalrm_handler_call_count;
auto sigalrm_handler = [](int) { sigalrm_handler_call_count++; };
ScopedSignalHandler sigalrm{SIGALRM, sigalrm_handler};
ScopedSignalMask mask;
static void TestSigholdSigpauseSigrelse(int sig) {
static int signal_handler_call_count = 0;
ScopedSignalHandler ssh{sig, [](int) { signal_handler_call_count++; }};
SignalMaskRestorer mask_restorer;
sigset_t set;
// sighold(SIGALRM) should add SIGALRM to the signal mask ...
ASSERT_EQ(0, sighold(SIGALRM));
// sighold(SIGALRM/SIGRTMIN) should add SIGALRM/SIGRTMIN to the signal mask ...
ASSERT_EQ(0, sighold(sig));
ASSERT_EQ(0, sigprocmask(SIG_SETMASK, 0, &set));
EXPECT_TRUE(sigismember(&set, SIGALRM));
EXPECT_TRUE(sigismember(&set, sig));
// ... preventing our SIGALRM handler from running ...
raise(SIGALRM);
ASSERT_EQ(0, sigalrm_handler_call_count);
// ... until sigpause(SIGALRM) temporarily unblocks it.
ASSERT_EQ(-1, sigpause(SIGALRM));
// ... preventing our SIGALRM/SIGRTMIN handler from running ...
raise(sig);
ASSERT_EQ(0, signal_handler_call_count);
// ... until sigpause(SIGALRM/SIGRTMIN) temporarily unblocks it.
ASSERT_EQ(-1, sigpause(sig));
ASSERT_EQ(EINTR, errno);
ASSERT_EQ(1, sigalrm_handler_call_count);
ASSERT_EQ(1, signal_handler_call_count);
// But sigpause(SIGALRM) shouldn't permanently unblock SIGALRM.
if (sig >= SIGRTMIN && sizeof(void*) == 8) {
// But sigpause(SIGALRM/SIGRTMIN) shouldn't permanently unblock SIGALRM/SIGRTMIN.
ASSERT_EQ(0, sigprocmask(SIG_SETMASK, 0, &set));
EXPECT_TRUE(sigismember(&set, SIGALRM));
EXPECT_TRUE(sigismember(&set, sig));
ASSERT_EQ(0, sigrelse(SIGALRM));
// Whereas sigrelse(SIGALRM/SIGRTMIN) should.
ASSERT_EQ(0, sigrelse(sig));
ASSERT_EQ(0, sigprocmask(SIG_SETMASK, 0, &set));
EXPECT_FALSE(sigismember(&set, SIGALRM));
EXPECT_FALSE(sigismember(&set, sig));
} else {
// sigismember won't work for SIGRTMIN on LP32.
}
}
TEST(signal, sighold_sigpause_sigrelse) {
TestSigholdSigpauseSigrelse(SIGALRM);
}
TEST(signal, sighold_sigpause_sigrelse_RT) {
TestSigholdSigpauseSigrelse(SIGRTMIN);
}
TEST(signal, sigset_EINVAL) {
@ -499,23 +511,48 @@ TEST(signal, sigset_EINVAL) {
ASSERT_EQ(EINVAL, errno);
}
TEST(signal, sigset) {
auto sigalrm_handler = [](int) { };
ScopedSignalHandler sigalrm{SIGALRM, sigalrm_handler};
ScopedSignalMask mask;
// block SIGALRM so the next sigset(SIGARLM) call will return SIG_HOLD
sigset_t sigalrm_set;
sigemptyset(&sigalrm_set);
sigaddset(&sigalrm_set, SIGALRM);
ASSERT_EQ(0, sigprocmask(SIG_BLOCK, &sigalrm_set, nullptr));
TEST(signal, sigset_RT) {
static int signal_handler_call_count = 0;
auto signal_handler = [](int) { signal_handler_call_count++; };
ScopedSignalHandler ssh{SIGRTMIN, signal_handler};
SignalMaskRestorer mask_restorer;
ASSERT_EQ(signal_handler, sigset(SIGRTMIN, SIG_HOLD));
#if defined(__LP64__)
sigset_t set;
ASSERT_EQ(SIG_HOLD, sigset(SIGALRM, sigalrm_handler));
ASSERT_EQ(0, sigprocmask(SIG_BLOCK, nullptr, &set));
ASSERT_TRUE(sigismember(&set, SIGRTMIN));
#endif
ASSERT_EQ(SIG_HOLD, sigset(SIGRTMIN, signal_handler));
ASSERT_EQ(signal_handler, sigset(SIGRTMIN, signal_handler));
ASSERT_EQ(0, signal_handler_call_count);
raise(SIGRTMIN);
ASSERT_EQ(1, signal_handler_call_count);
}
TEST(signal, sigset) {
static int signal_handler_call_count = 0;
auto signal_handler = [](int) { signal_handler_call_count++; };
ScopedSignalHandler ssh{SIGALRM, signal_handler};
SignalMaskRestorer mask_restorer;
ASSERT_EQ(0, signal_handler_call_count);
raise(SIGALRM);
ASSERT_EQ(1, signal_handler_call_count);
// Block SIGALRM so the next sigset(SIGARLM) call will return SIG_HOLD.
sigset_t set;
sigemptyset(&set);
sigaddset(&set, SIGALRM);
ASSERT_EQ(0, sigprocmask(SIG_BLOCK, &set, nullptr));
sigemptyset(&set);
ASSERT_EQ(SIG_HOLD, sigset(SIGALRM, signal_handler));
ASSERT_EQ(0, sigprocmask(SIG_BLOCK, nullptr, &set));
EXPECT_FALSE(sigismember(&set, SIGALRM));
ASSERT_EQ(sigalrm_handler, sigset(SIGALRM, SIG_IGN));
ASSERT_EQ(signal_handler, sigset(SIGALRM, SIG_IGN));
ASSERT_EQ(0, sigprocmask(SIG_BLOCK, nullptr, &set));
EXPECT_FALSE(sigismember(&set, SIGALRM));

View file

@ -376,6 +376,7 @@ TEST(spawn, posix_spawn_POSIX_SPAWN_SETSIGDEF) {
sigset_t just_SIGALRM;
sigemptyset(&just_SIGALRM);
sigaddset(&just_SIGALRM, SIGALRM);
ASSERT_EQ(0, posix_spawnattr_setsigdefault(&sa, &just_SIGALRM));
ASSERT_EQ(0, posix_spawnattr_setflags(&sa, POSIX_SPAWN_SETSIGDEF));
@ -393,15 +394,18 @@ TEST(spawn, signal_stress) {
// child without first defaulting any caught signals (http://b/68707996).
static pid_t parent = getpid();
setpgid(0, 0);
pid_t pid = fork();
ASSERT_NE(-1, pid);
if (pid == 0) {
signal(SIGRTMIN, SIG_IGN);
for (size_t i = 0; i < 1024; ++i) {
kill(0, SIGWINCH);
kill(0, SIGRTMIN);
usleep(10);
}
return;
_exit(99);
}
// We test both with and without attributes, because they used to be
@ -417,11 +421,15 @@ TEST(spawn, signal_stress) {
posix_spawnattr_t* attrs[] = { nullptr, &attr1, &attr2 };
ScopedSignalHandler ssh(SIGWINCH, [](int) { ASSERT_EQ(getpid(), parent); });
// We use a real-time signal because that's a tricky case for LP32
// because our sigset_t was too small.
ScopedSignalHandler ssh(SIGRTMIN, [](int) { ASSERT_EQ(getpid(), parent); });
ExecTestHelper eth;
eth.SetArgs({"true", nullptr});
for (size_t i = 0; i < 128; ++i) {
posix_spawn(nullptr, "true", nullptr, attrs[i % 3], eth.GetArgs(), nullptr);
}
AssertChildExited(pid, 99);
}