Skip to content

Commit

Permalink
Reland "[Fuchsia] Use memory offsets instead of file offsets for ELF …
Browse files Browse the repository at this point in the history
…headers."

This is a reland of 224e8c2

The previous CL broke on ChromeOS test bots.

This reland CL adds the following fixes:
* Enable the ReadElfLibraryName unit test on Fuchsia.
* Read ELF data from the linked module's memory instead of from the filesystem
  directly, so that p_vaddr is valid.
* Switch to interpreting SYMTAB pointers as offsets instead of memory
  addresses under Fuchsia and Android, which use -fPIE executables.
* Modify test code to query for the exported MallocWrapper() symbol.
  The previous implementation queried for "_init", which doesn't exist for
  built Fuchsia shared libraries.


Original change's description:
> [Fuchsia] Use memory offsets instead of file offsets for ELF headers.
>
> The ELF reader code uses file offsets for computing locations of data
> in ELF headers. It is technically incorrect but generally works on
> Intel without problems. However, ARM has wider alignment boundaries
> which results in the computation and access of incorrect memory
> addresses, producing page faults in certain circumstances.
>
> The bug is fixed by using p_vaddr instead of p_offset.
>
> Bug: 941279
> Change-Id: Ia88dc58411dfe046f882a94cea0e80e8ec525957
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1518226
> Reviewed-by: Wez <wez@chromium.org>
> Commit-Queue: Kevin Marshall <kmarshall@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#640165}

Bug: 941279
Change-Id: Id17ce92eeb0e97114b0b131ab746b4a4a15be36e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1520209
Reviewed-by: Wez <wez@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Commit-Queue: Kevin Marshall <kmarshall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#643524}
  • Loading branch information
Kevin Marshall authored and Commit Bot committed Mar 22, 2019
1 parent 23cde2a commit c26b4a5
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 27 deletions.
11 changes: 5 additions & 6 deletions base/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -2792,11 +2792,7 @@ test("base_unittests") {
}
}

