From 09385c935af54e8ed9993896df129ed037a513c8 Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Mon, 23 Sep 2019 09:03:10 -0700 Subject: [PATCH 01/16] Stop executing if skip occurs. Bug: 141358530 Test: Forced a skip and verified it registers as a skip. Change-Id: I9915c67ebae4389a26f28e16375ad4a41f3e4837 (cherry picked from commit 103b998a52fa80eee2c85a420533bf75a0b11e41) --- tests/sys_ptrace_test.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/sys_ptrace_test.cpp b/tests/sys_ptrace_test.cpp index 90539fe8c..15e9a2491 100644 --- a/tests/sys_ptrace_test.cpp +++ b/tests/sys_ptrace_test.cpp @@ -176,6 +176,9 @@ static void run_watchpoint_test(std::function child_func, size_t offse ASSERT_EQ(SIGSTOP, WSTOPSIG(status)) << "Status was: " << status; check_hw_feature_supported(child, HwFeature::Watchpoint); + if (::testing::Test::IsSkipped()) { + return; + } set_watchpoint(child, uintptr_t(untag_address(&data)) + offset, size); @@ -224,6 +227,10 @@ TEST(sys_ptrace, watchpoint_stress) { if (!CPU_ISSET(cpu, &available_cpus)) continue; run_watchpoint_stress(cpu); + if (::testing::Test::IsSkipped()) { + // Only check first case, since all others would skip for same reason. + return; + } run_watchpoint_stress(cpu); run_watchpoint_stress(cpu); #if defined(__LP64__) @@ -343,6 +350,9 @@ TEST(sys_ptrace, hardware_breakpoint) { ASSERT_EQ(SIGSTOP, WSTOPSIG(status)) << "Status was: " << status; check_hw_feature_supported(child, HwFeature::Breakpoint); + if (::testing::Test::IsSkipped()) { + return; + } set_breakpoint(child); From ea0c61e7045eeb691503a6a077dc223f8a8da711 Mon Sep 17 00:00:00 2001 From: Dmytro Chystiakov Date: Fri, 29 Mar 2019 13:49:12 -0700 Subject: [PATCH 02/16] Fix Bionic dlfcn.dladdr_libc test case for 64bit binary translations 64bit libraries should be located in /system/lib64 directory instead of /system/lib for platforms with emulated arhitecture. This patch updated ALTERNATE_PATH_TO_SYSTEM_LIB for 64bit values Test: run cts -m CtsBionicTestCases -t dlfcn.dladdr_libc Bug: 129389761 Change-Id: I29d6d36d15d7e61818c7ed1cfd0786745d9ae6a2 Signed-off-by: Dmytro Chystiakov (cherry picked from commit e712cd185f34dd154b4672abae76eae262a03c65) --- tests/dlfcn_test.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/dlfcn_test.cpp b/tests/dlfcn_test.cpp index f3be9883d..c6d7ddba1 100644 --- a/tests/dlfcn_test.cpp +++ b/tests/dlfcn_test.cpp @@ -982,16 +982,16 @@ TEST(dlfcn, dlopen_executable_by_absolute_path) { } #if defined (__aarch64__) -#define ALTERNATE_PATH_TO_SYSTEM_LIB "/system/lib/arm64/" +#define ALTERNATE_PATH_TO_SYSTEM_LIB "/system/lib64/arm64/" #elif defined (__arm__) #define ALTERNATE_PATH_TO_SYSTEM_LIB "/system/lib/arm/" #elif defined (__i386__) #define ALTERNATE_PATH_TO_SYSTEM_LIB "/system/lib/x86/" #elif defined (__x86_64__) -#define ALTERNATE_PATH_TO_SYSTEM_LIB "/system/lib/x86_64/" +#define ALTERNATE_PATH_TO_SYSTEM_LIB "/system/lib64/x86_64/" #elif defined (__mips__) #if defined(__LP64__) -#define ALTERNATE_PATH_TO_SYSTEM_LIB "/system/lib/mips64/" +#define ALTERNATE_PATH_TO_SYSTEM_LIB "/system/lib64/mips64/" #else #define ALTERNATE_PATH_TO_SYSTEM_LIB "/system/lib/mips/" #endif From e58efb84c5b21b55ca28d8aeeeff9c41424c1a34 Mon Sep 17 00:00:00 2001 From: Dmytro Chystiakov Date: Tue, 1 Oct 2019 11:28:49 -0700 Subject: [PATCH 03/16] Fix linker path for emulated architecture dl#exec_linker* tests are failing on devices with emulated architecture due to hardcoded path to linker. Test: bionic-unit-tests --gtest_filter=dl.exec_linker* Bug: b/141914915 Change-Id: Id6d8d3ee7114e70b07e44034aa62dce0a3e0760e Signed-off-by: Dmytro Chystiakov (cherry picked from commit 595c38184120e286670aa1ae60b6ae35c62147b7) --- tests/dl_test.cpp | 51 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 9 deletions(-) diff --git a/tests/dl_test.cpp b/tests/dl_test.cpp index 9e463948a..c1cd6949f 100644 --- a/tests/dl_test.cpp +++ b/tests/dl_test.cpp @@ -25,6 +25,7 @@ #include #include #include +#include #include #include @@ -85,20 +86,50 @@ TEST(dl, lib_does_not_preempt_global_protected) { #else static constexpr const char* kPathToLinker = "/system/bin/linker"; #endif + +#if defined (__aarch64__) + static constexpr const char* kAlternatePathToLinker = "/system/bin/arm64/linker64"; +#elif defined (__arm__) + static constexpr const char* kAlternatePathToLinker = "/system/bin/arm/linker"; +#elif defined (__x86_64__) + static constexpr const char* kAlternatePathToLinker = "/system/bin/x86_64/linker64"; +#elif defined (__i386__) + static constexpr const char* kAlternatePathToLinker = "/system/bin/x86/linker"; +#elif defined (__mips__) +#if defined(__LP64__) + static constexpr const char* kAlternatePathToLinker = "/system/bin/mips64/linker64"; +#else + static constexpr const char* kAlternatePathToLinker = "/system/bin/mips/linker"; #endif +#else +#error "Unknown architecture" +#endif + +const char* PathToLinker() { + // On the systems with emulated architecture linker would be of different + // architecture. Try to use alternate paths first. + struct stat buffer; + if (stat(kAlternatePathToLinker, &buffer) == 0) { + return kAlternatePathToLinker; + } + return kPathToLinker; +} +#endif // defined(__BIONIC__) TEST(dl, exec_linker) { #if defined(__BIONIC__) - std::string usage_prefix = std::string("Usage: ") + kPathToLinker; + const char* path_to_linker = PathToLinker(); + std::string usage_prefix = std::string("Usage: ") + path_to_linker; ExecTestHelper eth; - eth.SetArgs({ kPathToLinker, nullptr }); - eth.Run([&]() { execve(kPathToLinker, eth.GetArgs(), eth.GetEnv()); }, 0, nullptr); + eth.SetArgs({ path_to_linker, nullptr }); + eth.Run([&]() { execve(path_to_linker, eth.GetArgs(), eth.GetEnv()); }, 0, nullptr); ASSERT_EQ(0u, eth.GetOutput().find(usage_prefix)) << "Test output:\n" << eth.GetOutput(); #endif } TEST(dl, exec_linker_load_file) { #if defined(__BIONIC__) + const char* path_to_linker = PathToLinker(); std::string helper = GetTestlibRoot() + "/exec_linker_helper/exec_linker_helper"; std::string expected_output = @@ -107,13 +138,14 @@ TEST(dl, exec_linker_load_file) { "__progname=" + helper + "\n" + "helper_func called\n"; ExecTestHelper eth; - eth.SetArgs({ kPathToLinker, helper.c_str(), nullptr }); - eth.Run([&]() { execve(kPathToLinker, eth.GetArgs(), eth.GetEnv()); }, 0, expected_output.c_str()); + eth.SetArgs({ path_to_linker, helper.c_str(), nullptr }); + eth.Run([&]() { execve(path_to_linker, eth.GetArgs(), eth.GetEnv()); }, 0, expected_output.c_str()); #endif } TEST(dl, exec_linker_load_from_zip) { #if defined(__BIONIC__) + const char* path_to_linker = PathToLinker(); std::string helper = GetTestlibRoot() + "/libdlext_test_zip/libdlext_test_zip_zipaligned.zip!/libdir/exec_linker_helper"; std::string expected_output = @@ -122,17 +154,18 @@ TEST(dl, exec_linker_load_from_zip) { "__progname=" + helper + "\n" + "helper_func called\n"; ExecTestHelper eth; - eth.SetArgs({ kPathToLinker, helper.c_str(), nullptr }); - eth.Run([&]() { execve(kPathToLinker, eth.GetArgs(), eth.GetEnv()); }, 0, expected_output.c_str()); + eth.SetArgs({ path_to_linker, helper.c_str(), nullptr }); + eth.Run([&]() { execve(path_to_linker, eth.GetArgs(), eth.GetEnv()); }, 0, expected_output.c_str()); #endif } TEST(dl, exec_linker_load_self) { #if defined(__BIONIC__) + const char* path_to_linker = PathToLinker(); std::string error_message = "error: linker cannot load itself\n"; ExecTestHelper eth; - eth.SetArgs({ kPathToLinker, kPathToLinker, nullptr }); - eth.Run([&]() { execve(kPathToLinker, eth.GetArgs(), eth.GetEnv()); }, EXIT_FAILURE, error_message.c_str()); + eth.SetArgs({ path_to_linker, path_to_linker, nullptr }); + eth.Run([&]() { execve(path_to_linker, eth.GetArgs(), eth.GetEnv()); }, EXIT_FAILURE, error_message.c_str()); #endif } From ac7ffe344e6aa5b96c47a3ba2388b71f19abb955 Mon Sep 17 00:00:00 2001 From: Evgenii Stepanov Date: Mon, 20 Jul 2020 15:52:06 -0700 Subject: [PATCH 04/16] Remove stack address check in cfi_basic test. This is not actually a property that is guaranteed by the bionic implementation of CFI shadow. Since the gaps between libraries are not completely inaccessible, it is possible for a stack mapping to sneak in, which would cause the callback to register in the test library. This is not a correctness issue in CFI as the actual __cfi_check callback will reject such address anyway, at a small CPU cost. Bug: 156218352 Test: CtsBionicTestCases-cfi_test#basic Change-Id: I8d04fb7132e1eac2a8abfbc48a37c8eac6e25a09 (cherry picked from commit 0f6b504e0c682e7c3d377416dacfe57b6c59f1b8) --- tests/cfi_test.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/cfi_test.cpp b/tests/cfi_test.cpp index e155e1a84..b4522ec8b 100644 --- a/tests/cfi_test.cpp +++ b/tests/cfi_test.cpp @@ -85,9 +85,6 @@ TEST(cfi_test, basic) { EXPECT_EQ(get_global_address(), get_last_address()); EXPECT_EQ(c, get_count()); - // CFI check for a stack address. This is always invalid and gets the process killed. - EXPECT_DEATH(__cfi_slowpath(45, reinterpret_cast(&c)), ""); - // CFI check for a heap address. This is always invalid and gets the process killed. void* p = malloc(4096); EXPECT_DEATH(__cfi_slowpath(46, p), ""); From 47df89fcfb21db19bbb6cfc416903e6b4127b102 Mon Sep 17 00:00:00 2001 From: Evgeny Eltsin Date: Mon, 8 Jun 2020 21:12:56 +0200 Subject: [PATCH 05/16] Add util to skip tests with native_bridge Bug: 37920774 Bug: 157394871 Test: bionic-unit-tests Change-Id: Id949c9e568fd068daaf405a377813ee1480c2df7 (cherry picked from commit b56d1182d1067e70910584cf0c8fd797a262d82a) --- tests/utils.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/utils.h b/tests/utils.h index 5014ef743..34283a0b4 100644 --- a/tests/utils.h +++ b/tests/utils.h @@ -25,6 +25,10 @@ #include #include +#if defined(__BIONIC__) +#include +#endif + #include #include #include @@ -66,6 +70,21 @@ static inline bool running_with_hwasan() { #define SKIP_WITH_HWASAN if (running_with_hwasan()) GTEST_SKIP() +static inline bool running_with_native_bridge() { +#if defined(__BIONIC__) +#if defined(__arm__) + static const prop_info* pi = __system_property_find("ro.dalvik.vm.isa.arm"); + return pi != nullptr; +#elif defined(__aarch64__) + static const prop_info* pi = __system_property_find("ro.dalvik.vm.isa.arm64"); + return pi != nullptr; +#endif +#endif + return false; +} + +#define SKIP_WITH_NATIVE_BRIDGE if (running_with_native_bridge()) GTEST_SKIP() + static inline void* untag_address(void* addr) { #if defined(__LP64__) constexpr uintptr_t mask = (static_cast(1) << 56) - 1; From 55c315335da754c540fae82e7ea44e65a4d424fe Mon Sep 17 00:00:00 2001 From: Evgeny Eltsin Date: Tue, 9 Jun 2020 15:49:20 +0200 Subject: [PATCH 06/16] Skip pthread.pthread_create__mmap_failures with native_bridge The test reserves all memory but the minimum required to create a thread. However, after the thread is created, native_bridge needs more memory to translate and run the thread function. This might be prevented by native_bridge preallocating a memory buffer to be used for translation. But, first, this complication seems to be needed just for this kind of tests, and, second, it is pretty flaky regarding changes both in native_bridge and bionic. Looks better to disable this test with native_bridge. Bug: 67745607 Bug: 148608153 Bug: 157394871 Test: bionic-unit-tests --gtest_filter=pthread.pthread_create__mmap_failures Change-Id: I42ce2b5a01a7d9f10d952a5fc7b75d51fa89072a (cherry picked from commit b4f7aaac5cdda45ff0d9dc58e1fd2d727601f619) --- tests/pthread_test.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/pthread_test.cpp b/tests/pthread_test.cpp index d8257381c..851b86f50 100644 --- a/tests/pthread_test.cpp +++ b/tests/pthread_test.cpp @@ -2821,6 +2821,9 @@ TEST(pthread, pthread_attr_getdetachstate__pthread_attr_setdetachstate) { } TEST(pthread, pthread_create__mmap_failures) { + // After thread is successfully created, native_bridge might need more memory to run it. + SKIP_WITH_NATIVE_BRIDGE; + pthread_attr_t attr; ASSERT_EQ(0, pthread_attr_init(&attr)); ASSERT_EQ(0, pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED)); From 1d1c4f9e259e2d095ceafe9e2a222f3ba466db89 Mon Sep 17 00:00:00 2001 From: Evgeny Eltsin Date: Tue, 9 Jun 2020 15:49:20 +0200 Subject: [PATCH 07/16] Skip android_unsafe_frame_pointer_chase.pthread with native bridge Bug: 167968941 Bug: 161082441 Test: bionic-unit-tests --gtest_filter=android_unsafe_frame_pointer_chase.pthread Change-Id: I42a8121003be2fbcd1486b0d5281bc60ac67eb22 --- tests/Android.bp | 3 +++ tests/android_unsafe_frame_pointer_chase_test.cpp | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/tests/Android.bp b/tests/Android.bp index 8b1eebc3f..f215e602f 100644 --- a/tests/Android.bp +++ b/tests/Android.bp @@ -308,6 +308,9 @@ cc_test_library { cflags: [ "-fno-omit-frame-pointer", ], + static_libs: [ + "libbase", + ], } // ----------------------------------------------------------------------------- diff --git a/tests/android_unsafe_frame_pointer_chase_test.cpp b/tests/android_unsafe_frame_pointer_chase_test.cpp index 7fa50e149..5b436ff81 100644 --- a/tests/android_unsafe_frame_pointer_chase_test.cpp +++ b/tests/android_unsafe_frame_pointer_chase_test.cpp @@ -22,6 +22,8 @@ #include "platform/bionic/android_unsafe_frame_pointer_chase.h" +#include "utils.h" + // Prevent tail calls inside recurse. __attribute__((weak, noinline)) size_t nop(size_t val) { return val; @@ -94,6 +96,10 @@ static void* BacktraceThread(void*) { } TEST(android_unsafe_frame_pointer_chase, pthread) { + // Android11 missed the fix (r.android.com/1382566) so disabling the test. + // It will be re-enabled in the next releases. + SKIP_WITH_NATIVE_BRIDGE; + pthread_t t; ASSERT_EQ(0, pthread_create(&t, nullptr, BacktraceThread, nullptr)); void* retval; From 46dcc0785c61a79db9c19bc22b3b15073e885c60 Mon Sep 17 00:00:00 2001 From: "pyung.lee" Date: Tue, 6 Oct 2020 16:20:16 +0900 Subject: [PATCH 08/16] Fix possible issue with cfi_basic test. It's possible for malloc to return a pointer that is not going to crash with __cfi_slowpath. It's possible to modify the cfi code to avoid this problem, but I'm not convinced that this will be any better at catching problems. So I'm just modifying the test so that it will eventually allocate a pointer that does not overlap. This previous version of the test failed on jemalloc svelte config, but there is nothing that would not result in a failure on scudo leading to a failure every once in a while. Bug: 142556796 Bug: 140079007 Test: cts -m CtsBionicTestCases Change-Id: Ibf12a286c411e0bdc9f81589f2f66fd0ccd7f07a (cherrypick of f322483b3f8a6e05db5c3fc7974601032a03ba0a) --- tests/cfi_test.cpp | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/tests/cfi_test.cpp b/tests/cfi_test.cpp index b4522ec8b..e0ae3afc7 100644 --- a/tests/cfi_test.cpp +++ b/tests/cfi_test.cpp @@ -18,6 +18,8 @@ #include #include +#include + #include "BionicDeathTest.h" #include "gtest_globals.h" #include "utils.h" @@ -35,6 +37,14 @@ size_t __cfi_shadow_size(); static void f() {} +static void test_cfi_slowpath_with_alloc() { + std::vector allocs; + for (size_t i = 0; i < 1000; i++) { + allocs.push_back(malloc(4096)); + __cfi_slowpath(46, allocs.back()); + } +} + TEST(cfi_test, basic) { #if defined(__BIONIC__) void* handle; @@ -85,10 +95,11 @@ TEST(cfi_test, basic) { EXPECT_EQ(get_global_address(), get_last_address()); EXPECT_EQ(c, get_count()); - // CFI check for a heap address. This is always invalid and gets the process killed. - void* p = malloc(4096); - EXPECT_DEATH(__cfi_slowpath(46, p), ""); - free(p); + // CFI check for a heap address. + // It's possible that this allocation could wind up in the same CFI granule as + // an unchecked library, which means the below might not crash. To force a + // crash keep allocating up to a max until there is a crash. + EXPECT_DEATH(test_cfi_slowpath_with_alloc(), ""); // Check all the addresses. const size_t bss_size = 1024 * 1024; From a38477d03d94396702596f288581eb18117c8c1c Mon Sep 17 00:00:00 2001 From: Evgeny Eltsin Date: Mon, 22 Jun 2020 18:49:16 +0200 Subject: [PATCH 09/16] Skip MTE tests with native_bridge With native_bridge, native and emulated parts exchange data, including pointers. If tagging on native architecture is different from tagging on emulated architecture, all the pointers in the data exchange must be identified and marshalled, which is hardly feasible. Disable MTE tests with native_bridge. Bug: 135772972 Bug: 159352723 Test: bionic-unit-tests --gtest_filter=*mte* Change-Id: Icba90636173e9e71036def5302c7d0a09dd8873b (cherry picked from commit ed51fb9a078aa57bc0e8a1c8a71f69275f00edf9) --- tests/mte_test.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/mte_test.cpp b/tests/mte_test.cpp index 8928805e5..79ee0f808 100644 --- a/tests/mte_test.cpp +++ b/tests/mte_test.cpp @@ -18,6 +18,7 @@ #include #include +#include "utils.h" __attribute__((no_sanitize("hwaddress"))) static void test_tag_mismatch() { @@ -44,5 +45,10 @@ static void test_tag_mismatch() { } TEST(mte_test, ScopedDisableMTE) { + // With native_bridge, native and emulated parts exchange data, including pointers. + // This implies tagging on native and emulated architectures should match, which is + // not the case at the moment. + SKIP_WITH_NATIVE_BRIDGE; + test_tag_mismatch(); } From ee192afe81a1dcce597a810970854c46719e0bc3 Mon Sep 17 00:00:00 2001 From: Evgeny Eltsin Date: Mon, 8 Jun 2020 21:19:12 +0200 Subject: [PATCH 10/16] Skip pthread_leak* tests with native_bridge Bug: 37920774 Bug: 157394871 Test: bionic-unit-tests --gtest_filter=*leak* Change-Id: Ifc5b66e4b640d1abae4dcf8dbc28612d24c7e972 (cherry picked from commit 45b36c2921c8d5986820d4c07864c7366ad7efd6) --- tests/leak_test.cpp | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/tests/leak_test.cpp b/tests/leak_test.cpp index e0a3d5772..4ebf41f59 100644 --- a/tests/leak_test.cpp +++ b/tests/leak_test.cpp @@ -109,23 +109,21 @@ std::ostream& operator<<(std::ostream& os, const LeakChecker& lc) { // http://b/36045112 TEST(pthread_leak, join) { + SKIP_WITH_NATIVE_BRIDGE; // http://b/37920774 + LeakChecker lc; - for (size_t pass = 0; pass < 2; ++pass) { - for (int i = 0; i < 100; ++i) { - pthread_t thread; - ASSERT_EQ(0, pthread_create(&thread, nullptr, [](void*) -> void* { return nullptr; }, nullptr)); - ASSERT_EQ(0, pthread_join(thread, nullptr)); - } - - // A native bridge implementation might need a warm up pass to reach a steady state. - // http://b/37920774. - if (pass == 0) lc.Reset(); + for (int i = 0; i < 100; ++i) { + pthread_t thread; + ASSERT_EQ(0, pthread_create(&thread, nullptr, [](void*) -> void* { return nullptr; }, nullptr)); + ASSERT_EQ(0, pthread_join(thread, nullptr)); } } // http://b/36045112 TEST(pthread_leak, detach) { + SKIP_WITH_NATIVE_BRIDGE; // http://b/37920774 + LeakChecker lc; // Ancient devices with only 2 cores need a lower limit. @@ -158,8 +156,7 @@ TEST(pthread_leak, detach) { WaitUntilAllThreadsExited(tids, thread_count); - // A native bridge implementation might need a warm up pass to reach a steady state. - // http://b/37920774. + // TODO(b/158573595): the test is flaky without the warmup pass. if (pass == 0) lc.Reset(); } } From 069cf41429fe8eab0f7412182f02ae6e0852ef7b Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Tue, 8 Jun 2021 15:33:22 -0700 Subject: [PATCH 11/16] Skip the malloc_slack test on native bridge. The allocator for the native bridge is not necessarily going to allocate the slack data. Bug: 202428612 Test: Ran on non-native bridge and verified test isn't skipped. Test: Ran on native bridge and verified test is skipped. Change-Id: Ia1555be0e9f55896af7ca81830605367133c44a1 (cherry picked from commit 7c0ce86a00433159dfc5dc7164feb752d094dff4) --- tests/malloc_test.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/malloc_test.cpp b/tests/malloc_test.cpp index d73f2436d..30da5c32f 100644 --- a/tests/malloc_test.cpp +++ b/tests/malloc_test.cpp @@ -1355,6 +1355,8 @@ TEST(malloc, disable_mte) { TEST(malloc, allocation_slack) { #if defined(__BIONIC__) + SKIP_WITH_NATIVE_BRIDGE; // http://b/189606147 + bool allocator_scudo; GetAllocatorVersion(&allocator_scudo); if (!allocator_scudo) { From a0d8367d705afa56dbd098b8448a32e9073beaa8 Mon Sep 17 00:00:00 2001 From: Evgenii Stepanov Date: Tue, 16 Nov 2021 17:34:39 -0800 Subject: [PATCH 12/16] Regression test for scudo crash in resizeTaggedChunk. This is a copy of the upstream scudo test for CTS: https://reviews.llvm.org/rG913d78c40c37c9c3428285d868ce454b058e40f3 Bug: 206701345 Test: CtsBionicTestCases Change-Id: I76b6b33c0665d7ad3bdd8c07d39a39d0d24d94df (cherry picked from commit f0d7a34e257494c7a0248e399849bb714bb9fcc4) --- tests/malloc_test.cpp | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/malloc_test.cpp b/tests/malloc_test.cpp index 30da5c32f..e5e78a5b4 100644 --- a/tests/malloc_test.cpp +++ b/tests/malloc_test.cpp @@ -1373,3 +1373,25 @@ TEST(malloc, allocation_slack) { GTEST_SKIP() << "bionic extension"; #endif } + +// Regression test for b/206701345 -- scudo bug, MTE only. +// Fix: https://reviews.llvm.org/D105261 +// Fix: https://android-review.googlesource.com/c/platform/external/scudo/+/1763655 +TEST(malloc, realloc_mte_crash_b206701345) { + // We want to hit in-place realloc at the very end of an mmap-ed region. Not + // all size classes allow such placement - mmap size has to be divisible by + // the block size. At the time of writing this could only be reproduced with + // 64 byte size class (i.e. 48 byte allocations), but that may change in the + // future. Try several different classes at the lower end. + std::vector ptrs(10000); + for (int i = 1; i < 32; ++i) { + size_t sz = 16 * i - 1; + for (void*& p : ptrs) { + p = realloc(malloc(sz), sz + 1); + } + + for (void* p : ptrs) { + free(p); + } + } +} From 95577defec5556d36f5940e076a1922b8fa9335e Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Fri, 1 Apr 2022 11:12:36 -0700 Subject: [PATCH 13/16] Fix unistd.exec_argv0_null for new kernels. There are other options here (see the code comment for details), but this is the least effort/least disruptive for now. Bug: 228898932, 227498625 Test: treehugger Change-Id: I33be6fbfc022238de2f1846a69af1e712a9d6391 (cherry picked from commit bb1cc5a82c8e19d5f4231988ba48ce3de43ff3ed) --- tests/unistd_test.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/tests/unistd_test.cpp b/tests/unistd_test.cpp index 10c1710bd..38046ff14 100644 --- a/tests/unistd_test.cpp +++ b/tests/unistd_test.cpp @@ -1425,11 +1425,21 @@ TEST(UNISTD_TEST, execvp_libcore_test_55017) { } TEST(UNISTD_TEST, exec_argv0_null) { - // http://b/33276926 + // http://b/33276926 and http://b/227498625. + // + // With old kernels, bionic will see the null pointer and use "" but + // with new (5.18+) kernels, the kernel will already have substituted the + // empty string, so we don't make any assertion here about what (if anything) + // comes before the first ':'. + // + // If this ever causes trouble, we could change bionic to replace _either_ the + // null pointer or the empty string. We could also use the actual name from + // readlink() on /proc/self/exe if we ever had reason to disallow programs + // from trying to hide like this. char* args[] = {nullptr}; char* envs[] = {nullptr}; ASSERT_EXIT(execve("/system/bin/run-as", args, envs), testing::ExitedWithCode(1), - ": usage: run-as"); + ": usage: run-as"); } TEST(UNISTD_TEST, fexecve_failure) { From 92d178ace0170fb47613fde1d29bda46eae9b660 Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Fri, 1 Apr 2022 11:12:36 -0700 Subject: [PATCH 14/16] Fix unistd.exec_argv0_null for new kernels. There are other options here (see the code comment for details), but this is the least effort/least disruptive for now. Bug: 228898932, 227498625 Test: treehugger Change-Id: I33be6fbfc022238de2f1846a69af1e712a9d6391 (cherry picked from commit bb1cc5a82c8e19d5f4231988ba48ce3de43ff3ed) --- tests/unistd_test.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/tests/unistd_test.cpp b/tests/unistd_test.cpp index 6b2856169..23bd35aa2 100644 --- a/tests/unistd_test.cpp +++ b/tests/unistd_test.cpp @@ -1500,11 +1500,21 @@ TEST(UNISTD_TEST, execvp_libcore_test_55017) { } TEST(UNISTD_TEST, exec_argv0_null) { - // http://b/33276926 + // http://b/33276926 and http://b/227498625. + // + // With old kernels, bionic will see the null pointer and use "" but + // with new (5.18+) kernels, the kernel will already have substituted the + // empty string, so we don't make any assertion here about what (if anything) + // comes before the first ':'. + // + // If this ever causes trouble, we could change bionic to replace _either_ the + // null pointer or the empty string. We could also use the actual name from + // readlink() on /proc/self/exe if we ever had reason to disallow programs + // from trying to hide like this. char* args[] = {nullptr}; char* envs[] = {nullptr}; ASSERT_EXIT(execve("/system/bin/run-as", args, envs), testing::ExitedWithCode(1), - ": usage: run-as"); + ": usage: run-as"); } TEST(UNISTD_TEST, fexecve_failure) { From 478e63775b13e6bf44979b0938c5e5ad88d0d0e2 Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Fri, 1 Apr 2022 11:12:36 -0700 Subject: [PATCH 15/16] Fix unistd.exec_argv0_null for new kernels. There are other options here (see the code comment for details), but this is the least effort/least disruptive for now. Bug: 228898932, 227498625 Test: treehugger Change-Id: I33be6fbfc022238de2f1846a69af1e712a9d6391 (cherry picked from commit bb1cc5a82c8e19d5f4231988ba48ce3de43ff3ed) --- tests/unistd_test.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/tests/unistd_test.cpp b/tests/unistd_test.cpp index 7d1e6128b..19f58e3f9 100644 --- a/tests/unistd_test.cpp +++ b/tests/unistd_test.cpp @@ -1515,11 +1515,21 @@ TEST(UNISTD_TEST, execvp_libcore_test_55017) { } TEST(UNISTD_TEST, exec_argv0_null) { - // http://b/33276926 + // http://b/33276926 and http://b/227498625. + // + // With old kernels, bionic will see the null pointer and use "" but + // with new (5.18+) kernels, the kernel will already have substituted the + // empty string, so we don't make any assertion here about what (if anything) + // comes before the first ':'. + // + // If this ever causes trouble, we could change bionic to replace _either_ the + // null pointer or the empty string. We could also use the actual name from + // readlink() on /proc/self/exe if we ever had reason to disallow programs + // from trying to hide like this. char* args[] = {nullptr}; char* envs[] = {nullptr}; ASSERT_EXIT(execve("/system/bin/run-as", args, envs), testing::ExitedWithCode(1), - ": usage: run-as"); + ": usage: run-as"); } TEST(UNISTD_TEST, fexecve_failure) { From 1727595b0e06ba8d4cfe2f01e5ebde0313b0b4c1 Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Fri, 13 May 2022 16:06:54 -0700 Subject: [PATCH 16/16] mntent_test: don't assume /proc isn't the first mount. This test tried to be lazy and test both getmntent() and getmntent_r() in the same test, but that led to an implicit assumption that /proc isn't the first mount returned. This new version is quite a bit more thorough than the old. It does assume that the mount list doesn't change while the test is running, but that seems like a reasonable assumption to make during CTS? Bug: https://issuetracker.google.com/230228681 Test: treehugger Change-Id: I5c5f0b86ae1c4df9a2ce69d48e1c3accb42c687b (cherry picked from commit 1e393b0699745d6120d2fd43f58dc3d5863e3b87) --- tests/mntent_test.cpp | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/tests/mntent_test.cpp b/tests/mntent_test.cpp index b86af9fb6..4b8fc9a80 100644 --- a/tests/mntent_test.cpp +++ b/tests/mntent_test.cpp @@ -19,24 +19,40 @@ #include TEST(mntent, mntent_smoke) { + // Read all the entries with getmntent(). FILE* fp = setmntent("/proc/mounts", "r"); ASSERT_TRUE(fp != nullptr); - ASSERT_TRUE(getmntent(fp) != nullptr); + std::vector fsnames; + std::vector dirs; + mntent* me; + while ((me = getmntent(fp)) != nullptr) { + fsnames.push_back(me->mnt_fsname); + dirs.push_back(me->mnt_dir); + } - bool saw_proc = false; + ASSERT_EQ(1, endmntent(fp)); + + // Then again with getmntent_r(), checking they match. + fp = setmntent("/proc/mounts", "r"); + ASSERT_TRUE(fp != nullptr); struct mntent entry; char buf[BUFSIZ]; + size_t i = 0; while (getmntent_r(fp, &entry, buf, sizeof(buf)) != nullptr) { - if (strcmp(entry.mnt_fsname, "proc") == 0 && strcmp(entry.mnt_dir, "/proc") == 0) { - saw_proc = true; - } + ASSERT_EQ(fsnames[i], entry.mnt_fsname); + ASSERT_EQ(dirs[i], entry.mnt_dir); + i++; } - ASSERT_TRUE(saw_proc); - ASSERT_EQ(1, endmntent(fp)); + + // And just for good measure: we did see a /proc entry, right? + auto it = std::find(fsnames.begin(), fsnames.end(), "proc"); + ASSERT_TRUE(it != fsnames.end()); + size_t proc_index = it - fsnames.begin(); + ASSERT_EQ("/proc", dirs[proc_index]); } TEST(mntent, hasmntopt) {