Merge "Be clearer about linker warnings."

9 files changed, 102 insertions, 63 deletions

diff --git a/linker/linker.cpp b/linker/linker.cpp
index ff2a7e635..dd700fe88 100644
--- a/linker/linker.cpp
+++ b/linker/linker.cpp
@@ -1105,10 +1105,14 @@ static int open_library(android_namespace_t* ns,
 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
-  if (get_application_target_sdk_version() < __ANDROID_API_M__) {
+  int app_target_api_level = get_application_target_sdk_version();
+  if (app_target_api_level < __ANDROID_API_M__) {
     const char* bname = basename(dt_needed);
     if (bname != dt_needed) {
-      DL_WARN("library \"%s\" has invalid DT_NEEDED entry \"%s\"", sopath, dt_needed);
+      DL_WARN_documented_change(__ANDROID_API_M__,
+                                "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);
     }
 
@@ -1246,10 +1250,11 @@ static bool load_library(android_namespace_t* ns,
         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("library \"%s\" (\"%s\") needed or dlopened by \"%s\" is not accessible for the namespace \"%s\""
-                " - the access is temporarily granted as a workaround for http://b/26394120, note that the access"
-                " will be removed in future releases of Android.",
-                name, realpath.c_str(), sopath, ns->get_name());
+        DL_WARN_documented_change(__ANDROID_API_N__,
+                                  "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());
         add_dlwarning(sopath, "unauthorized access to",  name);
       }
     } else {
@@ -3363,7 +3368,9 @@ bool soinfo::prelink_image() {
         set_dt_flags_1(d->d_un.d_val);
 
         if ((d->d_un.d_val & ~SUPPORTED_DT_FLAGS_1) != 0) {
-          DL_WARN("\"%s\" has unsupported flags DT_FLAGS_1=%p", get_realpath(), reinterpret_cast<void*>(d->d_un.d_val));
+          DL_WARN("Warning: \"%s\" has unsupported flags DT_FLAGS_1=%p "
+                  "(ignoring unsupported flags)",
+                  get_realpath(), reinterpret_cast<void*>(d->d_un.d_val));
         }
         break;
 #if defined(__mips__)
@@ -3442,7 +3449,7 @@ bool soinfo::prelink_image() {
           } else {
             tag_name = "unknown";
           }
-          DL_WARN("\"%s\" unused DT entry: %s (type %p arg %p)",
+          DL_WARN("Warning: \"%s\" unused DT entry: %s (type %p arg %p) (ignoring)",
                   get_realpath(),
                   tag_name,
                   reinterpret_cast<void*>(d->d_tag),
@@ -3495,16 +3502,20 @@ bool soinfo::prelink_image() {
   // 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 and linker; they do not need to have dt_soname
+  // This workaround should keep them working. (Applies only
+  // 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 &&
       get_application_target_sdk_version() < __ANDROID_API_M__) {
     soname_ = basename(realpath_.c_str());
-    DL_WARN("%s: is missing DT_SONAME will use basename as a replacement: \"%s\"",
-        get_realpath(), soname_);
+    DL_WARN_documented_change(__ANDROID_API_M__,
+                              "missing-soname-enforced-for-api-level-23",
+                              "\"%s\" has no DT_SONAME (will use %s instead)",
+                              get_realpath(), soname_);
+
     // Don't call add_dlwarning because a missing DT_SONAME isn't important enough to show in the UI
   }
   return true;
@@ -3535,7 +3546,8 @@ bool soinfo::link_image(const soinfo_list_t& global_group, const soinfo_list_t&
 #if !defined(__LP64__)
   if (has_text_relocations) {
     // Fail if app is targeting M or above.
-    if (get_application_target_sdk_version() >= __ANDROID_API_M__) {
+    int app_target_api_level = get_application_target_sdk_version();
+    if (app_target_api_level >= __ANDROID_API_M__) {
       DL_ERR_AND_LOG("\"%s\" has text relocations (https://android.googlesource.com/platform/"
                      "bionic/+/master/android-changes-for-ndk-developers.md#Text-Relocations-"
                      "Enforced-for-API-level-23)", get_realpath());
@@ -3543,13 +3555,13 @@ bool soinfo::link_image(const soinfo_list_t& global_group, const soinfo_list_t&
     }
     // 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("\"%s\" has text relocations (https://android.googlesource.com/platform/"
-            "bionic/+/master/android-changes-for-ndk-developers.md#Text-Relocations-Enforced-"
-            "for-API-level-23)", get_realpath());
+    DL_WARN_documented_change(__ANDROID_API_M__,
+                              "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) < 0) {
-      DL_ERR("can't unprotect loadable segments for \"%s\": %s",
-             get_realpath(), strerror(errno));
+      DL_ERR("can't unprotect loadable segments for \"%s\": %s", get_realpath(), strerror(errno));
       return false;
     }
   }
@@ -3739,7 +3751,7 @@ std::vector<android_namespace_t*> init_default_namespaces(const char* executable
                                   &config,
                                   &error_msg)) {
     if (!error_msg.empty()) {
-      DL_WARN("error reading config file \"%s\" for \"%s\" (will use default configuration): %s",
+      DL_WARN("Warning: couldn't read \"%s\" for \"%s\" (using default configuration instead): %s",
               config_file,
               executable_path,
               error_msg.c_str());
diff --git a/linker/linker_config.cpp b/linker/linker_config.cpp
index 83c2f36a1..c00b73407 100644
--- a/linker/linker_config.cpp
+++ b/linker/linker_config.cpp
@@ -194,14 +194,14 @@ static bool parse_config_file(const char* ld_config_file_path,
 
   std::string section_name;
 
-  while(true) {
+  while (true) {
     std::string name;
     std::string value;
     std::string error;
 
     int result = cp.next_token(&name, &value, &error);
     if (result == ConfigParser::kError) {
-      DL_WARN("error parsing %s:%zd: %s (ignoring this line)",
+      DL_WARN("%s:%zd: warning: couldn't parse %s (ignoring this line)",
               ld_config_file_path,
               cp.lineno(),
               error.c_str());
@@ -214,7 +214,7 @@ static bool parse_config_file(const char* ld_config_file_path,
 
     if (result == ConfigParser::kPropertyAssign) {
       if (!android::base::StartsWith(name, "dir.")) {
-        DL_WARN("error parsing %s:%zd: unexpected property name \"%s\", "
+        DL_WARN("%s:%zd: warning: unexpected property name \"%s\", "
                 "expected format dir.<section_name> (ignoring this line)",
                 ld_config_file_path,
                 cp.lineno(),
@@ -228,7 +228,7 @@ static bool parse_config_file(const char* ld_config_file_path,
       }
 
       if (value.empty()) {
-        DL_WARN("error parsing %s:%zd: property value is empty (ignoring this line)",
+        DL_WARN("%s:%zd: warning: property value is empty (ignoring this line)",
                 ld_config_file_path,
                 cp.lineno());
         continue;
@@ -275,7 +275,7 @@ static bool parse_config_file(const char* ld_config_file_path,
 
     if (result == ConfigParser::kPropertyAssign) {
       if (properties->find(name) != properties->end()) {
-        DL_WARN("%s:%zd: warning: property \"%s\" redefinition",
+        DL_WARN("%s:%zd: warning: redefining property \"%s\" (overriding previous value)",
                 ld_config_file_path,
                 cp.lineno(),
                 name.c_str());
@@ -284,7 +284,7 @@ static bool parse_config_file(const char* ld_config_file_path,
       (*properties)[name] = PropertyValue(std::move(value), cp.lineno());
     } else if (result == ConfigParser::kPropertyAppend) {
       if (properties->find(name) == properties->end()) {
-        DL_WARN("%s:%zd: warning: appending to property \"%s\" which isn't defined",
+        DL_WARN("%s:%zd: warning: appending to undefined property \"%s\" (treating as assignment)",
                 ld_config_file_path,
                 cp.lineno(),
                 name.c_str());
@@ -299,7 +299,7 @@ static bool parse_config_file(const char* ld_config_file_path,
           value = ":" + value;
           (*properties)[name].append_value(std::move(value));
         } else {
-          DL_WARN("%s:%zd: warning: += isn't allowed to property \"%s\". Ignoring.",
+          DL_WARN("%s:%zd: warning: += isn't allowed for property \"%s\" (ignoring)",
                   ld_config_file_path,
                   cp.lineno(),
                   name.c_str());
@@ -308,7 +308,7 @@ static bool parse_config_file(const char* ld_config_file_path,
     }
 
     if (result == ConfigParser::kError) {
-      DL_WARN("error parsing %s:%zd: %s (ignoring this line)",
+      DL_WARN("%s:%zd: warning: couldn't parse %s (ignoring this line)",
               ld_config_file_path,
               cp.lineno(),
               error.c_str());
diff --git a/linker/linker_globals.cpp b/linker/linker_globals.cpp
index 155ebf45a..bcc2a1e6f 100644
--- a/linker/linker_globals.cpp
+++ b/linker/linker_globals.cpp
@@ -26,10 +26,12 @@
  * SUCH DAMAGE.
  */
 
-
+#include "linker.h"
 #include "linker_globals.h"
 #include "linker_namespaces.h"
 
+#include "android-base/stringprintf.h"
+
 int g_argc = 0;
 char** g_argv = nullptr;
 char** g_envp = nullptr;
@@ -48,3 +50,18 @@ size_t linker_get_error_buffer_size() {
   return sizeof(__linker_dl_err_buf);
 }
 
+void DL_WARN_documented_change(int api_level, const char* doc_link, const char* fmt, ...) {
+  std::string result{"Warning: "};
+
+  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 "
+                               "(https://android.googlesource.com/platform/bionic/+/master/%s) "
+                               "(allowing for now because this app's target API level is still %d)",
+                               api_level, doc_link, get_application_target_sdk_version());
+  DL_WARN("%s", result.c_str());
+}
diff --git a/linker/linker_globals.h b/linker/linker_globals.h
index b470918b0..32aa09db6 100644
--- a/linker/linker_globals.h
+++ b/linker/linker_globals.h
@@ -49,6 +49,8 @@
       async_safe_format_fd(2, "\n"); \
     } while (false)
 
+void DL_WARN_documented_change(int api_level, const char* doc_link, const char* fmt, ...);
+
 #define DL_ERR_AND_LOG(fmt, x...) \
   do { \
     DL_ERR(fmt, x); \
diff --git a/linker/linker_namespaces.cpp b/linker/linker_namespaces.cpp
index 9fdf0b51c..fd72cdc73 100644
--- a/linker/linker_namespaces.cpp
+++ b/linker/linker_namespaces.cpp
@@ -64,7 +64,8 @@ bool android_namespace_t::is_accessible(soinfo* s) {
     // This is workaround for apps hacking into soinfo list.
     // and inserting their own entries into it. (http://b/37191433)
     if (!si->has_min_version(3)) {
-      DL_WARN("invalid soinfo version for \"%s\"", si->get_soname());
+      DL_WARN("Warning: invalid soinfo version for \"%s\" (assuming inaccessible)",
+              si->get_soname());
       return false;
     }
 
diff --git a/linker/linker_phdr.cpp b/linker/linker_phdr.cpp
index a9873c483..a5eab44ec 100644
--- a/linker/linker_phdr.cpp
+++ b/linker/linker_phdr.cpp
@@ -268,8 +268,10 @@ bool ElfReader::VerifyElfHeader() {
                      name_.c_str(), header_.e_shentsize, sizeof(ElfW(Shdr)));
       return false;
     }
-    DL_WARN("\"%s\" has unsupported e_shentsize: 0x%x (expected 0x%zx)",
-            name_.c_str(), header_.e_shentsize, sizeof(ElfW(Shdr)));
+    DL_WARN_documented_change(__ANDROID_API_O__,
+                              "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");
   }
 
@@ -280,7 +282,9 @@ bool ElfReader::VerifyElfHeader() {
       return false;
     }
 
-    DL_WARN("\"%s\" has invalid e_shstrndx", name_.c_str());
+    DL_WARN_documented_change(__ANDROID_API_O__,
+                              "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");
   }
 
@@ -395,11 +399,13 @@ bool ElfReader::ReadDynamicSection() {
                      pt_dynamic_offset);
       return false;
     }
-    DL_WARN("\"%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);
+    DL_WARN_documented_change(__ANDROID_API_O__,
+                              "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");
   }
 
@@ -412,11 +418,13 @@ bool ElfReader::ReadDynamicSection() {
                      pt_dynamic_filesz);
       return false;
     }
-    DL_WARN("\"%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);
+    DL_WARN_documented_change(__ANDROID_API_O__,
+                              "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");
   }
 
@@ -651,10 +659,13 @@ bool ElfReader::LoadSegments() {
       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() >= __ANDROID_API_O__) {
-          DL_ERR_AND_LOG("\"%s\": W + E load segments are not allowed", name_.c_str());
+          DL_ERR_AND_LOG("\"%s\": W+E load segments are not allowed", name_.c_str());
           return false;
         }
-        DL_WARN("\"%s\": W + E load segments are not allowed", name_.c_str());
+        DL_WARN_documented_change(__ANDROID_API_O__,
+                                  "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_soinfo.cpp b/linker/linker_soinfo.cpp
index 54bfcf0b3..731e8f5e0 100644
--- a/linker/linker_soinfo.cpp
+++ b/linker/linker_soinfo.cpp
@@ -152,7 +152,7 @@ static bool is_symbol_global_and_defined(const soinfo* si, const ElfW(Sym)* s) {
       ELF_ST_BIND(s->st_info) == STB_WEAK) {
     return s->st_shndx != SHN_UNDEF;
   } else if (ELF_ST_BIND(s->st_info) != STB_LOCAL) {
-    DL_WARN("unexpected ST_BIND value: %d for \"%s\" in \"%s\"",
+    DL_WARN("Warning: unexpected ST_BIND value: %d for \"%s\" in \"%s\" (ignoring)",
             ELF_ST_BIND(s->st_info), si->get_string(s->st_name), si->get_realpath());
   }
 
diff --git a/linker/linker_utils.cpp b/linker/linker_utils.cpp
index 5bf88e764..661e7cbc1 100644
--- a/linker/linker_utils.cpp
+++ b/linker/linker_utils.cpp
@@ -209,31 +209,28 @@ void resolve_paths(std::vector<std::string>& paths,
     const char* original_path = path.c_str();
     if (realpath(original_path, resolved_path) != nullptr) {
       struct stat s;
-      if (stat(resolved_path, &s) == 0) {
-        if (S_ISDIR(s.st_mode)) {
-          resolved_paths->push_back(resolved_path);
-        } else {
-          DL_WARN("Warning: \"%s\" is not a directory (excluding from path)", resolved_path);
-          continue;
-        }
-      } else {
-        DL_WARN("Warning: cannot stat file \"%s\": %s", resolved_path, strerror(errno));
+      if (stat(resolved_path, &s) == -1) {
+        DL_WARN("Warning: cannot stat file \"%s\": %s (ignoring)", resolved_path, strerror(errno));
+        continue;
+      }
+      if (!S_ISDIR(s.st_mode)) {
+        DL_WARN("Warning: \"%s\" is not a directory (ignoring)", resolved_path);
         continue;
       }
+      resolved_paths->push_back(resolved_path);
     } else {
-      std::string zip_path;
-      std::string entry_path;
-
       std::string normalized_path;
-
       if (!normalize_path(original_path, &normalized_path)) {
-        DL_WARN("Warning: unable to normalize \"%s\"", original_path);
+        DL_WARN("Warning: unable to normalize \"%s\" (ignoring)", original_path);
         continue;
       }
 
+      std::string zip_path;
+      std::string entry_path;
       if (parse_zip_path(normalized_path.c_str(), &zip_path, &entry_path)) {
         if (realpath(zip_path.c_str(), resolved_path) == nullptr) {
-          DL_WARN("Warning: unable to resolve \"%s\": %s", zip_path.c_str(), strerror(errno));
+          DL_WARN("Warning: unable to resolve \"%s\": %s (ignoring)",
+                  zip_path.c_str(), strerror(errno));
           continue;
         }
 
@@ -242,4 +239,3 @@ void resolve_paths(std::vector<std::string>& paths,
     }
   }
 }
-
diff --git a/tests/dlfcn_test.cpp b/tests/dlfcn_test.cpp
index c40a35bf2..46f5097a7 100644
--- a/tests/dlfcn_test.cpp
+++ b/tests/dlfcn_test.cpp
@@ -1503,7 +1503,7 @@ TEST(dlfcn, dlopen_invalid_rw_load_segment) {
                               "/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 + "\": W+E load segments are not allowed";
   ASSERT_STREQ(expected_dlerror.c_str(), dlerror());
 }