Revert fwalk/sfp locking to fix concurrent reads
The locking can fail in a couple of ways:
- A concurrent fread from an unbuffered or line-buffered file flushes
the output of other line-buffered files, and if _fwalk locks every
file, then the fread blocks until other file reads have completed.
- __sfp can initialize a file lock while _fwalk is locking/unlocking it.
For now, revert to the behavior Bionic had in previous releases. This
commit reverts the file locking parts of commit
468efc80da2504f4ae7de8b5e137426d44dda9d7.
Bug: http://b/131251441
Bug: http://b/130189834
Test: bionic unit tests
Change-Id: I9e20b9cd8ccd14e7962f7308e174f08af72b56c6
diff --git a/libc/stdio/local.h b/libc/stdio/local.h
index d306a21..1ecf122 100644
--- a/libc/stdio/local.h
+++ b/libc/stdio/local.h
@@ -204,6 +204,7 @@
__LIBC32_LEGACY_PUBLIC__ int _fwalk(int (*)(FILE*));
off64_t __sseek64(void*, off64_t, int);
+int __sflush_locked(FILE*);
int __swhatbuf(FILE*, size_t*, int*);
wint_t __fgetwc_unlock(FILE*);
wint_t __ungetwc(wint_t, FILE*);
diff --git a/libc/stdio/refill.c b/libc/stdio/refill.c
index cfa2bfd..1df4191 100644
--- a/libc/stdio/refill.c
+++ b/libc/stdio/refill.c
@@ -40,7 +40,7 @@
lflush(FILE *fp)
{
if ((fp->_flags & (__SLBF|__SWR)) == (__SLBF|__SWR))
- return (__sflush(fp)); /* ignored... */
+ return (__sflush_locked(fp)); /* ignored... */
return (0);
}
diff --git a/libc/stdio/stdio.cpp b/libc/stdio/stdio.cpp
index 4cec757..91c7689 100644
--- a/libc/stdio/stdio.cpp
+++ b/libc/stdio/stdio.cpp
@@ -106,7 +106,7 @@
FILE* stdout = &__sF[1];
FILE* stderr = &__sF[2];
-static pthread_mutex_t __stdio_mutex = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
+static pthread_mutex_t __stdio_mutex = PTHREAD_MUTEX_INITIALIZER;
static uint64_t __get_file_tag(FILE* fp) {
// Don't use a tag for the standard streams.
@@ -211,21 +211,23 @@
}
int _fwalk(int (*callback)(FILE*)) {
- pthread_mutex_lock(&__stdio_mutex);
int result = 0;
for (glue* g = &__sglue; g != nullptr; g = g->next) {
FILE* fp = g->iobs;
for (int n = g->niobs; --n >= 0; ++fp) {
- ScopedFileLock sfl(fp);
if (fp->_flags != 0 && (fp->_flags & __SIGN) == 0) {
result |= (*callback)(fp);
}
}
}
- pthread_mutex_unlock(&__stdio_mutex);
return result;
}
+extern "C" __LIBC_HIDDEN__ void __libc_stdio_cleanup(void) {
+ // Equivalent to fflush(nullptr), but without all the locking since we're shutting down anyway.
+ _fwalk(__sflush);
+}
+
static FILE* __fopen(int fd, int flags) {
#if !defined(__LP64__)
if (fd > SHRT_MAX) {
@@ -520,6 +522,11 @@
return 0;
}
+int __sflush_locked(FILE* fp) {
+ ScopedFileLock sfl(fp);
+ return __sflush(fp);
+}
+
int __sread(void* cookie, char* buf, int n) {
FILE* fp = reinterpret_cast<FILE*>(cookie);
return TEMP_FAILURE_RETRY(read(fp->_file, buf, n));
@@ -1061,7 +1068,7 @@
}
static int fflush_all() {
- return _fwalk(__sflush);
+ return _fwalk(__sflush_locked);
}
int fflush(FILE* fp) {
diff --git a/libc/stdlib/atexit.c b/libc/stdlib/atexit.c
index 692e0c0..0efb118 100644
--- a/libc/stdlib/atexit.c
+++ b/libc/stdlib/atexit.c
@@ -188,7 +188,8 @@
/* If called via exit(), flush output of all open files. */
if (dso == NULL) {
- fflush(NULL);
+ extern void __libc_stdio_cleanup(void);
+ __libc_stdio_cleanup();
}
/* BEGIN android-changed: call __unregister_atfork if dso is not null */
diff --git a/tests/stdio_test.cpp b/tests/stdio_test.cpp
index 65a942c..c237d6d 100644
--- a/tests/stdio_test.cpp
+++ b/tests/stdio_test.cpp
@@ -29,6 +29,7 @@
#include <locale.h>
#include <string>
+#include <thread>
#include <vector>
#include <android-base/file.h>
@@ -2577,3 +2578,24 @@
ASSERT_LT(0, length);
ASSERT_EQ("/proc/self/fd/2", std::string(path, length));
}
+
+TEST(STDIO_TEST, fread_with_locked_file) {
+ // Reading an unbuffered/line-buffered file from one thread shouldn't block on
+ // files locked on other threads, even if it flushes some line-buffered files.
+ FILE* fp1 = fopen("/dev/zero", "r");
+ ASSERT_TRUE(fp1 != nullptr);
+ flockfile(fp1);
+
+ std::thread([] {
+ for (int mode : { _IONBF, _IOLBF }) {
+ FILE* fp2 = fopen("/dev/zero", "r");
+ ASSERT_TRUE(fp2 != nullptr);
+ setvbuf(fp2, nullptr, mode, 0);
+ ASSERT_EQ('\0', fgetc(fp2));
+ fclose(fp2);
+ }
+ }).join();
+
+ funlockfile(fp1);
+ fclose(fp1);
+}