if (is_linux) {
if (is_desktop_linux) {
sources += [ "nix/xdg_util_unittest.cc" ]
}

if (is_fuchsia || is_linux) {
sources += [ "debug/elf_reader_unittest.cc" ]

deps += [ "//base/test:malloc_wrapper" ]
Expand All @@ -2811,6 +2807,10 @@ test("base_unittests") {
}
}

if (is_desktop_linux) {
sources += [ "nix/xdg_util_unittest.cc" ]
}

if (!use_glib) {
sources -= [ "message_loop/message_pump_glib_unittest.cc" ]
}
Expand All @@ -2822,7 +2822,6 @@ test("base_unittests") {

if (is_fuchsia) {
sources += [
"debug/elf_reader_unittest.cc",
"files/dir_reader_posix_unittest.cc",
"files/file_descriptor_watcher_posix_unittest.cc",
"fuchsia/file_utils_unittest.cc",
Expand Down
19 changes: 13 additions & 6 deletions base/debug/elf_reader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "base/containers/span.h"
#include "base/sha1.h"
#include "base/strings/safe_sprintf.h"
#include "build/build_config.h"

// NOTE: This code may be used in crash handling code, so the implementation
// must avoid dynamic memory allocation or using data structures which rely on
Expand Down Expand Up @@ -77,9 +78,9 @@ size_t ReadElfBuildId(const void* elf_mapped_base,
continue;

// Look for a NT_GNU_BUILD_ID note with name == "GNU".
const void* section_end = elf_base + header.p_offset + header.p_memsz;
const void* section_end = elf_base + header.p_vaddr + header.p_memsz;
const Nhdr* current_note =
reinterpret_cast<const Nhdr*>(elf_base + header.p_offset);
reinterpret_cast<const Nhdr*>(elf_base + header.p_vaddr);
bool found = false;
while (current_note < section_end) {
if (current_note->n_type == NT_GNU_BUILD_ID) {
Expand Down Expand Up @@ -141,16 +142,22 @@ Optional<StringPiece> ReadElfLibraryName(const void* elf_mapped_base) {
// SONAME offsets, which are used to compute the offset of the library
// name string.
const Dyn* dynamic_start =
reinterpret_cast<const Dyn*>(elf_base + header.p_offset);
reinterpret_cast<const Dyn*>(elf_base + header.p_vaddr);
const Dyn* dynamic_end = reinterpret_cast<const Dyn*>(
elf_base + header.p_offset + header.p_memsz);
elf_base + header.p_vaddr + header.p_memsz);
Word soname_strtab_offset = 0;
const char* strtab_addr = 0;
for (const Dyn* dynamic_iter = dynamic_start; dynamic_iter < dynamic_end;
++dynamic_iter) {
if (dynamic_iter->d_tag == DT_STRTAB) {
strtab_addr =
dynamic_iter->d_un.d_ptr + reinterpret_cast<const char*>(elf_base);
#if defined(OS_FUCHSIA) || defined(OS_ANDROID)
// Fuchsia and Android executables are position-independent, so treat
// pointers in the ELF header as offsets into the address space instead
// of absolute addresses.
strtab_addr = (size_t)dynamic_iter->d_un.d_ptr + (const char*)elf_base;
#else
strtab_addr = (const char*)dynamic_iter->d_un.d_ptr;
#endif
} else if (dynamic_iter->d_tag == DT_SONAME) {
soname_strtab_offset = dynamic_iter->d_un.d_val;
}
Expand Down
27 changes: 14 additions & 13 deletions base/debug/elf_reader_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <string>

#include "base/files/memory_mapped_file.h"
#include "base/native_library.h"
#include "base/strings/string_util.h"
#include "build/build_config.h"
#include "testing/gtest/include/gtest/gtest.h"
Expand Down Expand Up @@ -55,41 +56,41 @@ TEST(ElfReaderTest, ReadElfBuildIdLowercase) {
}
#endif // defined(OFFICIAL_BUILD) || defined(OS_FUCHSIA)

#if !defined(OS_FUCHSIA)
TEST(ElfReaderTest, ReadElfLibraryName) {
#if defined(OS_ANDROID)
// On Android the library loader memory maps the full so file.
const char kLibraryName[] = "lib_base_unittests__library";
const void* addr = &__executable_start;
#else
const char kLibraryName[] = MALLOC_WRAPPER_LIB;
// On Linux the executable does not contain soname and is not mapped till
// dynamic segment. So, use malloc wrapper so file on which the test already
// depends on.
const char kLibraryName[] = MALLOC_WRAPPER_LIB;
// Find any symbol in the loaded file.
void* handle = dlopen(kLibraryName, RTLD_NOW | RTLD_LOCAL);
const void* init_addr = dlsym(handle, "_init");
//
NativeLibraryLoadError error;
NativeLibrary library =
LoadNativeLibrary(base::FilePath(kLibraryName), &error);
void* init_addr =
GetFunctionPointerFromNativeLibrary(library, "MallocWrapper");

// Use this symbol to get full path to the loaded library.
Dl_info info;
int res = dladdr(init_addr, &info);
ASSERT_NE(0, res);
std::string filename(info.dli_fname);
EXPECT_FALSE(filename.empty());
EXPECT_NE(std::string::npos, filename.find(kLibraryName));

// Memory map the so file and use it to test reading so name.
MemoryMappedFile file;
ASSERT_TRUE(file.Initialize(FilePath(filename)));
const void* addr = file.data();
const void* addr = info.dli_fbase;
#endif

auto name = ReadElfLibraryName(addr);
ASSERT_TRUE(name);
EXPECT_NE(std::string::npos, name->find(kLibraryName))
<< "Library name " << *name << " doesn't contain expected "
<< kLibraryName;

#if !defined(OS_ANDROID)
UnloadNativeLibrary(library);
#endif
}
#endif // !defined(OS_FUCHSIA)

} // namespace debug
} // namespace base
2 changes: 2 additions & 0 deletions base/test/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,9 @@ if (is_linux) {
]
}
}
}

if (is_fuchsia || is_linux) {
shared_library("malloc_wrapper") {
testonly = true
sources = [
Expand Down
5 changes: 3 additions & 2 deletions base/test/malloc_wrapper.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
#define MALLOC_WRAPPER_EXPORT __attribute__((visibility("default")))
#endif // defined(WIN32)

// Calls malloc directly.
MALLOC_WRAPPER_EXPORT void* MallocWrapper(size_t size);
// Calls malloc directly. Defined as a C function so that the function can be
// easily referenced by dlsym() without complications from C++ name mangling.
extern "C" MALLOC_WRAPPER_EXPORT void* MallocWrapper(size_t size);

#endif // BASE_TEST_MALLOC_WRAPPER_H_

0 comments on commit c26b4a5

Please sign in to comment.