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/libc/stdio/stdio.cpp b/libc/stdio/stdio.cpp
index c7b1ba4..2b0e2b2 100644
--- a/libc/stdio/stdio.cpp
+++ b/libc/stdio/stdio.cpp
@@ -50,11 +50,12 @@
#include <async_safe/log.h>
-#include "local.h"
#include "glue.h"
+#include "local.h"
+#include "private/ErrnoRestorer.h"
+#include "private/FdPath.h"
#include "private/__bionic_get_shell_path.h"
#include "private/bionic_fortify.h"
-#include "private/ErrnoRestorer.h"
#include "private/thread_private.h"
#include "private/bsd_sys_param.h" // For ALIGN/ALIGNBYTES.
@@ -225,25 +226,26 @@
_fwalk(__sflush);
}
-static FILE* __fopen(int fd, int flags) {
+static FILE* __finit(FILE* fp, int fd, int flags) {
+ if (fp == nullptr) return nullptr;
+
+ fp->_file = fd;
+ android_fdsan_exchange_owner_tag(fd, 0, __get_file_tag(fp));
+ fp->_flags = flags;
+ fp->_cookie = fp;
+ fp->_read = __sread;
+ fp->_write = __swrite;
+ fp->_close = __sclose;
+ _EXT(fp)->_seek64 = __sseek64;
+
#if !defined(__LP64__)
if (fd > SHRT_MAX) {
errno = EMFILE;
+ fclose(fp);
return nullptr;
}
#endif
- FILE* fp = __sfp();
- if (fp != nullptr) {
- fp->_file = fd;
- android_fdsan_exchange_owner_tag(fd, 0, __get_file_tag(fp));
- fp->_flags = flags;
- fp->_cookie = fp;
- fp->_read = __sread;
- fp->_write = __swrite;
- fp->_close = __sclose;
- _EXT(fp)->_seek64 = __sseek64;
- }
return fp;
}
@@ -257,14 +259,15 @@
return nullptr;
}
- FILE* fp = __fopen(fd, flags);
+ FILE* fp = __finit(__sfp(), fd, flags);
if (fp == nullptr) {
ErrnoRestorer errno_restorer;
close(fd);
return nullptr;
}
- // For append mode, even though we use O_APPEND, we need to seek to the end now.
+ // For append mode, O_APPEND sets the write position for free, but we need to
+ // set the read position manually.
if ((mode_flags & O_APPEND) != 0) __sseek64(fp, 0, SEEK_END);
return fp;
}
@@ -295,15 +298,26 @@
fcntl(fd, F_SETFD, tmp | FD_CLOEXEC);
}
- return __fopen(fd, flags);
+ return __finit(__sfp(), fd, flags);
}
-// Re-direct an existing, open (probably) file to some other file.
-// ANSI is written such that the original file gets closed if at
-// all possible, no matter what.
-// TODO: rewrite this mess completely.
FILE* freopen(const char* file, const char* mode, FILE* fp) {
CHECK_FP(fp);
+
+ // POSIX says: "If pathname is a null pointer, the freopen() function shall
+ // attempt to change the mode of the stream to that specified by mode, as if
+ // the name of the file currently associated with the stream had been used. In
+ // this case, the file descriptor associated with the stream need not be
+ // closed if the call to freopen() succeeds. It is implementation-defined
+ // which changes of mode are permitted (if any), and under what
+ // circumstances."
+ //
+ // Linux is quite restrictive about what changes you can make with F_SETFL,
+ // and in particular won't let you touch the access bits. It's easiest and
+ // most effective to just rely on /proc/self/fd/...
+ FdPath fd_path(fp->_file);
+ if (file == nullptr) file = fd_path.c_str();
+
int mode_flags;
int flags = __sflags(mode, &mode_flags);
if (flags == 0) {
@@ -313,6 +327,8 @@
ScopedFileLock sfl(fp);
+ // TODO: rewrite this mess completely.
+
// There are actually programs that depend on being able to "freopen"
// descriptors that weren't originally open. Keep this from breaking.
// Remember whether the stream was open to begin with, and which file
@@ -382,24 +398,12 @@
}
}
- // _file is only a short.
- if (fd > SHRT_MAX) {
- fp->_flags = 0; // Release.
- errno = EMFILE;
- return nullptr;
- }
+ fp = __finit(fp, fd, flags);
- fp->_flags = flags;
- fp->_file = fd;
- android_fdsan_exchange_owner_tag(fd, 0, __get_file_tag(fp));
- fp->_cookie = fp;
- fp->_read = __sread;
- fp->_write = __swrite;
- fp->_close = __sclose;
- _EXT(fp)->_seek64 = __sseek64;
+ // For append mode, O_APPEND sets the write position for free, but we need to
+ // set the read position manually.
+ if (fp && (mode_flags & O_APPEND) != 0) __sseek64(fp, 0, SEEK_END);
- // For append mode, even though we use O_APPEND, we need to seek to the end now.
- if ((mode_flags & O_APPEND) != 0) __sseek64(fp, 0, SEEK_END);
return fp;
}
__strong_alias(freopen64, freopen);
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