Revert "Revert "Fix dlsym(3) to do breadth first search.""

This reverts commit 1b1966d944.

Change-Id: I05d6d3481aaf8f3e260d2e5e950248519a1d347f

linker/dlfcn.cpp

@@ -111,8 +111,7 @@ void* dlsym(void* handle, const char* symbol) {
       sym = dlsym_linear_lookup(symbol, &found, caller_si->next, caller_si);
     }
   } else {
-    found = reinterpret_cast<soinfo*>(handle);
-    sym = dlsym_handle_lookup(found, symbol, caller_si);
+    sym = dlsym_handle_lookup(reinterpret_cast<soinfo*>(handle), &found, symbol, caller_si);
   }
 
   if (sym != NULL && sym->st_shndx != 0) {

linker/linked_list.h

@@ -31,13 +31,45 @@ struct LinkedListEntry {
 template<typename T, typename Allocator>
 class LinkedList {
  public:
-  LinkedList() : head_(nullptr) {}
+  LinkedList() : head_(nullptr), tail_(nullptr) {}
 
   void push_front(T* const element) {
     LinkedListEntry<T>* new_entry = Allocator::alloc();
     new_entry->next = head_;
     new_entry->element = element;
     head_ = new_entry;
+    if (tail_ == nullptr) {
+      tail_ = new_entry;
+    }
+  }
+
+  void push_back(T* const element) {
+    LinkedListEntry<T>* new_entry = Allocator::alloc();
+    new_entry->next = nullptr;
+    new_entry->element = element;
+    if (tail_ == nullptr) {
+      tail_ = head_ = new_entry;
+    } else {
+      tail_->next = new_entry;
+      tail_ = new_entry;
+    }
+  }
+
+  T* pop_front() {
+    if (head_ == nullptr) {
+      return nullptr;
+    }
+
+    LinkedListEntry<T>* entry = head_;
+    T* element = entry->element;
+    head_ = entry->next;
+    Allocator::free(entry);
+
+    if (head_ == nullptr) {
+      tail_ = nullptr;
+    }
+
+    return element;
   }
 
   void clear() {
@@ -46,6 +78,8 @@ class LinkedList {
       head_ = head_->next;
       Allocator::free(p);
     }
+
+    tail_ = nullptr;
   }
 
   template<typename F>
@@ -68,6 +102,7 @@ class LinkedList {
 
  private:
   LinkedListEntry<T>* head_;
+  LinkedListEntry<T>* tail_;
   DISALLOW_COPY_AND_ASSIGN(LinkedList);
 };
 

linker/linker.cpp

@@ -469,6 +469,10 @@ static ElfW(Sym)* soinfo_elf_lookup(soinfo* si, unsigned hash, const char* name,
     }
   }
 
+  TRACE_TYPE(LOOKUP, "NOT FOUND %s in %s@%p %x %zd",
+             name, si->name, reinterpret_cast<void*>(si->base), hash, hash % si->nbucket);
+
+
   return NULL;
 }
 
@@ -585,18 +589,43 @@ done:
     return NULL;
 }
 
