From 76144aaa6397fe9e16893882cf59c5c9c0684a66 Mon Sep 17 00:00:00 2001 From: Yabin Cui Date: Thu, 19 Nov 2015 13:52:16 -0800 Subject: [PATCH] Change _stdio_handles_locking into _caller_handles_locking. It is reported by tsan that funlockfile() can unlock an unlocked mutex. It happens when printf() is called before fopen() or other stdio stuff. As FLOCKFILE(fp) is called before __sinit(), _stdio_handles_locking is false, and _FLOCK(fp) will not be locked. But then cantwrite(fp) in __vfprintf() calls__sinit(), which makes _stdio_handles_locking become true, and FUNLOCKFILE(fp) unlocks _FLOCK(fp). Change _stdio_handles_locking into _caller_handles_locking, so __sinit() won't change its value. Add test due to my previous fault. Bug: 25392375 Change-Id: I483e3c3cdb28da65e62f1fd9615bf58c5403b4dd --- libc/bionic/flockfile.cpp | 6 +++--- libc/stdio/local.h | 8 ++++---- libc/stdio/stdio_ext.cpp | 4 ++-- tests/pthread_test.cpp | 23 ----------------------- tests/stdio_ext_test.cpp | 22 ++++++++++++++++++++++ tests/utils.h | 27 +++++++++++++++++++++++++++ 6 files changed, 58 insertions(+), 32 deletions(-) diff --git a/libc/bionic/flockfile.cpp b/libc/bionic/flockfile.cpp index b73907cbc..db688016b 100644 --- a/libc/bionic/flockfile.cpp +++ b/libc/bionic/flockfile.cpp @@ -40,7 +40,7 @@ void flockfile(FILE* fp) { __sinit(); } - if (fp != NULL) { + if (fp != nullptr) { pthread_mutex_lock(&_FLOCK(fp)); } } @@ -52,7 +52,7 @@ int ftrylockfile(FILE* fp) { // The specification for ftrylockfile() says it returns 0 on success, // or non-zero on error. So return an errno code directly on error. - if (fp == NULL) { + if (fp == nullptr) { return EINVAL; } @@ -64,7 +64,7 @@ void funlockfile(FILE* fp) { __sinit(); } - if (fp != NULL) { + if (fp != nullptr) { pthread_mutex_unlock(&_FLOCK(fp)); } } diff --git a/libc/stdio/local.h b/libc/stdio/local.h index ced94e3e0..3ae7059da 100644 --- a/libc/stdio/local.h +++ b/libc/stdio/local.h @@ -109,7 +109,7 @@ struct __sfileext { pthread_mutex_t _lock; /* __fsetlocking support */ - bool _stdio_handles_locking; + bool _caller_handles_locking; }; #if defined(__cplusplus) @@ -131,7 +131,7 @@ do { \ pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); \ pthread_mutex_init(&_FLOCK(fp), &attr); \ pthread_mutexattr_destroy(&attr); \ - _EXT(fp)->_stdio_handles_locking = true; \ + _EXT(fp)->_caller_handles_locking = false; \ } while (0) #define _FILEEXT_SETUP(f, fext) \ @@ -208,8 +208,8 @@ extern void __atexit_register_cleanup(void (*)(void)); (fp)->_lb._base = NULL; \ } -#define FLOCKFILE(fp) if (_EXT(fp)->_stdio_handles_locking) flockfile(fp) -#define FUNLOCKFILE(fp) if (_EXT(fp)->_stdio_handles_locking) funlockfile(fp) +#define FLOCKFILE(fp) if (!_EXT(fp)->_caller_handles_locking) flockfile(fp) +#define FUNLOCKFILE(fp) if (!_EXT(fp)->_caller_handles_locking) funlockfile(fp) #define FLOATING_POINT #define PRINTF_WIDE_CHAR diff --git a/libc/stdio/stdio_ext.cpp b/libc/stdio/stdio_ext.cpp index 310076a3a..f273d45fe 100644 --- a/libc/stdio/stdio_ext.cpp +++ b/libc/stdio/stdio_ext.cpp @@ -74,7 +74,7 @@ void _flushlbf() { } int __fsetlocking(FILE* fp, int type) { - int old_state = _EXT(fp)->_stdio_handles_locking ? FSETLOCKING_INTERNAL : FSETLOCKING_BYCALLER; + int old_state = _EXT(fp)->_caller_handles_locking ? FSETLOCKING_BYCALLER : FSETLOCKING_INTERNAL; if (type == FSETLOCKING_QUERY) { return old_state; } @@ -84,7 +84,7 @@ int __fsetlocking(FILE* fp, int type) { __libc_fatal("Bad type (%d) passed to __fsetlocking", type); } - _EXT(fp)->_stdio_handles_locking = (type == FSETLOCKING_INTERNAL); + _EXT(fp)->_caller_handles_locking = (type == FSETLOCKING_BYCALLER); return old_state; } diff --git a/tests/pthread_test.cpp b/tests/pthread_test.cpp index e237d2647..284af8f5e 100755 --- a/tests/pthread_test.cpp +++ b/tests/pthread_test.cpp @@ -30,12 +30,8 @@ #include #include -#include #include -#include -#include - #include "private/bionic_macros.h" #include "private/ScopeGuard.h" #include "BionicDeathTest.h" @@ -43,8 +39,6 @@ #include "utils.h" -extern "C" pid_t gettid(); - TEST(pthread, pthread_key_create) { pthread_key_t key; ASSERT_EQ(0, pthread_key_create(&key, NULL)); @@ -721,23 +715,6 @@ TEST(pthread, pthread_rwlock_smoke) { ASSERT_EQ(0, pthread_rwlock_destroy(&l)); } -static void WaitUntilThreadSleep(std::atomic& tid) { - while (tid == 0) { - usleep(1000); - } - std::string filename = android::base::StringPrintf("/proc/%d/stat", tid.load()); - std::regex regex {R"(\s+S\s+)"}; - - while (true) { - std::string content; - ASSERT_TRUE(android::base::ReadFileToString(filename, &content)); - if (std::regex_search(content, regex)) { - break; - } - usleep(1000); - } -} - struct RwlockWakeupHelperArg { pthread_rwlock_t lock; enum Progress { diff --git a/tests/stdio_ext_test.cpp b/tests/stdio_ext_test.cpp index c95cbbdd5..78725675d 100644 --- a/tests/stdio_ext_test.cpp +++ b/tests/stdio_ext_test.cpp @@ -30,6 +30,7 @@ #include #include "TemporaryFile.h" +#include "utils.h" TEST(stdio_ext, __fbufsize) { FILE* fp = fopen("/proc/version", "r"); @@ -140,3 +141,24 @@ TEST(stdio_ext, __fsetlocking) { ASSERT_EQ(FSETLOCKING_INTERNAL, __fsetlocking(fp, FSETLOCKING_QUERY)); fclose(fp); } + +static void LockingByCallerHelper(std::atomic* pid) { + *pid = gettid(); + flockfile(stdout); + funlockfile(stdout); +} + +TEST(stdio_ext, __fsetlocking_BYCALLER) { + // Check if users can use flockfile/funlockfile to protect stdio operations. + int old_state = __fsetlocking(stdout, FSETLOCKING_BYCALLER); + flockfile(stdout); + pthread_t thread; + std::atomic pid(0); + ASSERT_EQ(0, pthread_create(&thread, nullptr, + reinterpret_cast(LockingByCallerHelper), &pid)); + WaitUntilThreadSleep(pid); + funlockfile(stdout); + + ASSERT_EQ(0, pthread_join(thread, nullptr)); + __fsetlocking(stdout, old_state); +} diff --git a/tests/utils.h b/tests/utils.h index 53cf6b6ea..9e77f244e 100644 --- a/tests/utils.h +++ b/tests/utils.h @@ -18,6 +18,14 @@ #define __TEST_UTILS_H #include #include +#include + +#include +#include +#include + +#include +#include #include "private/ScopeGuard.h" @@ -82,4 +90,23 @@ class Maps { } }; +extern "C" pid_t gettid(); + +static inline void WaitUntilThreadSleep(std::atomic& tid) { + while (tid == 0) { + usleep(1000); + } + std::string filename = android::base::StringPrintf("/proc/%d/stat", tid.load()); + std::regex regex {R"(\s+S\s+)"}; + + while (true) { + std::string content; + ASSERT_TRUE(android::base::ReadFileToString(filename, &content)); + if (std::regex_search(content, regex)) { + break; + } + usleep(1000); + } +} + #endif