From dcc97c0887c57844c832f4497866320697811e88 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 9 Dec 2020 14:01:13 -0800 Subject: [PATCH] Disable fdtrack post-fork. Also delete some fdsan code that attempts to check for the post-fork state, but never will, because we update the cached pid upon fork. Bug: http://b/174542867 Test: /data/nativetest64/bionic-unit-tests/bionic-unit-tests Test: treehugger Change-Id: I9b748dac9de9b4c741897d93e64d31737e52bf8e --- libc/bionic/fdsan.cpp | 8 ---- libc/bionic/fdtrack.cpp | 8 +++- libc/bionic/fork.cpp | 4 +- libc/libc.map.txt | 1 + libc/platform/bionic/fdtrack.h | 4 ++ libc/private/bionic_fdtrack.h | 81 ++++++++++++++++++---------------- libfdtrack/fdtrack.cpp | 2 + tests/fdtrack_test.cpp | 22 ++++++++- 8 files changed, 80 insertions(+), 50 deletions(-) diff --git a/libc/bionic/fdsan.cpp b/libc/bionic/fdsan.cpp index 043510c63..48e8674d6 100644 --- a/libc/bionic/fdsan.cpp +++ b/libc/bionic/fdsan.cpp @@ -137,14 +137,6 @@ __printflike(1, 0) static void fdsan_error(const char* fmt, ...) { return; } - // Lots of code will (sensibly) fork, call close on all of their fds, - // and then exec. Compare our cached pid value against the real one to detect - // this scenario and permit it. - pid_t cached_pid = __get_cached_pid(); - if (cached_pid == 0 || cached_pid != syscall(__NR_getpid)) { - return; - } - struct { size_t size; char buf[512]; diff --git a/libc/bionic/fdtrack.cpp b/libc/bionic/fdtrack.cpp index 11235129d..89a208fbe 100644 --- a/libc/bionic/fdtrack.cpp +++ b/libc/bionic/fdtrack.cpp @@ -37,8 +37,14 @@ _Atomic(android_fdtrack_hook_t) __android_fdtrack_hook; +bool __android_fdtrack_globally_disabled = false; + +void android_fdtrack_set_globally_enabled(bool new_value) { + __android_fdtrack_globally_disabled = !new_value; +} + bool android_fdtrack_get_enabled() { - return !__get_bionic_tls().fdtrack_disabled; + return !__get_bionic_tls().fdtrack_disabled && !__android_fdtrack_globally_disabled; } bool android_fdtrack_set_enabled(bool new_value) { diff --git a/libc/bionic/fork.cpp b/libc/bionic/fork.cpp index 8c5cf2b1b..d432c6db7 100644 --- a/libc/bionic/fork.cpp +++ b/libc/bionic/fork.cpp @@ -31,6 +31,7 @@ #include #include "private/bionic_defs.h" +#include "private/bionic_fdtrack.h" #include "pthread_internal.h" __BIONIC_WEAK_FOR_NATIVE_BRIDGE_INLINE @@ -55,9 +56,10 @@ int fork() { int result = __clone_for_fork(); if (result == 0) { - // Disable fdsan post-fork, so we don't falsely trigger on processes that + // Disable fdsan and fdtrack post-fork, so we don't falsely trigger on processes that // fork, close all of their fds, and then exec. android_fdsan_set_error_level(ANDROID_FDSAN_ERROR_LEVEL_DISABLED); + android_fdtrack_set_globally_enabled(false); // Reset the stack_and_tls VMA name so it doesn't end with a tid from the // parent process. diff --git a/libc/libc.map.txt b/libc/libc.map.txt index 51a10763e..c31e30681 100644 --- a/libc/libc.map.txt +++ b/libc/libc.map.txt @@ -1772,6 +1772,7 @@ LIBC_PLATFORM { android_fdtrack_compare_exchange_hook; # llndk android_fdtrack_get_enabled; # llndk android_fdtrack_set_enabled; # llndk + android_fdtrack_set_globally_enabled; # llndk android_net_res_stats_get_info_for_net; android_net_res_stats_aggregate; android_net_res_stats_get_usable_servers; diff --git a/libc/platform/bionic/fdtrack.h b/libc/platform/bionic/fdtrack.h index 6eb379ba0..fe6ca84b8 100644 --- a/libc/platform/bionic/fdtrack.h +++ b/libc/platform/bionic/fdtrack.h @@ -70,4 +70,8 @@ bool android_fdtrack_compare_exchange_hook(android_fdtrack_hook_t* expected, and bool android_fdtrack_get_enabled() __INTRODUCED_IN(30); bool android_fdtrack_set_enabled(bool new_value) __INTRODUCED_IN(30); +// Globally enable/disable fdtrack. +// This is primaryily useful to reenable fdtrack after it's been automatically disabled post-fork. +void android_fdtrack_set_globally_enabled(bool new_value) __INTRODUCED_IN(31); + __END_DECLS diff --git a/libc/private/bionic_fdtrack.h b/libc/private/bionic_fdtrack.h index 259897c22..c05b32ba6 100644 --- a/libc/private/bionic_fdtrack.h +++ b/libc/private/bionic_fdtrack.h @@ -28,41 +28,43 @@ #pragma once -#include #include +#include #include "platform/bionic/fdtrack.h" #include "bionic/pthread_internal.h" -#include "private/bionic_tls.h" #include "private/ErrnoRestorer.h" +#include "private/bionic_tls.h" extern "C" _Atomic(android_fdtrack_hook_t) __android_fdtrack_hook; +extern "C" bool __android_fdtrack_globally_disabled; // Macro to record file descriptor creation. // e.g.: // int socket(int domain, int type, int protocol) { // return FDTRACK_CREATE_NAME("socket", __socket(domain, type, protocol)); // } -#define FDTRACK_CREATE_NAME(name, fd_value) \ - ({ \ - int __fd = (fd_value); \ - if (__fd != -1 && __predict_false(__android_fdtrack_hook) && \ - !__predict_false(__get_thread()->is_vforked())) { \ - bionic_tls& tls = __get_bionic_tls(); \ - /* fdtrack_disabled is only true during reentrant calls. */ \ - if (!__predict_false(tls.fdtrack_disabled)) { \ - ErrnoRestorer r; \ - tls.fdtrack_disabled = true; \ - android_fdtrack_event event; \ - event.fd = __fd; \ - event.type = ANDROID_FDTRACK_EVENT_TYPE_CREATE; \ - event.data.create.function_name = name; \ - atomic_load(&__android_fdtrack_hook)(&event); \ - tls.fdtrack_disabled = false; \ - } \ - } \ - __fd; \ +#define FDTRACK_CREATE_NAME(name, fd_value) \ + ({ \ + int __fd = (fd_value); \ + if (__fd != -1 && __predict_false(__android_fdtrack_hook) && \ + !__predict_false(__get_thread()->is_vforked())) { \ + bionic_tls& tls = __get_bionic_tls(); \ + /* fdtrack_disabled is only true during reentrant calls. */ \ + if (!__predict_false(tls.fdtrack_disabled) && \ + !__predict_false(__android_fdtrack_globally_disabled)) { \ + ErrnoRestorer r; \ + tls.fdtrack_disabled = true; \ + android_fdtrack_event event; \ + event.fd = __fd; \ + event.type = ANDROID_FDTRACK_EVENT_TYPE_CREATE; \ + event.data.create.function_name = name; \ + atomic_load (&__android_fdtrack_hook)(&event); \ + tls.fdtrack_disabled = false; \ + } \ + } \ + __fd; \ }) // Macro to record file descriptor creation, with the current function's name. @@ -74,22 +76,23 @@ extern "C" _Atomic(android_fdtrack_hook_t) __android_fdtrack_hook; // Macro to record file descriptor closure. // Note that this does not actually close the file descriptor. -#define FDTRACK_CLOSE(fd_value) \ - ({ \ - int __fd = (fd_value); \ - if (__fd != -1 && __predict_false(__android_fdtrack_hook) && \ - !__predict_false(__get_thread()->is_vforked())) { \ - bionic_tls& tls = __get_bionic_tls(); \ - if (!__predict_false(tls.fdtrack_disabled)) { \ - int saved_errno = errno; \ - tls.fdtrack_disabled = true; \ - android_fdtrack_event event; \ - event.fd = __fd; \ - event.type = ANDROID_FDTRACK_EVENT_TYPE_CLOSE; \ - atomic_load(&__android_fdtrack_hook)(&event); \ - tls.fdtrack_disabled = false; \ - errno = saved_errno; \ - } \ - } \ - __fd; \ +#define FDTRACK_CLOSE(fd_value) \ + ({ \ + int __fd = (fd_value); \ + if (__fd != -1 && __predict_false(__android_fdtrack_hook) && \ + !__predict_false(__get_thread()->is_vforked())) { \ + bionic_tls& tls = __get_bionic_tls(); \ + if (!__predict_false(tls.fdtrack_disabled) && \ + !__predict_false(__android_fdtrack_globally_disabled)) { \ + int saved_errno = errno; \ + tls.fdtrack_disabled = true; \ + android_fdtrack_event event; \ + event.fd = __fd; \ + event.type = ANDROID_FDTRACK_EVENT_TYPE_CLOSE; \ + atomic_load (&__android_fdtrack_hook)(&event); \ + tls.fdtrack_disabled = false; \ + errno = saved_errno; \ + } \ + } \ + __fd; \ }) diff --git a/libfdtrack/fdtrack.cpp b/libfdtrack/fdtrack.cpp index fd562741f..2e9cfbcd0 100644 --- a/libfdtrack/fdtrack.cpp +++ b/libfdtrack/fdtrack.cpp @@ -93,6 +93,8 @@ __attribute__((constructor)) static void ctor() { android_fdtrack_hook_t expected = nullptr; installed = android_fdtrack_compare_exchange_hook(&expected, &fd_hook); } + + android_fdtrack_set_globally_enabled(true); } __attribute__((destructor)) static void dtor() { diff --git a/tests/fdtrack_test.cpp b/tests/fdtrack_test.cpp index 13f1b2e18..9fcb40283 100644 --- a/tests/fdtrack_test.cpp +++ b/tests/fdtrack_test.cpp @@ -57,8 +57,13 @@ void DumpEvent(std::vector* events, size_t index) { } } -std::vector FdtrackRun(void (*func)()) { +std::vector FdtrackRun(void (*func)(), bool reenable = true) { // Each bionic test is run in separate process, so we can safely use a static here. + // However, since they're all forked, we need to reenable fdtrack. + if (reenable) { + android_fdtrack_set_globally_enabled(true); + } + static std::vector events; events.clear(); @@ -129,6 +134,21 @@ TEST(fdtrack, close) { #endif } +TEST(fdtrack, fork) { +#if defined(__BIONIC__) + ASSERT_EXIT( + []() { + static int fd = open("/dev/null", O_WRONLY | O_CLOEXEC); + ASSERT_NE(-1, fd); + + auto events = FdtrackRun([]() { close(fd); }, false); + ASSERT_EQ(0U, events.size()); + exit(0); + }(), + testing::ExitedWithCode(0), ""); +#endif +} + TEST(fdtrack, enable_disable) { #if defined(__BIONIC__) static int fd1 = -1;