-/* This is used by dlsym(3).  It performs symbol lookup only within the
-   specified soinfo object and not in any of its dependencies.
+// Another soinfo list allocator to use in dlsym. We don't reuse
+// SoinfoListAllocator because it is write-protected most of the time.
+static LinkerAllocator<LinkedListEntry<soinfo>> g_soinfo_list_allocator_rw;
+class SoinfoListAllocatorRW {
+ public:
+  static LinkedListEntry<soinfo>* alloc() {
+    return g_soinfo_list_allocator_rw.alloc();
+  }
 
-   TODO: Only looking in the specified soinfo seems wrong. dlsym(3) says
-   that it should do a breadth first search through the dependency
-   tree. This agrees with the ELF spec (aka System V Application
-   Binary Interface) where in Chapter 5 it discuss resolving "Shared
-   Object Dependencies" in breadth first search order.
- */
-ElfW(Sym)* dlsym_handle_lookup(soinfo* si, const char* name, soinfo* caller) {
-    return soinfo_elf_lookup(si, elfhash(name), name,
-        caller == si ? SymbolLookupScope::kAllowLocal : SymbolLookupScope::kExcludeLocal);
+  static void free(LinkedListEntry<soinfo>* ptr) {
+    g_soinfo_list_allocator_rw.free(ptr);
+  }
+};
+
+// This is used by dlsym(3).  It performs symbol lookup only within the
+// specified soinfo object and its dependencies in breadth first order.
+ElfW(Sym)* dlsym_handle_lookup(soinfo* si, soinfo** found, const char* name, soinfo* caller) {
+  LinkedList<soinfo, SoinfoListAllocatorRW> visit_list;
+  visit_list.push_back(si);
+  soinfo* current_soinfo;
+  while ((current_soinfo = visit_list.pop_front()) != nullptr) {
+    ElfW(Sym)* result = soinfo_elf_lookup(current_soinfo, elfhash(name), name,
+        caller == current_soinfo ? SymbolLookupScope::kAllowLocal : SymbolLookupScope::kExcludeLocal);
+
+    if (result != nullptr) {
+      *found = current_soinfo;
+      visit_list.clear();
+      return result;
+    }
+
+    current_soinfo->get_children().for_each([&](soinfo* child) {
+      visit_list.push_back(child);
+    });
+  }
+
+  visit_list.clear();
+  return nullptr;
 }
 
 /* This is used by dlsym(3) to performs a global symbol lookup. If the

linker/linker.h

@@ -238,7 +238,7 @@ ElfW(Sym)* dlsym_linear_lookup(const char* name, soinfo** found, soinfo* start,
 soinfo* find_containing_library(const void* addr);
 
 ElfW(Sym)* dladdr_find_symbol(soinfo* si, const void* addr);
-ElfW(Sym)* dlsym_handle_lookup(soinfo* si, const char* name, soinfo* caller_si);
+ElfW(Sym)* dlsym_handle_lookup(soinfo* si, soinfo** found, const char* name, soinfo* caller_si);
 
 void debuggerd_init();
 extern "C" abort_msg_t* g_abort_message;

linker/tests/linked_list_test.cpp

@@ -95,3 +95,23 @@ TEST(linked_list, simple) {
   ASSERT_TRUE(free_called);
   ASSERT_EQ("", test_list_to_string(list));
 }
+
+TEST(linked_list, push_pop) {
+  test_list_t list;
+  list.push_front("b");
+  list.push_front("a");
+  ASSERT_EQ("ab", test_list_to_string(list));
+  list.push_back("c");
+  ASSERT_EQ("abc", test_list_to_string(list));
+  ASSERT_EQ("a", list.pop_front());
+  ASSERT_EQ("bc", test_list_to_string(list));
+  ASSERT_EQ("b", list.pop_front());
+  ASSERT_EQ("c", test_list_to_string(list));
+  ASSERT_EQ("c", list.pop_front());
+  ASSERT_EQ("", test_list_to_string(list));
+  ASSERT_TRUE(list.pop_front() == nullptr);
+  list.push_back("r");
+  ASSERT_EQ("r", test_list_to_string(list));
+  ASSERT_EQ("r", list.pop_front());
+  ASSERT_TRUE(list.pop_front() == nullptr);
+}

tests/dlfcn_test.cpp

@@ -62,10 +62,9 @@ TEST(dlfcn, dlsym_in_self) {
   ASSERT_EQ(0, dlclose(self));
 }
 
-#if !defined(__LP64__)
-// Current compiler/static linker used for aarch64
-// platform optimizes LOCAL PROTECTED symbol
-// in libtest_local_symbol.so out of existence
+#if defined(__arm__)
+// This seems to be working only for arm.
+// Others platforms optimize LOCAL PROTECTED symbols.
 TEST(dlfcn, dlsym_local_symbol) {
   void* handle = dlopen("libtest_local_symbol.so", RTLD_NOW);
   ASSERT_TRUE(handle != NULL);
@@ -78,9 +77,23 @@ TEST(dlfcn, dlsym_local_symbol) {
   f = reinterpret_cast<uint32_t (*)(void)>(dlsym(handle, "dlsym_local_symbol_get_taxicab_number_using_dlsym"));
   ASSERT_TRUE(f != NULL);
   ASSERT_EQ(1729U, f());
+  dlclose(handle);
 }
 #endif
 
+TEST(dlfcn, dlsym_with_dependencies) {
+  void* handle = dlopen("libtest_with_dependency.so", RTLD_NOW);
+  ASSERT_TRUE(handle != NULL);
+  dlerror();
+  // This symbol is in DT_NEEDED library.
+  void* sym = dlsym(handle, "getRandomNumber");
+  ASSERT_TRUE(sym != NULL);
+  int (*fn)(void);
+  fn = reinterpret_cast<int (*)(void)>(sym);
+  EXPECT_EQ(4, fn());
+  dlclose(handle);
+}
+
 TEST(dlfcn, dlopen_noload) {
   void* handle = dlopen("libtest_simple.so", RTLD_NOW | RTLD_NOLOAD);
   ASSERT_TRUE(handle == NULL);

tests/libs/Android.mk

@@ -101,6 +101,19 @@ build_type := target
 build_target := SHARED_LIBRARY
 include $(TEST_PATH)/Android.build.mk
 
+# -----------------------------------------------------------------------------
+# Library with dependency used by dlfcn tests
+# -----------------------------------------------------------------------------
+libtest_with_dependency_src_files := \
+    dlopen_testlib_simple.cpp
+
+libtest_with_dependency_shared_libraries := libdlext_test
+
+module := libtest_with_dependency
+build_type := target
+build_target := SHARED_LIBRARY
+include $(TEST_PATH)/Android.build.mk
+
 # -----------------------------------------------------------------------------
 # Library used to test local symbol lookup
 # -----------------------------------------------------------------------------