From 0f52f33196f12f7488d31232f6c3b0eb8d1f8f5e Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 1 Feb 2024 15:36:45 +0100 Subject: [PATCH] Patch libunwind to hotfix validation pipe creation race A change in libunwind 1.8.0 https://github.com/libunwind/libunwind/pull/536 has introduced a race in one of its commits (https://github.com/libunwind/libunwind/commit/ec03043244082b8f552881ba9fb790aa49c85468) This race causes relatively rare unwind failures when multiple threads race for an initialization of a pipe that is used to verify that a memory address is accessible. This change reverts the change in the affected file. Once a proper fix is made in the libunwind, this patch would be reverted and the real fix applied. This issue is currently blocking source build and the new exception handling enabling by default. --- src/native/external/libunwind-version.txt | 1 + .../libunwind/src/mi/Gaddress_validator.c | 231 ++++++++++-------- 2 files changed, 126 insertions(+), 106 deletions(-) diff --git a/src/native/external/libunwind-version.txt b/src/native/external/libunwind-version.txt index 4a989653b3f204..776043575539fd 100644 --- a/src/native/external/libunwind-version.txt +++ b/src/native/external/libunwind-version.txt @@ -7,3 +7,4 @@ Apply https://github.com/libunwind/libunwind/pull/703 Apply https://github.com/libunwind/libunwind/pull/704 Revert https://github.com/libunwind/libunwind/pull/503 # issue: https://github.com/libunwind/libunwind/issues/702 Apply https://github.com/libunwind/libunwind/pull/714 +Revert https://github.com/libunwind/libunwind/commit/ec03043244082b8f552881ba9fb790aa49c85468 and follow up changes in the same file # issue: https://github.com/libunwind/libunwind/issues/715 diff --git a/src/native/external/libunwind/src/mi/Gaddress_validator.c b/src/native/external/libunwind/src/mi/Gaddress_validator.c index aaf5a0941214fa..257d1dca93ea8c 100644 --- a/src/native/external/libunwind/src/mi/Gaddress_validator.c +++ b/src/native/external/libunwind/src/mi/Gaddress_validator.c @@ -24,7 +24,6 @@ */ #include "libunwind_i.h" - #ifdef UNW_REMOTE_ONLY bool unw_address_is_valid(UNUSED unw_word_t addr, UNUSED size_t len) @@ -35,17 +34,19 @@ unw_address_is_valid(UNUSED unw_word_t addr, UNUSED size_t len) #else /* !UNW_REMOTE_ONLY */ -#include +static pthread_once_t _unw_address_validator_init_once = PTHREAD_ONCE_INIT; +static sig_atomic_t _unw_address_validator_initialized = 0; +static int _mem_validate_pipe[2] = {-1, -1}; +static bool (*_mem_validate_func) (unw_word_t, size_t); +#pragma weak pthread_once -static atomic_flag _unw_address_validator_initialized = ATOMIC_FLAG_INIT; -static int _mem_validate_pipe[2] = {-1, -1}; #ifdef HAVE_PIPE2 -static int +static void _do_pipe2 (int pipefd[2]) { - return pipe2 (pipefd, O_CLOEXEC | O_NONBLOCK); + int result UNUSED = pipe2 (pipefd, O_CLOEXEC | O_NONBLOCK); } #else static void @@ -53,80 +54,54 @@ _set_pipe_flags (int fd) { int fd_flags = fcntl (fd, F_GETFD, 0); int status_flags = fcntl (fd, F_GETFL, 0); - fd_flags |= FD_CLOEXEC; fcntl (fd, F_SETFD, fd_flags); - status_flags |= O_NONBLOCK; fcntl (fd, F_SETFL, status_flags); } - -static int +static void _do_pipe2 (int pipefd[2]) { - if (pipe (pipefd) < 0) - { - return -1; - } + pipe (pipefd); _set_pipe_flags(pipefd[0]); _set_pipe_flags(pipefd[1]); } #endif - - -static int +static void _open_pipe (void) { if (_mem_validate_pipe[0] != -1) close (_mem_validate_pipe[0]); if (_mem_validate_pipe[1] != -1) close (_mem_validate_pipe[1]); - - return _do_pipe2 (_mem_validate_pipe); + _do_pipe2 (_mem_validate_pipe); } - - /** * Test is a memory address is valid by trying to write from it * @param[in] addr The address to validate * - * @returns true if the memory address is valid (readable), false otherwise. + * @returns true of the memory address is valid (readable), false otherwise. * * This check works by using the address as a (one-byte) buffer in a * write-to-pipe operation. The write will fail if the memory is not in the - * process's address space and marked as readable. The read will force the page - * to be swapped in if it's not already there. + * process's address space and marked as readable. */ static bool _write_validate (unw_word_t addr) { int ret = -1; ssize_t bytes = 0; - - if (unlikely (!atomic_flag_test_and_set(&_unw_address_validator_initialized))) - { - if (_open_pipe () != 0) - { - return false; - } - } - do { char buf; bytes = read (_mem_validate_pipe[0], &buf, 1); } while ( errno == EINTR ); - if (!(bytes > 0 || errno == EAGAIN || errno == EWOULDBLOCK)) { // re-open closed pipe - if (_open_pipe () != 0) - { - return false; - } + _open_pipe (); } - do { #ifdef HAVE_SYS_SYSCALL_H @@ -137,11 +112,71 @@ _write_validate (unw_word_t addr) #endif } while ( errno == EINTR ); - return ret > 0; } +static bool +_msync_validate (unw_word_t addr, size_t len) +{ + if (msync ( (void *)unw_page_start (addr), len, MS_ASYNC) != 0) + { + return false; + } + + return _write_validate (addr); +} + + +#ifdef HAVE_MINCORE +static bool +_mincore_validate (unw_word_t addr, size_t len) +{ + unsigned char mvec[2]; /* Unaligned access may cross page boundary */ + + /* mincore could fail with EAGAIN but we conservatively return false + instead of looping. */ + if (mincore ((void *)unw_page_start (addr), len, mvec) != 0) + { + return false; + } + + return _write_validate (addr); +} +#endif + + +static void +_unw_address_validator_init(void) +{ + _open_pipe (); + + /* Work out dynamically what memory validation function to use. */ +#ifdef HAVE_MINCORE + unsigned char present = 1; + size_t len = unw_page_size; + unw_word_t addr = unw_page_start((unw_word_t)&present); + unsigned char mvec[1]; + int ret; + do + { + ret = mincore ((void*)addr, len, mvec); + } + while (ret == -1 && errno == EAGAIN); + if (ret == 0) + { + Debug(1, "using mincore to validate memory\n"); + _mem_validate_func = _mincore_validate; + } + else +#endif + { + Debug(1, "using msync to validate memory\n"); + _mem_validate_func = _msync_validate; + } + _unw_address_validator_initialized = ~0; +} + /* Cache of already validated addresses */ enum { NLGA = 4 }; @@ -149,99 +184,90 @@ enum { NLGA = 4 }; // thread-local variant static _Thread_local unw_word_t last_good_addr[NLGA]; static _Thread_local int lga_victim; - - static bool -_is_cached_valid_mem(unw_word_t page_addr) +_is_cached_valid_mem(unw_word_t addr) { + addr = unw_page_start (addr); int i; for (i = 0; i < NLGA; i++) { - if (page_addr == last_good_addr[i]) + if (addr == last_good_addr[i]) return true; } return false; } - - static void -_cache_valid_mem(unw_word_t page_addr) +_cache_valid_mem(unw_word_t addr) { + addr = unw_page_start (addr); int i, victim; victim = lga_victim; for (i = 0; i < NLGA; i++) { if (last_good_addr[victim] == 0) { - last_good_addr[victim] = page_addr; + last_good_addr[victim] = addr; return; } victim = (victim + 1) % NLGA; } - /* All slots full. Evict the victim. */ - last_good_addr[victim] = page_addr; + last_good_addr[victim] = addr; victim = (victim + 1) % NLGA; lga_victim = victim; } - #else // global, thread safe variant static _Atomic unw_word_t last_good_addr[NLGA]; static _Atomic int lga_victim; - - static bool -_is_cached_valid_mem(unw_word_t page_addr) +_is_cached_valid_mem(unw_word_t addr) { int i; + addr = unw_page_start (addr); for (i = 0; i < NLGA; i++) { - if (page_addr == atomic_load(&last_good_addr[i])) + if (addr == atomic_load(&last_good_addr[i])) return true; } return false; } - - /** * Adds a known-valid page address to the cache. * * This implementation is racy as all get-out but the worst case is that cached * address get lost, forcing extra unnecessary validation checks. All of the - * atomic operations don't matter because of TOCTOU races. + * atomic operatrions don't matter because of TOCTOU races. */ static void -_cache_valid_mem(unw_word_t page_addr) +_cache_valid_mem(unw_word_t addr) { + int i, victim; + victim = atomic_load(&lga_victim); unw_word_t zero = 0; - int victim = atomic_load(&lga_victim); - for (int i = 0; i < NLGA; i++) + addr = unw_page_start (addr); + for (i = 0; i < NLGA; i++) { - if (atomic_compare_exchange_strong(&last_good_addr[victim], &zero, page_addr)) + if (atomic_compare_exchange_strong(&last_good_addr[victim], &zero, addr)) { return; } victim = (victim + 1) % NLGA; } - /* All slots full. Evict the victim. */ - atomic_store(&last_good_addr[victim], page_addr); + atomic_store(&last_good_addr[victim], addr); victim = (victim + 1) % NLGA; atomic_store(&lga_victim, victim); } #endif - - /** * Validate an address is readable - * @param[in] addr The (starting) address of the memory range to validate - * @param[in] len The size of the memory range to validate in bytes + * @param[in] addr The (starting) address of the memory to validate + * @param[in] len The size of the memory to validate in bytes * - * Validates the memory range from @p addr to (@p addr + @p len - 1) is - * readable. Since the granularity of memory readability is the page, only one - * byte needs to be validated per page for each page starting at @p addr and - * encompassing @p len bytes. Only the first address of each page is checked. + * Validates the memory at address @p addr is readable. Since the granularity of + * memory readability is the page, only one byte needs to be validated per page + * for each page starting at @p addr and encompassing @p len bytes. * * @returns true if the memory is readable, false otherwise. */ @@ -250,53 +276,46 @@ unw_address_is_valid(unw_word_t addr, size_t len) { if (len == 0) return true; - - /* - * Find the starting address of the page containing the start of the range. - */ - unw_word_t start_page_addr = unw_page_start (addr); - - /* - * Bounds check on bottom of memory: first page is always deemed inaccessible. - * This is potentially incorrect on an embedded system, especially one running - * on bare metal with no VMM, but the check has always been here and no one - * has complained. - */ - if (start_page_addr == 0) - return false; - - /* - * Bounds check on top of memory. Unsigned wraparound could be hazardous. - */ - if (addr > (UNW_WORD_MAX - len - unw_page_size)) + if (unw_page_start (addr) == 0) return false; /* - * Find the starting address of the page containing the end of the range. + * First time through initialize everything: once case if linked with pthreads + * and another when pthreads are not linked (which assumes the single-threaded + * case). + * + * There is a potential race condition in the second case if multiple signals + * are raised at exactly the same time but the worst case is that several + * unnecessary validations get done. */ - unw_word_t end_page_addr = unw_page_start (addr + (len - 1)) + unw_page_size; + if (likely (pthread_once != NULL)) + { + pthread_once (&_unw_address_validator_init_once, _unw_address_validator_init); + } + else if (unlikely (_unw_address_validator_initialized == 0)) + { + _unw_address_validator_init(); + } - /* - * Step through each page and check if the first address in each is readable. - * The first non-readable page encountered means none of them in the given - * range can be considered readable. - */ - for (unw_word_t page_addr = start_page_addr; - page_addr < end_page_addr; - page_addr += unw_page_size) + unw_word_t lastbyte = addr + (len - 1); // highest addressed byte of data to access + while (1) { - if (!_is_cached_valid_mem(page_addr)) + if (!_is_cached_valid_mem(addr)) { - if (!_write_validate (page_addr)) + if (!_mem_validate_func (addr, len)) { Debug(1, "returning false\n"); return false; } - _cache_valid_mem(page_addr); + _cache_valid_mem(addr); } + // If we're still on the same page, we're done. + size_t stride = len-1 < (size_t) unw_page_size ? len-1 : (size_t) unw_page_size; + len -= stride; + addr += stride; + if (unw_page_start (addr) == unw_page_start (lastbyte)) + break; } - return true; } - #endif /* !UNW_REMOTE_ONLY */