From 79248264f35b5ead4055bc42d7e17f56a58b7baa Mon Sep 17 00:00:00 2001 From: Martin Stjernholm Date: Thu, 19 Oct 2023 21:26:30 +0100 Subject: Avoid dynamic lib dependencies on internal ART libraries in the libartpalette and dex2oat CTS gtests. We cannot use art_standalone_gtest_defaults. dex2oat_cts_test.cc rips out the necessary bits from common_art_test.cc instead, with as few changes as possible. With this change we can reenable art_standalone_dex2oat_cts_tests again (cf. b/288212464#comment5). Merged-In set to a CL in main to avoid merging there - gtests are already linked statically there. Test: m art_standalone_libartpalette_tests \ art_standalone_dex2oat_cts_tests Do `readelf -d` on the test binaries to check that only stable ABI libraries are in the NEEDED list. Test: cts-tradefed run commandAndExit cts \ -m art_standalone_dex2oat_cts_tests on a tm-release image. Test: cts-tradefed run commandAndExit cts \ -m art_standalone_libartpalette_tests on a tm-release image. Bug: 306064090 Bug: 288212464 Change-Id: I1ff4174490ff50dbd2177cf5999fc6ef4bfbaa49 Merged-In: I75a844f53663385ef98351f60d3adb900157f5e5 Merged-In: I4a8d1eddaacf9e476f7d3d783edec0223cb39c04 --- dex2oat/Android.bp | 2 +- dex2oat/dex2oat_cts_test.cc | 238 ++++++++++++++++++++++++++++++++++++- libartpalette/Android.bp | 11 +- libartpalette/apex/palette_test.cc | 6 + test/Android.bp | 37 ++++++ 5 files changed, 287 insertions(+), 7 deletions(-) diff --git a/dex2oat/Android.bp b/dex2oat/Android.bp index 26cbd51459..80ffa51b00 100644 --- a/dex2oat/Android.bp +++ b/dex2oat/Android.bp @@ -589,7 +589,7 @@ art_cc_test { // Counterpart to art_standalone_dex2oat_tests for tests that go into CTS. art_cc_test { name: "art_standalone_dex2oat_cts_tests", - defaults: ["art_standalone_gtest_defaults"], + defaults: ["art_cts_gtest_defaults"], srcs: ["dex2oat_cts_test.cc"], data: [ ":art-gtest-jars-Main", diff --git a/dex2oat/dex2oat_cts_test.cc b/dex2oat/dex2oat_cts_test.cc index 254126446d..25cd94bb93 100644 --- a/dex2oat/dex2oat_cts_test.cc +++ b/dex2oat/dex2oat_cts_test.cc @@ -14,24 +14,252 @@ * limitations under the License. */ +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +#include "android-base/file.h" +#include "android-base/unique_fd.h" #include "base/file_utils.h" -#include "dex2oat_environment_test.h" +#include "base/os.h" +#include "base/stl_util.h" +#include "base/unix_file/fd_file.h" +#include "gtest/gtest.h" namespace art { +namespace { + +void ClearDirectory(const char* dirpath, bool recursive = true) { + ASSERT_TRUE(dirpath != nullptr); + DIR* dir = opendir(dirpath); + ASSERT_TRUE(dir != nullptr); + dirent* e; + struct stat s; + while ((e = readdir(dir)) != nullptr) { + if ((strcmp(e->d_name, ".") == 0) || (strcmp(e->d_name, "..") == 0)) { + continue; + } + std::string filename(dirpath); + filename.push_back('/'); + filename.append(e->d_name); + int stat_result = lstat(filename.c_str(), &s); + ASSERT_EQ(0, stat_result) << "unable to stat " << filename; + if (S_ISDIR(s.st_mode)) { + if (recursive) { + ClearDirectory(filename.c_str()); + int rmdir_result = rmdir(filename.c_str()); + ASSERT_EQ(0, rmdir_result) << filename; + } + } else { + int unlink_result = unlink(filename.c_str()); + ASSERT_EQ(0, unlink_result) << filename; + } + } + closedir(dir); +} + +class Dex2oatScratchDirs { + public: + void SetUp(const std::string& android_data) { + // Create a scratch directory to work from. + + // Get the realpath of the android data. The oat dir should always point to real location + // when generating oat files in dalvik-cache. This avoids complicating the unit tests + // when matching the expected paths. + UniqueCPtr android_data_real(realpath(android_data.c_str(), nullptr)); + ASSERT_TRUE(android_data_real != nullptr) + << "Could not get the realpath of the android data" << android_data << strerror(errno); + + scratch_dir_.assign(android_data_real.get()); + scratch_dir_ += "/Dex2oatEnvironmentTest"; + ASSERT_EQ(0, mkdir(scratch_dir_.c_str(), 0700)); + + // Create a subdirectory in scratch for odex files. + odex_oat_dir_ = scratch_dir_ + "/oat"; + ASSERT_EQ(0, mkdir(odex_oat_dir_.c_str(), 0700)); + + odex_dir_ = odex_oat_dir_ + "/" + std::string(GetInstructionSetString(kRuntimeISA)); + ASSERT_EQ(0, mkdir(odex_dir_.c_str(), 0700)); + } + + void TearDown() { + ClearDirectory(odex_dir_.c_str()); + ASSERT_EQ(0, rmdir(odex_dir_.c_str())); -class Dex2oatCtsTest : public CommonArtTest, public Dex2oatScratchDirs { + ClearDirectory(odex_oat_dir_.c_str()); + ASSERT_EQ(0, rmdir(odex_oat_dir_.c_str())); + + ClearDirectory(scratch_dir_.c_str()); + ASSERT_EQ(0, rmdir(scratch_dir_.c_str())); + } + + // Scratch directory, for dex and odex files (oat files will go in the + // dalvik cache). + const std::string& GetScratchDir() const { return scratch_dir_; } + + // Odex directory is the subdirectory in the scratch directory where odex + // files should be located. + const std::string& GetOdexDir() const { return odex_dir_; } + + private: + std::string scratch_dir_; + std::string odex_oat_dir_; + std::string odex_dir_; +}; + +class Dex2oatCtsTest : public Dex2oatScratchDirs, public testing::Test { public: + void SetUpAndroidDataDir(std::string& android_data) { + android_data = "/data/local/tmp"; + android_data += "/art-data-XXXXXX"; + if (mkdtemp(&android_data[0]) == nullptr) { + PLOG(FATAL) << "mkdtemp(\"" << &android_data[0] << "\") failed"; + } + setenv("ANDROID_DATA", android_data.c_str(), 1); + } + + void TearDownAndroidDataDir(const std::string& android_data, + bool fail_on_error) { + if (fail_on_error) { + ASSERT_EQ(rmdir(android_data.c_str()), 0); + } else { + rmdir(android_data.c_str()); + } + } + + std::string GetTestDexFileName(const char* name) const { + CHECK(name != nullptr); + // The needed jar files for gtest are located next to the gtest binary itself. + std::string executable_dir = android::base::GetExecutableDirectory(); + for (auto ext : {".jar", ".dex"}) { + std::string path = executable_dir + "/art-gtest-jars-" + name + ext; + if (OS::FileExists(path.c_str())) { + return path; + } + } + LOG(FATAL) << "Test file " << name << " not found"; + UNREACHABLE(); + } + void SetUp() override { - CommonArtTest::SetUp(); + SetUpAndroidDataDir(android_data_); Dex2oatScratchDirs::SetUp(android_data_); } void TearDown() override { Dex2oatScratchDirs::TearDown(); - CommonArtTest::TearDown(); + TearDownAndroidDataDir(android_data_, true); + } + + struct ForkAndExecResult { + enum Stage { + kLink, + kFork, + kWaitpid, + kFinished, + }; + Stage stage; + int status_code; + + bool StandardSuccess() { + return stage == kFinished && WIFEXITED(status_code) && WEXITSTATUS(status_code) == 0; + } + }; + using OutputHandlerFn = std::function; + using PostForkFn = std::function; + + ForkAndExecResult ForkAndExec( + const std::vector& argv, + const PostForkFn& post_fork, + const OutputHandlerFn& handler) { + ForkAndExecResult result; + result.status_code = 0; + result.stage = ForkAndExecResult::kLink; + + std::vector c_args; + c_args.reserve(argv.size() + 1); + for (const std::string& str : argv) { + c_args.push_back(str.c_str()); + } + c_args.push_back(nullptr); + + android::base::unique_fd link[2]; + { + int link_fd[2]; + + if (pipe(link_fd) == -1) { + return result; + } + link[0].reset(link_fd[0]); + link[1].reset(link_fd[1]); + } + + result.stage = ForkAndExecResult::kFork; + + pid_t pid = fork(); + if (pid == -1) { + return result; + } + + if (pid == 0) { + if (!post_fork()) { + LOG(ERROR) << "Failed post-fork function"; + exit(1); + UNREACHABLE(); + } + + // Redirect stdout and stderr. + dup2(link[1].get(), STDOUT_FILENO); + dup2(link[1].get(), STDERR_FILENO); + + link[0].reset(); + link[1].reset(); + + execv(c_args[0], const_cast(c_args.data())); + exit(1); + UNREACHABLE(); + } + + result.stage = ForkAndExecResult::kWaitpid; + link[1].reset(); + + char buffer[128] = { 0 }; + ssize_t bytes_read = 0; + while (TEMP_FAILURE_RETRY(bytes_read = read(link[0].get(), buffer, 128)) > 0) { + handler(buffer, bytes_read); + } + handler(buffer, 0u); // End with a virtual write of zero length to simplify clients. + + link[0].reset(); + + if (waitpid(pid, &result.status_code, 0) == -1) { + return result; + } + + result.stage = ForkAndExecResult::kFinished; + return result; + } + + ForkAndExecResult ForkAndExec( + const std::vector& argv, const PostForkFn& post_fork, std::string* output) { + auto string_collect_fn = [output](char* buf, size_t len) { + *output += std::string(buf, len); + }; + return ForkAndExec(argv, post_fork, string_collect_fn); } protected: + std::string android_data_; + // Stripped down counterpart to Dex2oatEnvironmentTest::Dex2Oat that only adds // enough arguments for our purposes. int Dex2Oat(const std::vector& dex2oat_args, @@ -63,6 +291,8 @@ class Dex2oatCtsTest : public CommonArtTest, public Dex2oatScratchDirs { } }; +} // namespace + // Run dex2oat with --enable-palette-compilation-hooks to force calls to // PaletteNotify{Start,End}Dex2oatCompilation. TEST_F(Dex2oatCtsTest, CompilationHooks) { diff --git a/libartpalette/Android.bp b/libartpalette/Android.bp index 37464f0832..6ed1045dba 100644 --- a/libartpalette/Android.bp +++ b/libartpalette/Android.bp @@ -113,7 +113,6 @@ art_cc_defaults { srcs: ["apex/palette_test.cc"], shared_libs: [ "libartpalette", - "libnativehelper", ], } @@ -125,6 +124,9 @@ art_cc_test { "art_gtest_defaults", "art_libartpalette_tests_defaults", ], + shared_libs: [ + "libnativehelper", + ], host_supported: true, } @@ -133,9 +135,14 @@ art_cc_test { art_cc_test { name: "art_standalone_libartpalette_tests", defaults: [ - "art_standalone_gtest_defaults", + "art_cts_gtest_defaults", "art_libartpalette_tests_defaults", ], + static_libs: [ + // libnativehelper_lazy has the glue we need to dlsym the platform-only + // APIs we need, like JniInvocationInit. + "libnativehelper_lazy", + ], test_config_template: ":art-gtests-target-standalone-cts-template", test_suites: ["cts"], } diff --git a/libartpalette/apex/palette_test.cc b/libartpalette/apex/palette_test.cc index 0bcc09329b..16f283872b 100644 --- a/libartpalette/apex/palette_test.cc +++ b/libartpalette/apex/palette_test.cc @@ -22,6 +22,7 @@ #include #include "gtest/gtest.h" +#include "nativehelper/JniInvocation.h" \ namespace { @@ -78,6 +79,11 @@ TEST_F(PaletteClientTest, JniInvocation) { bool enabled; EXPECT_EQ(PALETTE_STATUS_OK, PaletteShouldReportJniInvocations(&enabled)); + // This calls JniInvocationInit, which is necessary to load the VM. It's not + // public but still stable. + JniInvocation jni_invocation; + ASSERT_TRUE(jni_invocation.Init(nullptr)); + JavaVMInitArgs vm_args; JavaVMOption options[] = { {.optionString = "-verbose:jni", .extraInfo = nullptr}, diff --git a/test/Android.bp b/test/Android.bp index 8a1ca84af1..6396f5efe0 100644 --- a/test/Android.bp +++ b/test/Android.bp @@ -314,6 +314,43 @@ art_cc_defaults { ], } +// Variant of art_standalone_gtest_defaults that doesn't link dynamically to any +// internal ART libraries. +art_cc_defaults { + name: "art_cts_gtest_defaults", + defaults: [ + // Note: We don't include "art_debug_defaults" here, as standalone ART + // gtests link with the "non-d" versions of the libraries contained in + // the ART APEX, so that they can be used with all ART APEX flavors + // (including the Release ART APEX). + "art_standalone_test_defaults", + "art_gtest_common_defaults", + ], + gtest: true, + + // Support multilib variants (using different suffix per sub-architecture), which is needed on + // build targets with secondary architectures, as the MTS test suite packaging logic flattens + // all test artifacts into a single `testcases` directory. + compile_multilib: "both", + multilib: { + lib32: { + suffix: "32", + }, + lib64: { + suffix: "64", + }, + }, + + static_libs: [ + "libartbase", + ], + + test_suites: [ + "general-tests", + "mts-art", + ], +} + // Properties common to `libart-gtest-defaults` and `libartd-gtest-defaults`. art_cc_defaults { name: "libart-gtest-common-defaults", -- cgit v1.2.3 From 50cfe06af2167fb4b554ab6ecc155571c5acc448 Mon Sep 17 00:00:00 2001 From: Martin Stjernholm Date: Wed, 20 Dec 2023 17:32:04 +0000 Subject: Do not add the ART APEX to LD_LIBRARY_PATH in the CTS tests, and link statically what's needed instead. Adding /apex/com.android.art/${LIB} to LD_LIBRARY_PATH conflates the linker namespace separation and causes ART module libs to be loaded in the default (= system) namespace. That causes linker errors when the module is used on older system images where unstable shared libs are no longer compatible. Instead link all internal libs statically and only leave the ones with exported APIs (which these CTS tests are intended to test). Merged-In set to a CL in main to avoid merging there - gtests are already linked statically there - it'll require a different fix. Test: lunch aosp_x86_64 atest art_libnativebridge_cts_tests \ art_standalone_libdexfile_external_tests \ libnativeloader_test \ art_standalone_libartpalette_tests using a cvd with a TM system image (had to apply https://r.android.com/2654960 to make atest work in android13-tests-dev) Bug: 317042881 Change-Id: I538b00418f2e5563f6d88ae8a6aa99372e44bb78 Merged-In: I538b00418f2e5563f6d88ae8a6aa99372e44bb78 Merged-In: I75a844f53663385ef98351f60d3adb900157f5e5 --- libartpalette/Android.bp | 5 ++--- libdexfile/Android.bp | 7 ++++++- test/art-gtests-target-standalone-cts-template.xml | 2 -- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/libartpalette/Android.bp b/libartpalette/Android.bp index 6ed1045dba..773e7c16ed 100644 --- a/libartpalette/Android.bp +++ b/libartpalette/Android.bp @@ -111,9 +111,6 @@ art_cc_library { art_cc_defaults { name: "art_libartpalette_tests_defaults", srcs: ["apex/palette_test.cc"], - shared_libs: [ - "libartpalette", - ], } // Version of ART gtest `art_libartpalette_tests` bundled with the ART APEX on target. @@ -125,6 +122,7 @@ art_cc_test { "art_libartpalette_tests_defaults", ], shared_libs: [ + "libartpalette", "libnativehelper", ], host_supported: true, @@ -139,6 +137,7 @@ art_cc_test { "art_libartpalette_tests_defaults", ], static_libs: [ + "libartpalette", // libnativehelper_lazy has the glue we need to dlsym the platform-only // APIs we need, like JniInvocationInit. "libnativehelper_lazy", diff --git a/libdexfile/Android.bp b/libdexfile/Android.bp index fdc57d0f17..3d910f2c41 100644 --- a/libdexfile/Android.bp +++ b/libdexfile/Android.bp @@ -370,7 +370,6 @@ art_cc_defaults { "external/dex_file_ext_test.cc", ], shared_libs: [ - "libartbase", "libdexfile", ], header_libs: [ @@ -387,6 +386,9 @@ art_cc_test { "art_test_defaults", "art_libdexfile_external_tests_defaults", ], + shared_libs: [ + "libartbase", + ], } // Standalone version of ART gtest `art_libdexfile_external_tests`, not bundled with the ART APEX on @@ -397,6 +399,9 @@ art_cc_test { "art_standalone_test_defaults", "art_libdexfile_external_tests_defaults", ], + static_libs: [ + "libartbase", + ], // Support multilib variants (using different suffix per sub-architecture), which is needed on // build targets with secondary architectures, as the CTS test suite packaging logic flattens diff --git a/test/art-gtests-target-standalone-cts-template.xml b/test/art-gtests-target-standalone-cts-template.xml index 3749c3e990..e862f171e2 100644 --- a/test/art-gtests-target-standalone-cts-template.xml +++ b/test/art-gtests-target-standalone-cts-template.xml @@ -30,8 +30,6 @@