Don't leak a thread when timer_create() fails.
We do the same thing in pthread_create().
This has the nice side-effect of letting us move part of the child setup that we were doing in the parent into the child where it belongs.
Bug: https://issuetracker.google.com/340125671
Change-Id: Ibe646d5ca9066f487d08fd40f004550349e8a02e
diff --git a/libc/bionic/posix_timers.cpp b/libc/bionic/posix_timers.cpp
index f522516..ccbbfcf 100644
--- a/libc/bionic/posix_timers.cpp
+++ b/libc/bionic/posix_timers.cpp
@@ -34,6 +34,8 @@
#include <string.h>
#include <time.h>
+#include "private/bionic_lock.h"
+
// System calls.
extern "C" int __rt_sigprocmask(int, const sigset64_t*, sigset64_t*, size_t);
extern "C" int __rt_sigtimedwait(const sigset64_t*, siginfo_t*, const timespec*, size_t);
@@ -60,6 +62,7 @@
int sigev_notify;
// The fields below are only needed for a SIGEV_THREAD timer.
+ Lock startup_handshake_lock;
pthread_t callback_thread;
void (*callback)(sigval_t);
sigval_t callback_argument;
@@ -73,6 +76,18 @@
static void* __timer_thread_start(void* arg) {
PosixTimer* timer = reinterpret_cast<PosixTimer*>(arg);
+ // Check that our parent managed to create the kernel timer and bail if not...
+ timer->startup_handshake_lock.lock();
+ if (timer->kernel_timer_id == -1) {
+ free(timer);
+ return nullptr;
+ }
+
+ // Give ourselves a specific meaningful name now we have a kernel timer.
+ char name[16]; // 16 is the kernel-imposed limit.
+ snprintf(name, sizeof(name), "POSIX timer %d", to_kernel_timer_id(timer));
+ pthread_setname_np(timer->callback_thread, name);
+
sigset64_t sigset = {};
sigaddset64(&sigset, TIMER_SIGNAL);
@@ -109,6 +124,7 @@
return -1;
}
+ timer->kernel_timer_id = -1;
timer->sigev_notify = (evp == nullptr) ? SIGEV_SIGNAL : evp->sigev_notify;
// If not a SIGEV_THREAD timer, the kernel can handle it without our help.
@@ -149,6 +165,10 @@
sigaddset64(&sigset, TIMER_SIGNAL);
sigset64_t old_sigset;
+ // Prevent the child thread from running until the timer has been created.
+ timer->startup_handshake_lock.init(false);
+ timer->startup_handshake_lock.lock();
+
// Use __rt_sigprocmask instead of sigprocmask64 to avoid filtering out TIMER_SIGNAL.
__rt_sigprocmask(SIG_BLOCK, &sigset, &old_sigset, sizeof(sigset));
@@ -162,21 +182,21 @@
return -1;
}
+ // Try to create the kernel timer.
sigevent se = *evp;
se.sigev_signo = TIMER_SIGNAL;
se.sigev_notify = SIGEV_THREAD_ID;
se.sigev_notify_thread_id = pthread_gettid_np(timer->callback_thread);
- if (__timer_create(clock_id, &se, &timer->kernel_timer_id) == -1) {
- __timer_thread_stop(timer);
+ rc = __timer_create(clock_id, &se, &timer->kernel_timer_id);
+
+ // Let the child run (whether we created the kernel timer or not).
+ timer->startup_handshake_lock.unlock();
+ // If __timer_create(2) failed, the child will kill itself and free the
+ // timer struct, so we just need to exit.
+ if (rc == -1) {
return -1;
}
- // Give the thread a specific meaningful name.
- // It can't do this itself because the kernel timer isn't created until after it's running.
- char name[16]; // 16 is the kernel-imposed limit.
- snprintf(name, sizeof(name), "POSIX timer %d", to_kernel_timer_id(timer));
- pthread_setname_np(timer->callback_thread, name);
-
*timer_id = timer;
return 0;
}
diff --git a/tests/time_test.cpp b/tests/time_test.cpp
index ca8e260..baafbf6 100644
--- a/tests/time_test.cpp
+++ b/tests/time_test.cpp
@@ -31,6 +31,8 @@
#include <thread>
#include "SignalUtils.h"
+#include "android-base/file.h"
+#include "android-base/strings.h"
#include "utils.h"
using namespace std::chrono_literals;
@@ -797,21 +799,41 @@
ASSERT_EQ(1, timer_create_NULL_signal_handler_invocation_count);
}
-TEST(time, timer_create_EINVAL) {
- clockid_t invalid_clock = 16;
+static int GetThreadCount() {
+ std::string status;
+ if (android::base::ReadFileToString("/proc/self/status", &status)) {
+ for (const auto& line : android::base::Split(status, "\n")) {
+ int thread_count;
+ if (sscanf(line.c_str(), "Threads: %d", &thread_count) == 1) {
+ return thread_count;
+ }
+ }
+ }
+ return -1;
+}
- // A SIGEV_SIGNAL timer is easy; the kernel does all that.
+TEST(time, timer_create_EINVAL) {
+ const clockid_t kInvalidClock = 16;
+
+ // A SIGEV_SIGNAL timer failure is easy; that's the kernel's problem.
timer_t timer_id;
- ASSERT_EQ(-1, timer_create(invalid_clock, nullptr, &timer_id));
+ ASSERT_EQ(-1, timer_create(kInvalidClock, nullptr, &timer_id));
ASSERT_ERRNO(EINVAL);
- // A SIGEV_THREAD timer is more interesting because we have stuff to clean up.
- sigevent se;
- memset(&se, 0, sizeof(se));
+ // A SIGEV_THREAD timer failure is more interesting because we have a thread
+ // to clean up (https://issuetracker.google.com/340125671).
+ sigevent se = {};
se.sigev_notify = SIGEV_THREAD;
se.sigev_notify_function = NoOpNotifyFunction;
- ASSERT_EQ(-1, timer_create(invalid_clock, &se, &timer_id));
+ ASSERT_EQ(-1, timer_create(kInvalidClock, &se, &timer_id));
ASSERT_ERRNO(EINVAL);
+
+ // timer_create() doesn't guarantee that the thread will be dead _before_
+ // it returns because that would require extra synchronization that's
+ // unnecessary in the normal (successful) case. A timeout here means we
+ // leaked a thread.
+ while (GetThreadCount() > 1) {
+ }
}
TEST(time, timer_create_multiple) {