linker: simplify how warnings turn into errors past a certain api level.
This was motivated by the fact that most of the _anchors_ on the doc page were outdated. I've taken the simple expedient of removing those. I was then struck by the amount of copy & paste involved in showing both warning and error, so the new function takes care of that.
Change-Id: I82d3e6a6d8235a78f7cfe427b1209a1f9c83f682
diff --git a/linker/linker.cpp b/linker/linker.cpp
index 0b829b3..c42712a 100644
--- a/linker/linker.cpp
+++ b/linker/linker.cpp
@@ -1084,19 +1084,16 @@
const char* fix_dt_needed(const char* dt_needed, const char* sopath __unused) {
#if !defined(__LP64__)
- // Work around incorrect DT_NEEDED entries for old apps: http://b/21364029
- int app_target_api_level = get_application_target_sdk_version();
- if (app_target_api_level < 23) {
+ if (get_application_target_sdk_version() < 23) {
const char* bname = basename(dt_needed);
if (bname != dt_needed) {
- DL_WARN_documented_change(23,
- "invalid-dt_needed-entries-enforced-for-api-level-23",
- "library \"%s\" has invalid DT_NEEDED entry \"%s\"",
- sopath, dt_needed, app_target_api_level);
- add_dlwarning(sopath, "invalid DT_NEEDED entry", dt_needed);
+ // Work around incorrect DT_NEEDED entries for old apps: http://b/21364029
+ if (!DL_ERROR_AFTER(23, "library \"%s\" has invalid DT_NEEDED entry \"%s\"",
+ sopath, dt_needed)) {
+ add_dlwarning(sopath, "invalid DT_NEEDED entry", dt_needed);
+ return bname;
+ }
}
-
- return bname;
}
#endif
return dt_needed;
@@ -1236,11 +1233,12 @@
const soinfo* needed_or_dlopened_by = task->get_needed_by();
const char* sopath = needed_or_dlopened_by == nullptr ? "(unknown)" :
needed_or_dlopened_by->get_realpath();
- DL_WARN_documented_change(24,
- "private-api-enforced-for-api-level-24",
- "library \"%s\" (\"%s\") needed or dlopened by \"%s\" "
- "is not accessible by namespace \"%s\"",
- name, realpath.c_str(), sopath, ns->get_name());
+ // is_exempt_lib() always returns true for api levels < 24,
+ // so no need to check the return value of DL_ERROR_AFTER().
+ // We still call it rather than DL_WARN() to get the extra clarification.
+ DL_ERROR_AFTER(24, "library \"%s\" (\"%s\") needed or dlopened by \"%s\" "
+ "is not accessible by namespace \"%s\"",
+ name, realpath.c_str(), sopath, ns->get_name());
add_dlwarning(sopath, "unauthorized access to", name);
}
} else {
@@ -3327,19 +3325,19 @@
}
}
- // Before M release, linker was using basename in place of soname. In the case when DT_SONAME is
- // absent some apps stop working because they can't find DT_NEEDED library by soname. This
- // workaround should keep them working. (Applies only for apps targeting sdk version < M.) Make
- // an exception for the main executable, which does not need to have DT_SONAME. The linker has an
- // DT_SONAME but the soname_ field is initialized later on.
+ // Before API 23, the linker used the basename in place of DT_SONAME.
+ // After we switched, apps with libraries without a DT_SONAME stopped working:
+ // they could no longer be found by DT_NEEDED from another library.
+ // The main executable does not need to have a DT_SONAME.
+ // The linker has a DT_SONAME, but the soname_ field is initialized later on.
if (soname_.empty() && this != solist_get_somain() && !relocating_linker &&
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_.c_str());
-
- // Don't call add_dlwarning because a missing DT_SONAME isn't important enough to show in the UI
+ // The `if` above means we don't get here for api levels >= 23,
+ // so no need to check the return value of DL_ERROR_AFTER().
+ // We still call it rather than DL_WARN() to get the extra clarification.
+ DL_ERROR_AFTER(23, "\"%s\" has no DT_SONAME (will use %s instead)",
+ get_realpath(), soname_.c_str());
}
// Validate each library's verdef section once, so we don't have to validate
@@ -3385,20 +3383,12 @@
#if !defined(__LP64__)
if (has_text_relocations) {
- // Fail if app is targeting M or above.
- int app_target_api_level = get_application_target_sdk_version();
- if (app_target_api_level >= 23) {
- DL_ERR_AND_LOG("\"%s\" has text relocations (%s#Text-Relocations-Enforced-for-API-level-23)",
- get_realpath(), kBionicChangesUrl);
+ if (DL_ERROR_AFTER(23, "\"%s\" has text relocations", get_realpath())) {
return false;
}
+ add_dlwarning(get_realpath(), "text relocations");
// Make segments writable to allow text relocations to work properly. We will later call
// phdr_table_protect_segments() after all of them are applied.
- DL_WARN_documented_change(23,
- "Text-Relocations-Enforced-for-API-level-23",
- "\"%s\" has text relocations",
- get_realpath());
- add_dlwarning(get_realpath(), "text relocations");
if (phdr_table_unprotect_segments(phdr, phnum, load_bias, should_pad_segments_,
should_use_16kib_app_compat_) < 0) {
DL_ERR("can't unprotect loadable segments for \"%s\": %m", get_realpath());
diff --git a/linker/linker.h b/linker/linker.h
index 7afa0d7..86ef762 100644
--- a/linker/linker.h
+++ b/linker/linker.h
@@ -71,10 +71,6 @@
DISALLOW_COPY_AND_ASSIGN(VersionTracker);
};
-static constexpr const char* kBionicChangesUrl =
- "https://android.googlesource.com/platform/bionic/+/main/"
- "android-changes-for-ndk-developers.md";
-
soinfo* get_libdl_info(const soinfo& linker_si);
soinfo* find_containing_library(const void* p);
diff --git a/linker/linker_globals.cpp b/linker/linker_globals.cpp
index 4a17d09..c3a3bcd 100644
--- a/linker/linker_globals.cpp
+++ b/linker/linker_globals.cpp
@@ -52,19 +52,24 @@
return sizeof(__linker_dl_err_buf);
}
-void DL_WARN_documented_change(int api_level, const char* doc_fragment, const char* fmt, ...) {
- std::string result{"Warning: "};
-
+bool DL_ERROR_AFTER(int target_sdk_version, const char* fmt, ...) {
+ std::string result;
va_list ap;
va_start(ap, fmt);
android::base::StringAppendV(&result, fmt, ap);
va_end(ap);
- android::base::StringAppendF(&result,
- " and will not work when the app moves to API level %d or later "
- "(%s#%s) (allowing for now because this app's target API level is "
- "still %d)",
- api_level, kBionicChangesUrl, doc_fragment,
- get_application_target_sdk_version());
- DL_WARN("%s", result.c_str());
+ if (get_application_target_sdk_version() < target_sdk_version) {
+ android::base::StringAppendF(&result,
+ " and will not work when the app moves to API level %d or later "
+ "(see https://android.googlesource.com/platform/bionic/+/main/"
+ "android-changes-for-ndk-developers.md); "
+ "allowing for now because this app's target API level is still %d",
+ target_sdk_version,
+ get_application_target_sdk_version());
+ DL_WARN("Warning: %s", result.c_str());
+ return false;
+ }
+ DL_ERR_AND_LOG("%s", result.c_str());
+ return true;
}
diff --git a/linker/linker_globals.h b/linker/linker_globals.h
index 2bfdccd..e6b4bc6 100644
--- a/linker/linker_globals.h
+++ b/linker/linker_globals.h
@@ -28,6 +28,8 @@
#pragma once
+#include "linker_debug.h"
+
#include <link.h>
#include <stddef.h>
@@ -49,7 +51,7 @@
async_safe_format_fd(2, "\n"); \
} while (false)
-void DL_WARN_documented_change(int api_level, const char* doc_link, const char* fmt, ...);
+bool DL_ERROR_AFTER(int target_sdk_version, const char* fmt, ...);
#define DL_ERR_AND_LOG(fmt, x...) \
do { \
diff --git a/linker/linker_phdr.cpp b/linker/linker_phdr.cpp
index c52952f..e695efd 100644
--- a/linker/linker_phdr.cpp
+++ b/linker/linker_phdr.cpp
@@ -308,26 +308,17 @@
}
if (header_.e_shentsize != sizeof(ElfW(Shdr))) {
- if (get_application_target_sdk_version() >= 26) {
- DL_ERR_AND_LOG("\"%s\" has unsupported e_shentsize: 0x%x (expected 0x%zx)",
- name_.c_str(), header_.e_shentsize, sizeof(ElfW(Shdr)));
+ if (DL_ERROR_AFTER(26, "\"%s\" has unsupported e_shentsize: 0x%x (expected 0x%zx)",
+ name_.c_str(), header_.e_shentsize, sizeof(ElfW(Shdr)))) {
return false;
}
- DL_WARN_documented_change(26,
- "invalid-elf-header_section-headers-enforced-for-api-level-26",
- "\"%s\" has unsupported e_shentsize 0x%x (expected 0x%zx)",
- name_.c_str(), header_.e_shentsize, sizeof(ElfW(Shdr)));
add_dlwarning(name_.c_str(), "has invalid ELF header");
}
if (header_.e_shstrndx == 0) {
- if (get_application_target_sdk_version() >= 26) {
- DL_ERR_AND_LOG("\"%s\" has invalid e_shstrndx", name_.c_str());
+ if (DL_ERROR_AFTER(26, "\"%s\" has invalid e_shstrndx", name_.c_str())) {
return false;
}
- DL_WARN_documented_change(26,
- "invalid-elf-header_section-headers-enforced-for-api-level-26",
- "\"%s\" has invalid e_shstrndx", name_.c_str());
add_dlwarning(name_.c_str(), "has invalid ELF header");
}
@@ -434,40 +425,24 @@
}
if (pt_dynamic_offset != dynamic_shdr->sh_offset) {
- if (get_application_target_sdk_version() >= 26) {
- DL_ERR_AND_LOG("\"%s\" .dynamic section has invalid offset: 0x%zx, "
- "expected to match PT_DYNAMIC offset: 0x%zx",
- name_.c_str(),
- static_cast<size_t>(dynamic_shdr->sh_offset),
- pt_dynamic_offset);
+ if (DL_ERROR_AFTER(26, "\"%s\" .dynamic section has invalid offset: 0x%zx, "
+ "expected to match PT_DYNAMIC offset: 0x%zx",
+ name_.c_str(),
+ static_cast<size_t>(dynamic_shdr->sh_offset),
+ pt_dynamic_offset)) {
return false;
}
- DL_WARN_documented_change(26,
- "invalid-elf-header_section-headers-enforced-for-api-level-26",
- "\"%s\" .dynamic section has invalid offset: 0x%zx "
- "(expected to match PT_DYNAMIC offset 0x%zx)",
- name_.c_str(),
- static_cast<size_t>(dynamic_shdr->sh_offset),
- pt_dynamic_offset);
add_dlwarning(name_.c_str(), "invalid .dynamic section");
}
if (pt_dynamic_filesz != dynamic_shdr->sh_size) {
- if (get_application_target_sdk_version() >= 26) {
- DL_ERR_AND_LOG("\"%s\" .dynamic section has invalid size: 0x%zx, "
- "expected to match PT_DYNAMIC filesz: 0x%zx",
- name_.c_str(),
- static_cast<size_t>(dynamic_shdr->sh_size),
- pt_dynamic_filesz);
+ if (DL_ERROR_AFTER(26, "\"%s\" .dynamic section has invalid size: 0x%zx "
+ "(expected to match PT_DYNAMIC filesz 0x%zx)",
+ name_.c_str(),
+ static_cast<size_t>(dynamic_shdr->sh_size),
+ pt_dynamic_filesz)) {
return false;
}
- DL_WARN_documented_change(26,
- "invalid-elf-header_section-headers-enforced-for-api-level-26",
- "\"%s\" .dynamic section has invalid size: 0x%zx "
- "(expected to match PT_DYNAMIC filesz 0x%zx)",
- name_.c_str(),
- static_cast<size_t>(dynamic_shdr->sh_size),
- pt_dynamic_filesz);
add_dlwarning(name_.c_str(), "invalid .dynamic section");
}
@@ -1058,15 +1033,10 @@
if (file_length != 0) {
int prot = PFLAGS_TO_PROT(phdr->p_flags);
if ((prot & (PROT_EXEC | PROT_WRITE)) == (PROT_EXEC | PROT_WRITE)) {
- // W + E PT_LOAD segments are not allowed in O.
- if (get_application_target_sdk_version() >= 26) {
- DL_ERR_AND_LOG("\"%s\": W+E load segments are not allowed", name_.c_str());
+ if (DL_ERROR_AFTER(26, "\"%s\" has load segments that are both writable and executable",
+ name_.c_str())) {
return false;
}
- DL_WARN_documented_change(26,
- "writable-and-executable-segments-enforced-for-api-level-26",
- "\"%s\" has load segments that are both writable and executable",
- name_.c_str());
add_dlwarning(name_.c_str(), "W+E load segments");
}
diff --git a/linker/linker_test_globals.cpp b/linker/linker_test_globals.cpp
index 27ec6f7..5b4ffe3 100644
--- a/linker/linker_test_globals.cpp
+++ b/linker/linker_test_globals.cpp
@@ -27,7 +27,7 @@
*/
// Stub some symbols to avoid linking issues
-void DL_WARN_documented_change(int api_level [[maybe_unused]],
- const char* doc_link [[maybe_unused]],
- const char* fmt [[maybe_unused]], ...) {}
-
+bool DL_ERROR_AFTER(int target_sdk_version [[maybe_unused]],
+ const char* fmt [[maybe_unused]], ...) {
+ return false;
+}
diff --git a/tests/dlfcn_test.cpp b/tests/dlfcn_test.cpp
index c27adb6..57982b9 100644
--- a/tests/dlfcn_test.cpp
+++ b/tests/dlfcn_test.cpp
@@ -1584,7 +1584,7 @@
const std::string libpath = GetPrebuiltElfDir() + "/libtest_invalid-rw_load_segment.so";
void* handle = dlopen(libpath.c_str(), RTLD_NOW);
ASSERT_TRUE(handle == nullptr);
- std::string expected_dlerror = std::string("dlopen failed: \"") + libpath + "\": W+E load segments are not allowed";
+ std::string expected_dlerror = std::string("dlopen failed: \"") + libpath + "\" has load segments that are both writable and executable";
ASSERT_STREQ(expected_dlerror.c_str(), dlerror());
}