tequilaOS/platform_bionic
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

posix_spawn: always clear O_CLOEXEC for dup'ed fds.

Browse source 
dup2(2) is a no-op if the new and old fds are equal, but it's pretty
clear that any useful caller in the posix_spawn(3) context wants us to
clear O_CLOEXEC even if we don't actually "move" the fd.

Bug: https://www.austingroupbugs.net/view.php?id=411
Test: treehugger
Change-Id: I5ce1a1f9216df5afd295cc9e35b84527873e9541
This commit is contained in:
Elliott Hughes 2022-02-16 14:39:07 -08:00
parent ab74e3260a
commit 62d49fd8e1
2 changed files with 50 additions and 1 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

12
libc/bionic/spawn.cpp
View file

@ -69,7 +69,17 @@ struct __posix_spawn_file_action {
// Failure to close is ignored.
close(fd);
} else {
if (dup2(fd, new_fd) == -1) _exit(127);
// It's a dup2.
if (fd == new_fd) {
// dup2(2) is a no-op if fd == new_fd, but POSIX suggests that we should
// manually remove the O_CLOEXEC flag in that case (because otherwise
// what use is the dup?).
// See https://www.austingroupbugs.net/view.php?id=411 for details.
int flags = fcntl(fd, F_GETFD, 0);
if (flags == -1 || fcntl(fd, F_SETFD, flags & ~FD_CLOEXEC) == -1) _exit(127);
} else {
if (dup2(fd, new_fd) == -1) _exit(127);
}
}
}
};

39
tests/spawn_test.cpp
View file

@ -508,3 +508,42 @@ TEST(spawn, signal_stress) {
AssertChildExited(pid, 99);
}
TEST(spawn, posix_spawn_dup2_CLOEXEC) {
int fds[2];
ASSERT_NE(-1, pipe(fds));
posix_spawn_file_actions_t fa;
ASSERT_EQ(0, posix_spawn_file_actions_init(&fa));
int fd = open("/proc/version", O_RDONLY | O_CLOEXEC);
ASSERT_NE(-1, fd);
ASSERT_EQ(0, posix_spawn_file_actions_addclose(&fa, fds[0]));
ASSERT_EQ(0, posix_spawn_file_actions_adddup2(&fa, fds[1], 1));
// dup2() is a no-op when the two fds are the same, so this won't clear
// O_CLOEXEC unless we're doing extra work to make that happen.
ASSERT_EQ(0, posix_spawn_file_actions_adddup2(&fa, fd, fd));
// Read /proc/self/fd/<fd> in the child...
std::string fdinfo_path = android::base::StringPrintf("/proc/self/fd/%d", fd);
ExecTestHelper eth;
eth.SetArgs({"cat", fdinfo_path.c_str(), nullptr});
pid_t pid;
ASSERT_EQ(0, posix_spawnp(&pid, eth.GetArg0(), &fa, nullptr, eth.GetArgs(), eth.GetEnv()));
ASSERT_EQ(0, posix_spawn_file_actions_destroy(&fa));
ASSERT_EQ(0, close(fds[1]));
std::string content;
ASSERT_TRUE(android::base::ReadFdToString(fds[0], &content));
ASSERT_EQ(0, close(fds[0]));
// ...and compare that to the parent. This is overkill really, since the very
// fact that the child had a valid file descriptor strongly implies that we
// removed O_CLOEXEC, but we may as well check that the child ended up with
// the *right* file descriptor :-)
std::string expected;
ASSERT_TRUE(android::base::ReadFdToString(fd, &expected));
ASSERT_EQ(expected, content);
AssertChildExited(pid, 0);
}