Fix freopen() where the path is null.
This has been in the standard since C99, but we've never supported it
before. It's apparently used by SPIRV-Tools.
I tried implementing this the other way (with fcntl(2)) first, but
eventually realized that that's more complicated and gives worse
results. This implementation assumes that /proc is mounted, but so much
of libc relies on that at this point that I don't think there's any
realistic case where the fcntl(2) implementation would be preferable,
and there are many where it's not.
The fact that no-one's mentioned this until now suggests that it's not a
heavily used feature anyway.
I've also replaced AssertCloseOnExec() with a CloseOnExec()
boolean-valued function instead, because it's really annoying getting
assertion failures that don't point you at the test line in question,
and instead point to some common helper code.
Test: treehugger
Change-Id: Ia2e53bf2664a4f782581042054ecd492830e2aed
diff --git a/tests/eventfd_test.cpp b/tests/eventfd_test.cpp
index 3c303c2..eb423c1 100644
--- a/tests/eventfd_test.cpp
+++ b/tests/eventfd_test.cpp
@@ -51,7 +51,7 @@
constexpr unsigned int kInitialValue = 2;
int fd = eventfd(kInitialValue, EFD_CLOEXEC);
ASSERT_NE(-1, fd);
- AssertCloseOnExec(fd, true);
+ ASSERT_TRUE(CloseOnExec(fd));
eventfd_t value = 123;
ASSERT_EQ(0, eventfd_read(fd, &value));
@@ -61,7 +61,7 @@
fd = eventfd(kInitialValue, EFD_NONBLOCK | EFD_CLOEXEC);
ASSERT_NE(-1, fd);
- AssertCloseOnExec(fd, true);
+ ASSERT_TRUE(CloseOnExec(fd));
value = 123;
ASSERT_EQ(0, eventfd_read(fd, &value));
diff --git a/tests/stdio_test.cpp b/tests/stdio_test.cpp
index 5680f95..afbbf26 100644
--- a/tests/stdio_test.cpp
+++ b/tests/stdio_test.cpp
@@ -1956,34 +1956,64 @@
#endif
}
-TEST(STDIO_TEST, fdopen_CLOEXEC) {
- int fd = open("/proc/version", O_RDONLY);
- ASSERT_TRUE(fd != -1);
-
+TEST(STDIO_TEST, fdopen_add_CLOEXEC) {
// This fd doesn't have O_CLOEXEC...
- AssertCloseOnExec(fd, false);
-
- FILE* fp = fdopen(fd, "re");
- ASSERT_TRUE(fp != nullptr);
-
+ int fd = open("/proc/version", O_RDONLY);
+ ASSERT_FALSE(CloseOnExec(fd));
// ...but the new one does.
- AssertCloseOnExec(fileno(fp), true);
+ FILE* fp = fdopen(fd, "re");
+ ASSERT_TRUE(CloseOnExec(fileno(fp)));
+ fclose(fp);
+}
+
+TEST(STDIO_TEST, fdopen_remove_CLOEXEC) {
+ // This fd has O_CLOEXEC...
+ int fd = open("/proc/version", O_RDONLY | O_CLOEXEC);
+ ASSERT_TRUE(CloseOnExec(fd));
+ // ...but the new one doesn't.
+ FILE* fp = fdopen(fd, "r");
+ ASSERT_TRUE(CloseOnExec(fileno(fp)));
+ fclose(fp);
+}
+
+TEST(STDIO_TEST, freopen_add_CLOEXEC) {
+ // This FILE* doesn't have O_CLOEXEC...
+ FILE* fp = fopen("/proc/version", "r");
+ ASSERT_FALSE(CloseOnExec(fileno(fp)));
+ // ...but the new one does.
+ fp = freopen("/proc/version", "re", fp);
+ ASSERT_TRUE(CloseOnExec(fileno(fp)));
fclose(fp);
}
-TEST(STDIO_TEST, freopen_CLOEXEC) {
- FILE* fp = fopen("/proc/version", "r");
- ASSERT_TRUE(fp != nullptr);
+TEST(STDIO_TEST, freopen_remove_CLOEXEC) {
+ // This FILE* has O_CLOEXEC...
+ FILE* fp = fopen("/proc/version", "re");
+ ASSERT_TRUE(CloseOnExec(fileno(fp)));
+ // ...but the new one doesn't.
+ fp = freopen("/proc/version", "r", fp);
+ ASSERT_FALSE(CloseOnExec(fileno(fp)));
+ fclose(fp);
+}
+TEST(STDIO_TEST, freopen_null_filename_add_CLOEXEC) {
// This FILE* doesn't have O_CLOEXEC...
- AssertCloseOnExec(fileno(fp), false);
-
- fp = freopen("/proc/version", "re", fp);
-
+ FILE* fp = fopen("/proc/version", "r");
+ ASSERT_FALSE(CloseOnExec(fileno(fp)));
// ...but the new one does.
- AssertCloseOnExec(fileno(fp), true);
+ fp = freopen(nullptr, "re", fp);
+ ASSERT_TRUE(CloseOnExec(fileno(fp)));
+ fclose(fp);
+}
+TEST(STDIO_TEST, freopen_null_filename_remove_CLOEXEC) {
+ // This FILE* has O_CLOEXEC...
+ FILE* fp = fopen("/proc/version", "re");
+ ASSERT_TRUE(CloseOnExec(fileno(fp)));
+ // ...but the new one doesn't.
+ fp = freopen(nullptr, "r", fp);
+ ASSERT_FALSE(CloseOnExec(fileno(fp)));
fclose(fp);
}
@@ -2883,3 +2913,24 @@
char buf[L_tmpnam];
tmpnam_test(buf);
}
+
+TEST(STDIO_TEST, freopen_null_filename_mode) {
+ TemporaryFile tf;
+ FILE* fp = fopen(tf.path, "r");
+ ASSERT_TRUE(fp != nullptr);
+
+ // "r" = O_RDONLY
+ char buf[1];
+ ASSERT_EQ(0, read(fileno(fp), buf, 1));
+ ASSERT_EQ(-1, write(fileno(fp), "hello", 1));
+ // "r+" = O_RDWR
+ fp = freopen(nullptr, "r+", fp);
+ ASSERT_EQ(0, read(fileno(fp), buf, 1));
+ ASSERT_EQ(1, write(fileno(fp), "hello", 1));
+ // "w" = O_WRONLY
+ fp = freopen(nullptr, "w", fp);
+ ASSERT_EQ(-1, read(fileno(fp), buf, 1));
+ ASSERT_EQ(1, write(fileno(fp), "hello", 1));
+
+ fclose(fp);
+}
diff --git a/tests/stdlib_test.cpp b/tests/stdlib_test.cpp
index c7b2ad8..bb1fd7c 100644
--- a/tests/stdlib_test.cpp
+++ b/tests/stdlib_test.cpp
@@ -456,12 +456,12 @@
TEST(stdlib, mkostemp64) {
MyTemporaryFile tf([](char* path) { return mkostemp64(path, O_CLOEXEC); });
- AssertCloseOnExec(tf.fd, true);
+ ASSERT_TRUE(CloseOnExec(tf.fd));
}
TEST(stdlib, mkostemp) {
MyTemporaryFile tf([](char* path) { return mkostemp(path, O_CLOEXEC); });
- AssertCloseOnExec(tf.fd, true);
+ ASSERT_TRUE(CloseOnExec(tf.fd));
}
TEST(stdlib, mkstemp64) {
diff --git a/tests/sys_epoll_test.cpp b/tests/sys_epoll_test.cpp
index 18cf380..fb2a48f 100644
--- a/tests/sys_epoll_test.cpp
+++ b/tests/sys_epoll_test.cpp
@@ -83,14 +83,14 @@
TEST(sys_epoll, epoll_create1) {
int fd;
fd = epoll_create(1);
- AssertCloseOnExec(fd, false);
+ ASSERT_FALSE(CloseOnExec(fd));
close(fd);
fd = epoll_create1(0);
- AssertCloseOnExec(fd, false);
+ ASSERT_FALSE(CloseOnExec(fd));
close(fd);
fd = epoll_create1(EPOLL_CLOEXEC);
- AssertCloseOnExec(fd, true);
+ ASSERT_TRUE(CloseOnExec(fd));
close(fd);
}
diff --git a/tests/sys_socket_test.cpp b/tests/sys_socket_test.cpp
index 0342547..421a817 100644
--- a/tests/sys_socket_test.cpp
+++ b/tests/sys_socket_test.cpp
@@ -109,7 +109,7 @@
ASSERT_NE(fd_acc, -1) << strerror(errno);
// Check that SOCK_CLOEXEC was set properly.
- AssertCloseOnExec(fd_acc, true);
+ ASSERT_TRUE(CloseOnExec(fd_acc));
close(fd_acc);
}
diff --git a/tests/unistd_test.cpp b/tests/unistd_test.cpp
index 41ca8b4..595ba90 100644
--- a/tests/unistd_test.cpp
+++ b/tests/unistd_test.cpp
@@ -1166,10 +1166,10 @@
TEST(UNISTD_TEST, dup3) {
int fd = open("/proc/version", O_RDONLY);
ASSERT_EQ(666, dup3(fd, 666, 0));
- AssertCloseOnExec(666, false);
+ ASSERT_FALSE(CloseOnExec(666));
close(666);
ASSERT_EQ(667, dup3(fd, 667, O_CLOEXEC));
- AssertCloseOnExec(667, true);
+ ASSERT_TRUE(CloseOnExec(667));
close(667);
close(fd);
}
diff --git a/tests/utils.h b/tests/utils.h
index 94ff050..145ba1a 100644
--- a/tests/utils.h
+++ b/tests/utils.h
@@ -185,10 +185,14 @@
}
}
-static inline void AssertCloseOnExec(int fd, bool close_on_exec) {
+static inline bool CloseOnExec(int fd) {
int flags = fcntl(fd, F_GETFD);
- ASSERT_NE(flags, -1);
- ASSERT_EQ(close_on_exec ? FD_CLOEXEC : 0, flags & FD_CLOEXEC);
+ // This isn't ideal, but the alternatives are worse:
+ // * If we return void and use ASSERT_NE here, we get failures at utils.h:191
+ // rather than in the relevant test.
+ // * If we ignore failures of fcntl(), well, that's obviously a bad idea.
+ if (flags == -1) abort();
+ return flags & FD_CLOEXEC;
}
// The absolute path to the executable