From f52f65b2e351085250b743a5319dab418f2d48be Mon Sep 17 00:00:00 2001 From: Justine Tunney Date: Thu, 13 Oct 2022 15:38:31 -0700 Subject: [PATCH] Make system() and popen() thread safe --- libc/calls/__sig2.c | 6 +-- libc/calls/siglock.c | 2 - .../gettid_test.c => libc/calls/siglock_obj.c | 16 ++----- libc/calls/state.internal.h | 1 + libc/intrin/fds_lock_obj.c | 22 ++++++++++ libc/{stdio => intrin}/flushers.c | 1 + libc/intrin/g_fds.c | 1 - libc/runtime/fork.c | 11 ++++- libc/stdio/fflush.internal.h | 1 + libc/stdio/fflush_unlocked.c | 4 +- libc/stdio/popen.c | 1 + libc/stdio/posix_spawn.c | 5 ++- libc/stdio/system.c | 1 + libc/thread/posixthread.internal.h | 2 + libc/thread/pthread_create.c | 34 +++++++++++++- net/turfwar/turfwar.c | 4 +- test/libc/stdio/popen_test.c | 44 +++++++++++++++++++ third_party/dlmalloc/dlmalloc.c | 7 +++ third_party/dlmalloc/dlmalloc.h | 3 +- 19 files changed, 135 insertions(+), 31 deletions(-) rename test/libc/intrin/gettid_test.c => libc/calls/siglock_obj.c (86%) create mode 100644 libc/intrin/fds_lock_obj.c rename libc/{stdio => intrin}/flushers.c (98%) diff --git a/libc/calls/__sig2.c b/libc/calls/__sig2.c index 0a0a44c2c..4f3e0d039 100644 --- a/libc/calls/__sig2.c +++ b/libc/calls/__sig2.c @@ -66,7 +66,7 @@ static inline textwindows int __sig_is_masked(int sig) { } textwindows int __sig_is_applicable(struct Signal *s) { - return (s->tid <= 0 || s->tid == gettid()) && !__sig_is_masked(s->sig); + return s->tid <= 0 || s->tid == gettid(); } /** @@ -74,13 +74,11 @@ textwindows int __sig_is_applicable(struct Signal *s) { * @return signal or null if empty or none unmasked */ static textwindows struct Signal *__sig_remove(void) { - int tid; struct Signal *prev, *res; if (__sig.queue) { - tid = gettid(); __sig_lock(); for (prev = 0, res = __sig.queue; res; prev = res, res = res->next) { - if (__sig_is_applicable(res)) { + if (__sig_is_applicable(res) && !__sig_is_masked(res->sig)) { if (res == __sig.queue) { __sig.queue = res->next; } else if (prev) { diff --git a/libc/calls/siglock.c b/libc/calls/siglock.c index 9d3632dea..5fd32e6ad 100644 --- a/libc/calls/siglock.c +++ b/libc/calls/siglock.c @@ -19,8 +19,6 @@ #include "libc/calls/state.internal.h" #include "libc/thread/thread.h" -static pthread_mutex_t __sig_lock_obj; - void(__sig_lock)(void) { pthread_mutex_lock(&__sig_lock_obj); } diff --git a/test/libc/intrin/gettid_test.c b/libc/calls/siglock_obj.c similarity index 86% rename from test/libc/intrin/gettid_test.c rename to libc/calls/siglock_obj.c index b389f709f..8e70e819d 100644 --- a/test/libc/intrin/gettid_test.c +++ b/libc/calls/siglock_obj.c @@ -16,17 +16,7 @@ │ TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR │ │ PERFORMANCE OF THIS SOFTWARE. │ ╚─────────────────────────────────────────────────────────────────────────────*/ -#include "libc/calls/struct/sigaction.h" -#include "libc/intrin/intrin.h" -#include "libc/intrin/kprintf.h" -#include "libc/sysv/consts/sig.h" -#include "libc/testlib/testlib.h" +#include "libc/calls/state.internal.h" +#include "libc/thread/thread.h" -void OnSig(int sig) { - kprintf("got sig\n"); -} - -TEST(gettid, test) { - signal(SIGTRAP, OnSig); - DebugBreak(); -} +pthread_mutex_t __sig_lock_obj; diff --git a/libc/calls/state.internal.h b/libc/calls/state.internal.h index 4f9a4dc3b..33b2990ad 100644 --- a/libc/calls/state.internal.h +++ b/libc/calls/state.internal.h @@ -11,6 +11,7 @@ hidden extern bool __time_critical; hidden extern unsigned __sighandrvas[NSIG]; hidden extern unsigned __sighandflags[NSIG]; hidden extern pthread_mutex_t __fds_lock_obj; +hidden extern pthread_mutex_t __sig_lock_obj; hidden extern const struct NtSecurityAttributes kNtIsInheritable; void __fds_lock(void); diff --git a/libc/intrin/fds_lock_obj.c b/libc/intrin/fds_lock_obj.c new file mode 100644 index 000000000..79da205a5 --- /dev/null +++ b/libc/intrin/fds_lock_obj.c @@ -0,0 +1,22 @@ +/*-*- mode:c;indent-tabs-mode:nil;c-basic-offset:2;tab-width:8;coding:utf-8 -*-│ +│vi: set net ft=c ts=2 sts=2 sw=2 fenc=utf-8 :vi│ +╞══════════════════════════════════════════════════════════════════════════════╡ +│ Copyright 2022 Justine Alexandra Roberts Tunney │ +│ │ +│ Permission to use, copy, modify, and/or distribute this software for │ +│ any purpose with or without fee is hereby granted, provided that the │ +│ above copyright notice and this permission notice appear in all copies. │ +│ │ +│ THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL │ +│ WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED │ +│ WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE │ +│ AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL │ +│ DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR │ +│ PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER │ +│ TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR │ +│ PERFORMANCE OF THIS SOFTWARE. │ +╚─────────────────────────────────────────────────────────────────────────────*/ +#include "libc/calls/state.internal.h" +#include "libc/thread/thread.h" + +pthread_mutex_t __fds_lock_obj; diff --git a/libc/stdio/flushers.c b/libc/intrin/flushers.c similarity index 98% rename from libc/stdio/flushers.c rename to libc/intrin/flushers.c index 3b7c976bb..88221dd19 100644 --- a/libc/stdio/flushers.c +++ b/libc/intrin/flushers.c @@ -18,4 +18,5 @@ ╚─────────────────────────────────────────────────────────────────────────────*/ #include "libc/stdio/fflush.internal.h" +pthread_mutex_t __fflush_lock_obj; struct StdioFlush __fflush; diff --git a/libc/intrin/g_fds.c b/libc/intrin/g_fds.c index 93378c5ae..34c5aee5c 100644 --- a/libc/intrin/g_fds.c +++ b/libc/intrin/g_fds.c @@ -30,7 +30,6 @@ STATIC_YOINK("_init_g_fds"); struct Fds g_fds; -pthread_mutex_t __fds_lock_obj; static textwindows dontinline void SetupWinStd(struct Fds *fds, int i, int x) { int64_t h; diff --git a/libc/runtime/fork.c b/libc/runtime/fork.c index 23993d8d1..676bd7de4 100644 --- a/libc/runtime/fork.c +++ b/libc/runtime/fork.c @@ -21,19 +21,23 @@ #include "libc/calls/struct/sigset.internal.h" #include "libc/calls/syscall-nt.internal.h" #include "libc/calls/syscall-sysv.internal.h" -#include "libc/calls/syscall_support-sysv.internal.h" #include "libc/dce.h" #include "libc/intrin/atomic.h" #include "libc/intrin/strace.internal.h" +#include "libc/intrin/weaken.h" #include "libc/nt/process.h" #include "libc/runtime/internal.h" #include "libc/sysv/consts/sig.h" +#include "libc/thread/posixthread.internal.h" #include "libc/thread/tls.h" int _fork(uint32_t dwCreationFlags) { axdx_t ad; sigset_t old, all; - int ax, dx, parent; + int ax, dx, parent, parent_tid = 0; + if (_weaken(_pthread_atfork)) { + parent_tid = gettid(); + } if (!IsWindows()) { sigfillset(&all); sys_sigprocmask(SIG_BLOCK, &all, &old); @@ -61,6 +65,9 @@ int _fork(uint32_t dwCreationFlags) { IsLinux() ? dx : sys_gettid(), memory_order_relaxed); } + if (_weaken(_pthread_atfork)) { + _weaken(_pthread_atfork)(parent_tid); + } if (!IsWindows()) sys_sigprocmask(SIG_SETMASK, &old, 0); STRACE("fork() → 0 (child of %d)", parent); } else { diff --git a/libc/stdio/fflush.internal.h b/libc/stdio/fflush.internal.h index 089d80db4..0c05a8a8f 100644 --- a/libc/stdio/fflush.internal.h +++ b/libc/stdio/fflush.internal.h @@ -18,6 +18,7 @@ struct StdioFlush { }; hidden extern struct StdioFlush __fflush; +hidden extern pthread_mutex_t __fflush_lock_obj; void __fflush_lock(void); void __fflush_unlock(void); diff --git a/libc/stdio/fflush_unlocked.c b/libc/stdio/fflush_unlocked.c index a00ebe969..313234f39 100644 --- a/libc/stdio/fflush_unlocked.c +++ b/libc/stdio/fflush_unlocked.c @@ -19,7 +19,6 @@ #include "libc/calls/calls.h" #include "libc/errno.h" #include "libc/intrin/bits.h" -#include "libc/thread/thread.h" #include "libc/intrin/pushpop.h" #include "libc/macros.internal.h" #include "libc/mem/arraylist.internal.h" @@ -29,8 +28,7 @@ #include "libc/stdio/internal.h" #include "libc/stdio/stdio.h" #include "libc/sysv/consts/o.h" - -static pthread_mutex_t __fflush_lock_obj; +#include "libc/thread/thread.h" void(__fflush_lock)(void) { pthread_mutex_lock(&__fflush_lock_obj); diff --git a/libc/stdio/popen.c b/libc/stdio/popen.c index df3716ad0..ffd5c71c2 100644 --- a/libc/stdio/popen.c +++ b/libc/stdio/popen.c @@ -35,6 +35,7 @@ * Bourne-like syntax on all platforms including Windows. * * @see pclose() + * @threadsafe */ FILE *popen(const char *cmdline, const char *mode) { FILE *f; diff --git a/libc/stdio/posix_spawn.c b/libc/stdio/posix_spawn.c index 705e7558b..cc0e3f475 100644 --- a/libc/stdio/posix_spawn.c +++ b/libc/stdio/posix_spawn.c @@ -20,9 +20,12 @@ #include "libc/calls/struct/sigaction.h" #include "libc/calls/struct/sigset.h" #include "libc/errno.h" +#include "libc/intrin/weaken.h" #include "libc/runtime/runtime.h" #include "libc/stdio/posix_spawn.h" #include "libc/stdio/posix_spawn.internal.h" +#include "libc/thread/thread.h" +#include "libc/thread/tls.h" static int RunFileActions(struct _posix_faction *a) { int t; @@ -68,7 +71,7 @@ int posix_spawn(int *pid, const char *path, int s, child; sigset_t allsigs; struct sigaction dfl; - if (!(child = vfork())) { + if (!(child = _weaken(pthread_create) ? fork() : vfork())) { if (attrp && *attrp) { if ((*attrp)->flags & POSIX_SPAWN_SETPGROUP) { if (setpgid(0, (*attrp)->pgroup)) _Exit(127); diff --git a/libc/stdio/system.c b/libc/stdio/system.c index 16fc05b98..571401bd8 100644 --- a/libc/stdio/system.c +++ b/libc/stdio/system.c @@ -37,6 +37,7 @@ * @param cmdline is an interpreted Turing-complete command * @return -1 if child process couldn't be created, otherwise a wait * status that can be accessed using macros like WEXITSTATUS(s) + * @threadsafe */ int system(const char *cmdline) { int pid, wstatus; diff --git a/libc/thread/posixthread.internal.h b/libc/thread/posixthread.internal.h index 05c16cddb..de3e24ae4 100644 --- a/libc/thread/posixthread.internal.h +++ b/libc/thread/posixthread.internal.h @@ -82,6 +82,8 @@ hidden extern uint64_t _pthread_key_usage[(PTHREAD_KEYS_MAX + 63) / 64]; hidden extern pthread_key_dtor _pthread_key_dtor[PTHREAD_KEYS_MAX]; hidden extern _Thread_local void *_pthread_keys[PTHREAD_KEYS_MAX]; +void _pthread_atfork(int) hidden; +void _pthread_funlock(pthread_mutex_t *, int) hidden; int _pthread_reschedule(struct PosixThread *) hidden; int _pthread_setschedparam_freebsd(int, int, const struct sched_param *) hidden; void _pthread_free(struct PosixThread *) hidden; diff --git a/libc/thread/pthread_create.c b/libc/thread/pthread_create.c index 920c1a1da..b8fdb59e5 100644 --- a/libc/thread/pthread_create.c +++ b/libc/thread/pthread_create.c @@ -19,6 +19,7 @@ #include "libc/assert.h" #include "libc/calls/calls.h" #include "libc/calls/sched-sysv.internal.h" +#include "libc/calls/state.internal.h" #include "libc/calls/struct/sigaltstack.h" #include "libc/calls/syscall-sysv.internal.h" #include "libc/dce.h" @@ -35,6 +36,8 @@ #include "libc/runtime/clone.internal.h" #include "libc/runtime/runtime.h" #include "libc/runtime/stack.h" +#include "libc/stdio/fflush.internal.h" +#include "libc/stdio/stdio.h" #include "libc/sysv/consts/clone.h" #include "libc/sysv/consts/map.h" #include "libc/sysv/consts/prot.h" @@ -45,6 +48,7 @@ #include "libc/thread/thread.h" #include "libc/thread/tls.h" #include "libc/thread/wait0.internal.h" +#include "third_party/dlmalloc/dlmalloc.h" STATIC_YOINK("nsync_mu_lock"); STATIC_YOINK("nsync_mu_unlock"); @@ -72,6 +76,30 @@ void _pthread_free(struct PosixThread *pt) { free(pt); } +void _pthread_funlock(pthread_mutex_t *mu, int parent_tid) { + if (mu->_type == PTHREAD_MUTEX_NORMAL || + (atomic_load_explicit(&mu->_lock, memory_order_relaxed) && + mu->_owner != parent_tid)) { + atomic_store_explicit(&mu->_lock, 0, memory_order_relaxed); + mu->_nsync = 0; + mu->_depth = 0; + mu->_owner = 0; + } +} + +void _pthread_atfork(int parent_tid) { + FILE *f; + if (_weaken(dlmalloc_atfork)) _weaken(dlmalloc_atfork)(); + _pthread_funlock(&__fds_lock_obj, parent_tid); + _pthread_funlock(&__sig_lock_obj, parent_tid); + _pthread_funlock(&__fflush_lock_obj, parent_tid); + for (int i = 0; i < __fflush.handles.i; ++i) { + if ((f = __fflush.handles.p[i])) { + _pthread_funlock((pthread_mutex_t *)f->lock, parent_tid); + } + } +} + static int PosixThread(void *arg, int tid) { struct PosixThread *pt = arg; enum PosixThreadStatus status; @@ -281,10 +309,12 @@ errno_t pthread_create(pthread_t *thread, const pthread_attr_t *attr, // set initial status switch (pt->attr.__detachstate) { case PTHREAD_CREATE_JOINABLE: - pt->status = kPosixThreadJoinable; + atomic_store_explicit(&pt->status, kPosixThreadJoinable, + memory_order_relaxed); break; case PTHREAD_CREATE_DETACHED: - pt->status = kPosixThreadDetached; + atomic_store_explicit(&pt->status, kPosixThreadDetached, + memory_order_relaxed); _pthread_zombies_add(pt); break; default: diff --git a/net/turfwar/turfwar.c b/net/turfwar/turfwar.c index 209ee824c..70f87c5fa 100644 --- a/net/turfwar/turfwar.c +++ b/net/turfwar/turfwar.c @@ -1770,11 +1770,11 @@ OnError: } int main(int argc, char *argv[]) { - ShowCrashReports(); + // ShowCrashReports(); if (IsLinux()) { Write(2, "Enabling TCP_FASTOPEN for server sockets...\n"); - system("sudo sh -c 'echo 2 >/proc/sys/net/ipv4/tcp_fastopen'"); + system("sudo sh -c 'echo 3 >/proc/sys/net/ipv4/tcp_fastopen'"); } // we don't have proper futexes on these platforms diff --git a/test/libc/stdio/popen_test.c b/test/libc/stdio/popen_test.c index c4dabde5a..0fd173b62 100644 --- a/test/libc/stdio/popen_test.c +++ b/test/libc/stdio/popen_test.c @@ -20,10 +20,18 @@ #include "libc/calls/struct/sigaction.h" #include "libc/dce.h" #include "libc/fmt/fmt.h" +#include "libc/fmt/itoa.h" +#include "libc/limits.h" +#include "libc/mem/gc.h" +#include "libc/mem/mem.h" #include "libc/runtime/runtime.h" +#include "libc/stdio/lock.internal.h" +#include "libc/stdio/rand.h" #include "libc/stdio/stdio.h" +#include "libc/str/str.h" #include "libc/sysv/consts/sig.h" #include "libc/testlib/testlib.h" +#include "libc/thread/thread.h" FILE *f; char buf[32]; @@ -93,3 +101,39 @@ TEST(popen, complicated) { ASSERT_EQ(1, gotsig); signal(SIGUSR1, SIG_DFL); } + +void *Worker(void *arg) { + FILE *f; + char buf[32]; + char *arg1, *arg2, *cmd; + for (int i = 0; i < 8; ++i) { + cmd = malloc(64); + arg1 = malloc(13); + arg2 = malloc(13); + FormatInt32(arg1, _rand64()); + FormatInt32(arg2, _rand64()); + sprintf(cmd, "echo %s; ./echo.com %s", arg1, arg2); + strcat(arg1, "\n"); + strcat(arg2, "\n"); + ASSERT_NE(NULL, (f = popen(cmd, "r"))); + ASSERT_STREQ(arg1, fgets(buf, sizeof(buf), f)); + ASSERT_STREQ(arg2, fgets(buf, sizeof(buf), f)); + ASSERT_EQ(0, pclose(f)); + free(arg2); + free(arg1); + free(cmd); + } + return 0; +} + +TEST(popen, torture) { + int i, n = 8; + pthread_t *t = _gc(malloc(sizeof(pthread_t) * n)); + testlib_extract("/zip/echo.com", "echo.com", 0755); + for (i = 0; i < n; ++i) { + ASSERT_EQ(0, pthread_create(t + i, 0, Worker, 0)); + } + for (i = 0; i < n; ++i) { + ASSERT_EQ(0, pthread_join(t[i], 0)); + } +} diff --git a/third_party/dlmalloc/dlmalloc.c b/third_party/dlmalloc/dlmalloc.c index 061301f1c..1df8c7128 100644 --- a/third_party/dlmalloc/dlmalloc.c +++ b/third_party/dlmalloc/dlmalloc.c @@ -1315,6 +1315,13 @@ int dlposix_memalign(void** pp, size_t alignment, size_t bytes) { } } +#if USE_LOCKS +void dlmalloc_atfork(void) { + bzero(&gm->mutex, sizeof(gm->mutex)); + bzero(&malloc_global_mutex, sizeof(malloc_global_mutex)); +} +#endif + void* dlvalloc(size_t bytes) { size_t pagesz; ensure_initialization(); diff --git a/third_party/dlmalloc/dlmalloc.h b/third_party/dlmalloc/dlmalloc.h index 6de3f7623..2e3309fe7 100644 --- a/third_party/dlmalloc/dlmalloc.h +++ b/third_party/dlmalloc/dlmalloc.h @@ -505,7 +505,8 @@ void mspace_inspect_all(mspace msp, void (*handler)(void*, void*, size_t, void*), void* arg); -void dlmalloc_abort(void); +void dlmalloc_atfork(void) hidden; +void dlmalloc_abort(void) hidden; COSMOPOLITAN_C_END_ #endif /* !(__ASSEMBLER__ + __LINKER__ + 0) */