From f9dd1a760af7e3b285169bc38fb33739e54b4636 Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Mon, 11 Jan 2021 09:04:58 -0800 Subject: [PATCH] Store soname as a std::string. Once upon a time (and, indeed, to this very day if you're on LP32) the soinfo struct used a fixed-length buffer for the soname. This caused some issues, mainly with app developers who accidentally included a full Windows "C:\My Computer\...\libfoo.so" style path. To avoid all this we switched to just pointing into the ELF file itself, where the DT_SONAME is already stored as a NUL-terminated string. And all was well for many years. Now though, we've seen a bunch of slow startup traces from dogfood where `dlopen("libnativebridge.so")` in a cold start takes 125-200ms on a recent device, despite no IO contention. Even though libnativebridge.so is only 20KiB. Measurement showed that every library whose soname we check required pulling in a whole page just for the (usually) very short string. Worse, there's readahead. In one trace we saw 18 pages of libhwui.so pulled in just for `"libhwui.so\0"`. In fact, there were 3306 pages (~13MiB) added to the page cache during `dlopen("libnativebridge.so")`. 13MiB for a 20KiB shared library! This is the obvious change to use a std::string to copy the sonames instead. This will dirty slightly more memory, but massively improve locality. Testing with the same pathological setup took `dlopen("libnativebridge.so")` down from 192ms to 819us. Bug: http://b/177102905 Test: tested with a pathologically modified kernel Change-Id: I33837f4706adc25f93c6fa6013e8ba970911dfb9 --- linker/dlfcn.cpp | 5 +++-- linker/linker.cpp | 19 +++++++------------ linker/linker_cfi.cpp | 3 +-- linker/linker_namespaces.h | 3 --- linker/linker_soinfo.cpp | 6 +++--- linker/linker_soinfo.h | 2 +- 6 files changed, 15 insertions(+), 23 deletions(-) diff --git a/linker/dlfcn.cpp b/linker/dlfcn.cpp index ec6850a40..772e7b81c 100644 --- a/linker/dlfcn.cpp +++ b/linker/dlfcn.cpp @@ -328,11 +328,12 @@ soinfo* get_libdl_info(const soinfo& linker_si) { __libdl_info->ref_count_ = 1; __libdl_info->strtab_size_ = linker_si.strtab_size_; __libdl_info->local_group_root_ = __libdl_info; - __libdl_info->soname_ = linker_si.soname_; + __libdl_info->soname_ = linker_si.soname_.c_str(); __libdl_info->target_sdk_version_ = __ANDROID_API__; __libdl_info->generate_handle(); #if defined(__work_around_b_24465209__) - strlcpy(__libdl_info->old_name_, __libdl_info->soname_, sizeof(__libdl_info->old_name_)); + strlcpy(__libdl_info->old_name_, __libdl_info->soname_.c_str(), + sizeof(__libdl_info->old_name_)); #endif } diff --git a/linker/linker.cpp b/linker/linker.cpp index 2481be4e4..77824f615 100644 --- a/linker/linker.cpp +++ b/linker/linker.cpp @@ -1339,8 +1339,7 @@ static bool find_loaded_library_by_soname(android_namespace_t* ns, const char* name, soinfo** candidate) { return !ns->soinfo_list().visit([&](soinfo* si) { - const char* soname = si->get_soname(); - if (soname != nullptr && (strcmp(name, soname) == 0)) { + if (strcmp(name, si->get_soname()) == 0) { *candidate = si; return false; } @@ -2571,9 +2570,8 @@ bool VersionTracker::init_verneed(const soinfo* si_from) { const char* target_soname = si_from->get_string(verneed->vn_file); // find it in dependencies - soinfo* target_si = si_from->get_children().find_if([&](const soinfo* si) { - return si->get_soname() != nullptr && strcmp(si->get_soname(), target_soname) == 0; - }); + soinfo* target_si = si_from->get_children().find_if( + [&](const soinfo* si) { return strcmp(si->get_soname(), target_soname) == 0; }); if (target_si == nullptr) { DL_ERR("cannot find \"%s\" from verneed[%zd] in DT_NEEDED list for \"%s\"", @@ -3214,15 +3212,12 @@ bool soinfo::prelink_image() { // for apps targeting sdk version < M.) Make an exception for // the main executable and linker; they do not need to have dt_soname. // TODO: >= O the linker doesn't need this workaround. - if (soname_ == nullptr && - this != solist_get_somain() && - (flags_ & FLAG_LINKER) == 0 && + if (soname_.empty() && this != solist_get_somain() && (flags_ & FLAG_LINKER) == 0 && get_application_target_sdk_version() < 23) { soname_ = basename(realpath_.c_str()); - DL_WARN_documented_change(23, - "missing-soname-enforced-for-api-level-23", - "\"%s\" has no DT_SONAME (will use %s instead)", - get_realpath(), soname_); + DL_WARN_documented_change(23, "missing-soname-enforced-for-api-level-23", + "\"%s\" has no DT_SONAME (will use %s instead)", get_realpath(), + soname_.c_str()); // Don't call add_dlwarning because a missing DT_SONAME isn't important enough to show in the UI } diff --git a/linker/linker_cfi.cpp b/linker/linker_cfi.cpp index 87b5d3485..6bc261545 100644 --- a/linker/linker_cfi.cpp +++ b/linker/linker_cfi.cpp @@ -133,8 +133,7 @@ void CFIShadowWriter::Add(uintptr_t begin, uintptr_t end, uintptr_t cfi_check) { static soinfo* find_libdl(soinfo* solist) { for (soinfo* si = solist; si != nullptr; si = si->next) { - const char* soname = si->get_soname(); - if (soname && strcmp(soname, "libdl.so") == 0) { + if (strcmp(si->get_soname(), "libdl.so") == 0) { return si; } } diff --git a/linker/linker_namespaces.h b/linker/linker_namespaces.h index 77b662294..6817901a3 100644 --- a/linker/linker_namespaces.h +++ b/linker/linker_namespaces.h @@ -56,9 +56,6 @@ struct android_namespace_link_t { } bool is_accessible(const char* soname) const { - if (soname == nullptr) { - return false; - } return allow_all_shared_libs_ || shared_lib_sonames_.find(soname) != shared_lib_sonames_.end(); } diff --git a/linker/linker_soinfo.cpp b/linker/linker_soinfo.cpp index 60fd242cc..f44cb1c4b 100644 --- a/linker/linker_soinfo.cpp +++ b/linker/linker_soinfo.cpp @@ -695,7 +695,7 @@ void soinfo::set_soname(const char* soname) { if (has_min_version(2)) { soname_ = soname; } - strlcpy(old_name_, soname_, sizeof(old_name_)); + strlcpy(old_name_, soname_.c_str(), sizeof(old_name_)); #else soname_ = soname; #endif @@ -704,12 +704,12 @@ void soinfo::set_soname(const char* soname) { const char* soinfo::get_soname() const { #if defined(__work_around_b_24465209__) if (has_min_version(2)) { - return soname_; + return soname_.c_str(); } else { return old_name_; } #else - return soname_; + return soname_.c_str(); #endif } diff --git a/linker/linker_soinfo.h b/linker/linker_soinfo.h index 7372a51e8..9c589d608 100644 --- a/linker/linker_soinfo.h +++ b/linker/linker_soinfo.h @@ -401,7 +401,7 @@ struct soinfo { uint8_t* android_relocs_; size_t android_relocs_size_; - const char* soname_; + std::string soname_; std::string realpath_; const ElfW(Versym)* versym_;