From 462ca8b3148c8034cfee3a526033bb14c068102b Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Tue, 4 Apr 2023 13:33:28 -0700 Subject: [PATCH] : add posix_spawn_file_actions_addchdir_np()/posix_spawn_file_actions_addfchdir_np(). The recent header nullability additions and the corresponding source cleanup made me notice that we're missing a couple of actions that most of the other implementations have. They've also been added to the _next_ revision of POSIX, unchanged except for the removal of the `_np` suffix. They're trivial to implement, the testing is quite simple too, and if they're going to be in POSIX soon, having them accessible in older versions of Android via __RENAME() seems useful. (No-one else has shipped the POSIX names yet.) Bug: http://b/152414297 Test: treehugger Change-Id: I0d2a1e47fbd2e826cff9c45038928aa1b6fcce59 --- docs/status.md | 3 +++ libc/bionic/spawn.cpp | 19 +++++++++++++++++-- libc/include/spawn.h | 3 +++ libc/libc.map.txt | 2 ++ tests/spawn_test.cpp | 23 +++++++++++++++++++++-- 5 files changed, 46 insertions(+), 4 deletions(-) diff --git a/docs/status.md b/docs/status.md index 5d2603abe..3d104e44d 100644 --- a/docs/status.md +++ b/docs/status.md @@ -59,6 +59,9 @@ New libc functions in U (API level 34): * `close_range` and `copy_file_range` (Linux-specific GNU extensions). * `memset_explicit` in (C23 addition). * `__freadahead` in (in musl but not glibc). + * `posix_spawn_file_actions_addchdir_np` and + `posix_spawn_file_actions_addfchdir_np` in (in musl/glibc + and macOS, but not iOS). New libc behavior in U (API level 34): * Support for `%b` and `%B` in the printf/wprintf family, `%b` in the diff --git a/libc/bionic/spawn.cpp b/libc/bionic/spawn.cpp index 5cf95d84f..5d76f77ca 100644 --- a/libc/bionic/spawn.cpp +++ b/libc/bionic/spawn.cpp @@ -68,7 +68,9 @@ static int cloexec_except_stdioe() { enum Action { kOpen, kClose, - kDup2 + kDup2, + kChdir, + kFchdir, }; struct __posix_spawn_file_action { @@ -93,6 +95,10 @@ struct __posix_spawn_file_action { } else if (what == kClose) { // Failure to close is ignored. close(fd); + } else if (what == kChdir) { + if (chdir(path) == -1) _exit(127); + } else if (what == kFchdir) { + if (fchdir(fd) == -1) _exit(127); } else { // It's a dup2. if (fd == new_fd) { @@ -340,7 +346,7 @@ static int posix_spawn_add_file_action(posix_spawn_file_actions_t* actions, if (action == nullptr) return errno; action->next = nullptr; - if (what == kOpen) { + if (what == kOpen || what == kChdir) { action->path = strdup(path); if (action->path == nullptr) { free(action); @@ -380,3 +386,12 @@ int posix_spawn_file_actions_adddup2(posix_spawn_file_actions_t* actions, int fd if (fd < 0 || new_fd < 0) return EBADF; return posix_spawn_add_file_action(actions, kDup2, fd, new_fd, nullptr, 0, 0); } + +int posix_spawn_file_actions_addchdir_np(posix_spawn_file_actions_t* actions, const char* path) { + return posix_spawn_add_file_action(actions, kChdir, -1, -1, path, 0, 0); +} + +int posix_spawn_file_actions_addfchdir_np(posix_spawn_file_actions_t* actions, int fd) { + if (fd < 0) return EBADF; + return posix_spawn_add_file_action(actions, kFchdir, fd, -1, nullptr, 0, 0); +} diff --git a/libc/include/spawn.h b/libc/include/spawn.h index 8bb34d020..6c34b98bc 100644 --- a/libc/include/spawn.h +++ b/libc/include/spawn.h @@ -87,6 +87,9 @@ int posix_spawn_file_actions_addopen(posix_spawn_file_actions_t _Nonnull * _Nonn int posix_spawn_file_actions_addclose(posix_spawn_file_actions_t _Nonnull * _Nonnull __actions, int __fd) __INTRODUCED_IN(28); int posix_spawn_file_actions_adddup2(posix_spawn_file_actions_t _Nonnull * _Nonnull __actions, int __fd, int __new_fd) __INTRODUCED_IN(28); +int posix_spawn_file_actions_addchdir_np(posix_spawn_file_actions_t _Nonnull * _Nonnull __actions, const char* _Nonnull __path) __INTRODUCED_IN(34); +int posix_spawn_file_actions_addfchdir_np(posix_spawn_file_actions_t _Nonnull * _Nonnull __actions, int __fd) __INTRODUCED_IN(34); + __END_DECLS #endif diff --git a/libc/libc.map.txt b/libc/libc.map.txt index 4a4e60777..c75b13a18 100644 --- a/libc/libc.map.txt +++ b/libc/libc.map.txt @@ -1580,6 +1580,8 @@ LIBC_U { # introduced=UpsideDownCake close_range; copy_file_range; memset_explicit; + posix_spawn_file_actions_addchdir_np; + posix_spawn_file_actions_addfchdir_np; } LIBC_T; LIBC_PRIVATE { diff --git a/tests/spawn_test.cpp b/tests/spawn_test.cpp index a9563b8aa..ab3e877a3 100644 --- a/tests/spawn_test.cpp +++ b/tests/spawn_test.cpp @@ -232,18 +232,28 @@ TEST(spawn, posix_spawn_environment) { } TEST(spawn, posix_spawn_file_actions) { +#if !defined(__GLIBC__) int fds[2]; ASSERT_NE(-1, pipe(fds)); posix_spawn_file_actions_t fa; ASSERT_EQ(0, posix_spawn_file_actions_init(&fa)); + // Test addclose and adddup2 by redirecting output to the pipe created above. ASSERT_EQ(0, posix_spawn_file_actions_addclose(&fa, fds[0])); ASSERT_EQ(0, posix_spawn_file_actions_adddup2(&fa, fds[1], 1)); ASSERT_EQ(0, posix_spawn_file_actions_addclose(&fa, fds[1])); // Check that close(2) failures are ignored by closing the same fd again. ASSERT_EQ(0, posix_spawn_file_actions_addclose(&fa, fds[1])); + // Open a file directly, to test addopen. ASSERT_EQ(0, posix_spawn_file_actions_addopen(&fa, 56, "/proc/version", O_RDONLY, 0)); + // Test addfchdir by opening the same file a second way... + ASSERT_EQ(0, posix_spawn_file_actions_addopen(&fa, 57, "/proc", O_PATH, 0)); + ASSERT_EQ(0, posix_spawn_file_actions_addfchdir_np(&fa, 57)); + ASSERT_EQ(0, posix_spawn_file_actions_addopen(&fa, 58, "version", O_RDONLY, 0)); + // Test addchdir by opening the same file a third way... + ASSERT_EQ(0, posix_spawn_file_actions_addchdir_np(&fa, "/")); + ASSERT_EQ(0, posix_spawn_file_actions_addopen(&fa, 59, "proc/version", O_RDONLY, 0)); ExecTestHelper eth; eth.SetArgs({"ls", "-l", "/proc/self/fd", nullptr}); @@ -259,12 +269,21 @@ TEST(spawn, posix_spawn_file_actions) { AssertChildExited(pid, 0); // We'll know the dup2 worked if we see any ls(1) output in our pipe. - // The open we can check manually... + // The opens we can check manually (and they implicitly check the chdirs)... bool open_to_fd_56_worked = false; + bool open_to_fd_58_worked = false; + bool open_to_fd_59_worked = false; for (const auto& line : android::base::Split(content, "\n")) { if (line.find(" 56 -> /proc/version") != std::string::npos) open_to_fd_56_worked = true; + if (line.find(" 58 -> /proc/version") != std::string::npos) open_to_fd_58_worked = true; + if (line.find(" 59 -> /proc/version") != std::string::npos) open_to_fd_59_worked = true; } - ASSERT_TRUE(open_to_fd_56_worked); + ASSERT_TRUE(open_to_fd_56_worked) << content; + ASSERT_TRUE(open_to_fd_58_worked) << content; + ASSERT_TRUE(open_to_fd_59_worked) << content; +#else + GTEST_SKIP() << "our old glibc doesn't have the chdirs; newer versions and musl do."; +#endif } static void CatFileToString(posix_spawnattr_t* sa, const char* path, std::string* content) {