From 66bb0b5de175f218b4988c9fc8ea49ddce0de978 Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Tue, 11 Apr 2023 16:41:34 -0700 Subject: [PATCH 1/2] Convert preopen initialization to be lazy. Insteead of eagerly initializing the preopens in a static constructor, perform preopen initialization the first time it's needed, or before a close or a renumber which might disrupt the file descriptor space. And, use a weak symbol with a stub function for use by `close` or `fd_renumber`, we that they can trigger preopen initialization only if it's actually needed. This way, if a program doesn't contain any calls to any function that needs preopens, it can avoid linking in the preopen initialization code. And if it contains calls but doesn't execute them at runtime, it can avoid executing the preopen intiailization code. A downside here is that this may cause problems for users that call `__wasi_fd_close` or `__wasi_fd_renumber` directly and close over overwrite some preopens before libc has a chance to scan them. To partially address this, this PR does add a declaration for `__wasilibc_populate_preopens` to so that users can call it manually if they need to. --- .../wasm32-wasi-threads/defined-symbols.txt | 1 + expected/wasm32-wasi/defined-symbols.txt | 1 + .../cloudlibc/src/libc/unistd/close.c | 16 ---------- libc-bottom-half/headers/public/wasi/libc.h | 6 ++++ .../sources/__wasilibc_fd_renumber.c | 22 +++++++++++++ libc-bottom-half/sources/preopens.c | 31 ++++++++++++++----- 6 files changed, 54 insertions(+), 23 deletions(-) delete mode 100644 libc-bottom-half/cloudlibc/src/libc/unistd/close.c diff --git a/expected/wasm32-wasi-threads/defined-symbols.txt b/expected/wasm32-wasi-threads/defined-symbols.txt index 75954e6ec..c913f1620 100644 --- a/expected/wasm32-wasi-threads/defined-symbols.txt +++ b/expected/wasm32-wasi-threads/defined-symbols.txt @@ -390,6 +390,7 @@ __wasilibc_nocwd_scandirat __wasilibc_nocwd_symlinkat __wasilibc_nocwd_utimensat __wasilibc_open_nomode +__wasilibc_populate_preopens __wasilibc_pthread_self __wasilibc_register_preopened_fd __wasilibc_rename_newat diff --git a/expected/wasm32-wasi/defined-symbols.txt b/expected/wasm32-wasi/defined-symbols.txt index a69263216..6e743228b 100644 --- a/expected/wasm32-wasi/defined-symbols.txt +++ b/expected/wasm32-wasi/defined-symbols.txt @@ -329,6 +329,7 @@ __wasilibc_nocwd_scandirat __wasilibc_nocwd_symlinkat __wasilibc_nocwd_utimensat __wasilibc_open_nomode +__wasilibc_populate_preopens __wasilibc_register_preopened_fd __wasilibc_rename_newat __wasilibc_rename_oldat diff --git a/libc-bottom-half/cloudlibc/src/libc/unistd/close.c b/libc-bottom-half/cloudlibc/src/libc/unistd/close.c deleted file mode 100644 index 2f5814bb6..000000000 --- a/libc-bottom-half/cloudlibc/src/libc/unistd/close.c +++ /dev/null @@ -1,16 +0,0 @@ -// Copyright (c) 2015-2016 Nuxi, https://nuxi.nl/ -// -// SPDX-License-Identifier: BSD-2-Clause - -#include -#include -#include - -int close(int fildes) { - __wasi_errno_t error = __wasi_fd_close(fildes); - if (error != 0) { - errno = error; - return -1; - } - return 0; -} diff --git a/libc-bottom-half/headers/public/wasi/libc.h b/libc-bottom-half/headers/public/wasi/libc.h index b50518bfc..18d8e9e3c 100644 --- a/libc-bottom-half/headers/public/wasi/libc.h +++ b/libc-bottom-half/headers/public/wasi/libc.h @@ -11,6 +11,12 @@ extern "C" { struct stat; struct timespec; +/// Populate libc's preopen tables. This is normally done automatically +/// just before it's needed, however if you call `__wasi_fd_renumber` or +/// `__wasi_fd_close` directly, and you need the preopens to be accurate +/// afterward, you should call this before doing so. +void __wasilibc_populate_preopens(void); + /// Register the given pre-opened file descriptor under the given path. /// /// This function does not take ownership of `prefix` (it makes its own copy). diff --git a/libc-bottom-half/sources/__wasilibc_fd_renumber.c b/libc-bottom-half/sources/__wasilibc_fd_renumber.c index aa9d8dcb1..47992e91f 100644 --- a/libc-bottom-half/sources/__wasilibc_fd_renumber.c +++ b/libc-bottom-half/sources/__wasilibc_fd_renumber.c @@ -4,6 +4,9 @@ #include int __wasilibc_fd_renumber(int fd, int newfd) { + // Scan the preopen fds before making any changes. + __wasilibc_populate_preopens(); + __wasi_errno_t error = __wasi_fd_renumber(fd, newfd); if (error != 0) { errno = error; @@ -11,3 +14,22 @@ int __wasilibc_fd_renumber(int fd, int newfd) { } return 0; } + +int close(int fd) { + // Scan the preopen fds before making any changes. + __wasilibc_populate_preopens(); + + __wasi_errno_t error = __wasi_fd_close(fd); + if (error != 0) { + errno = error; + return -1; + } + + return 0; +} + +weak void __wasilibc_populate_preopens(void) { + // This version does nothing. It may be overridden by a version which does + // something if `__wasilibc_find_abspath` or `__wasilibc_find_relpath` are + // used. +} diff --git a/libc-bottom-half/sources/preopens.c b/libc-bottom-half/sources/preopens.c index 7293c8c49..d44857551 100644 --- a/libc-bottom-half/sources/preopens.c +++ b/libc-bottom-half/sources/preopens.c @@ -25,6 +25,7 @@ typedef struct preopen { } preopen; /// A simple growable array of `preopen`. +static _Atomic _Bool preopens_populated = false; static preopen *preopens; static size_t num_preopens; static size_t preopen_capacity; @@ -152,6 +153,8 @@ static bool prefix_matches(const char *prefix, size_t prefix_len, const char *pa // See the documentation in libc.h int __wasilibc_register_preopened_fd(int fd, const char *prefix) { + __wasilibc_populate_preopens(); + return internal_register_preopened_fd((__wasi_fd_t)fd, prefix); } @@ -172,6 +175,8 @@ int __wasilibc_find_relpath(const char *path, int __wasilibc_find_abspath(const char *path, const char **abs_prefix, const char **relative_path) { + __wasilibc_populate_preopens(); + // Strip leading `/` characters, the prefixes we're mataching won't have // them. while (*path == '/') @@ -219,13 +224,20 @@ int __wasilibc_find_abspath(const char *path, return fd; } -/// This is referenced by weak reference from crt1.c and lives in the same -/// source file as `__wasilibc_find_relpath` so that it's linked in when it's -/// needed. -// Concerning the 51 -- see the comment by the constructor priority in -// libc-bottom-half/sources/environ.c. -__attribute__((constructor(51))) -static void __wasilibc_populate_preopens(void) { +void __wasilibc_populate_preopens(void) { + // Fast path: If the preopens are already initialized, do nothing. + if (preopens_populated) { + return; + } + + LOCK(lock); + + // Check whether another thread initialized the preopens already. + if (preopens_populated) { + UNLOCK(lock); + return; + } + // Skip stdin, stdout, and stderr, and count up until we reach an invalid // file descriptor. for (__wasi_fd_t fd = 3; fd != 0; ++fd) { @@ -260,6 +272,11 @@ static void __wasilibc_populate_preopens(void) { } } + // Preopens are now initialized. + preopens_populated = true; + + UNLOCK(lock); + return; oserr: _Exit(EX_OSERR); From 56ef00ceb3826997a55477a15a1335b70d5258b0 Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Wed, 19 Apr 2023 09:05:40 -0700 Subject: [PATCH 2/2] Fix calling `internal_register_preopened_fd` with the lock held. Factor out the lock acquisition from the implementation of `internal_register_preopened_fd` so that we can call it from `__wasilibc_populate_preopens` with the lock held. --- libc-bottom-half/sources/preopens.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/libc-bottom-half/sources/preopens.c b/libc-bottom-half/sources/preopens.c index d44857551..b495433fa 100644 --- a/libc-bottom-half/sources/preopens.c +++ b/libc-bottom-half/sources/preopens.c @@ -101,12 +101,9 @@ static const char *strip_prefixes(const char *path) { return path; } -/// Register the given preopened file descriptor under the given path. -/// -/// This function takes ownership of `prefix`. -static int internal_register_preopened_fd(__wasi_fd_t fd, const char *relprefix) { - LOCK(lock); - +/// Similar to `internal_register_preopened_fd_unlocked` but does not +/// take a lock. +static int internal_register_preopened_fd_unlocked(__wasi_fd_t fd, const char *relprefix) { // Check preconditions. assert_invariants(); assert(fd != AT_FDCWD); @@ -114,22 +111,32 @@ static int internal_register_preopened_fd(__wasi_fd_t fd, const char *relprefix) assert(relprefix != NULL); if (num_preopens == preopen_capacity && resize() != 0) { - UNLOCK(lock); return -1; } char *prefix = strdup(strip_prefixes(relprefix)); if (prefix == NULL) { - UNLOCK(lock); return -1; } preopens[num_preopens++] = (preopen) { prefix, fd, }; assert_invariants(); - UNLOCK(lock); return 0; } +/// Register the given preopened file descriptor under the given path. +/// +/// This function takes ownership of `prefix`. +static int internal_register_preopened_fd(__wasi_fd_t fd, const char *relprefix) { + LOCK(lock); + + int r = internal_register_preopened_fd_unlocked(fd, relprefix); + + UNLOCK(lock); + + return r; +} + /// Are the `prefix_len` bytes pointed to by `prefix` a prefix of `path`? static bool prefix_matches(const char *prefix, size_t prefix_len, const char *path) { // Allow an empty string as a prefix of any relative path. @@ -261,7 +268,7 @@ void __wasilibc_populate_preopens(void) { goto oserr; prefix[prestat.u.dir.pr_name_len] = '\0'; - if (internal_register_preopened_fd(fd, prefix) != 0) + if (internal_register_preopened_fd_unlocked(fd, prefix) != 0) goto software; free(prefix);