Closed Bug 1081034 Opened 5 years ago Closed 5 years ago

Read symbols from libc.so directly

Categories

(Core :: mozglue, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(4 files, 1 obsolete file)

Bug 791419 happened because on some devices, some libc.so functions were weak symbols, and dlsym doesn't resolve such symbols.

Bug 1077384 initiated the move to resolving system library symbols by reading their symbol table directly instead of using dlsym.

This bug goes one step further, adding libc.so to the scheme, allowing to address the issue from bug 791419 without wrapping symbols.
(Taking baby steps because doing it for more libraries requires more work ; libc.so is easy to find ; finding other system libraries requires DT_DEBUG data that is not 100% guaranteed to be there, and better coordination with the system linker too. libc can, like libmozglue, be bruteforced)
Depends on: 1078837
The new class is kind of like SystemElf, but using our linker's own symbol
resolution. This also adds some initialization from ELF program headers that
weren't done previously for self_elf, as well as registration as for CustomElf
instances.
Attachment #8504688 - Flags: review?(nfroyd)
The new class is kind of like SystemElf, but using our linker's own symbol
resolution. This also adds some initialization from ELF program headers that
weren't done previously for self_elf, as well as registration as for CustomElf
instances.
Attachment #8504696 - Flags: review?(nfroyd)
Attachment #8504688 - Attachment is obsolete: true
Attachment #8504688 - Flags: review?(nfroyd)
This allows to resolve weak symbols from some Android device's libc that
dlsym() won't. This is effectively an alternative fix to bug 791419, without
requiring wrapping symbols.
Attachment #8504698 - Flags: review?(nfroyd)
This effectively backs out bug 791419, a part of bug 850332, and bug 1001703.
Attachment #8504701 - Flags: review?(nfroyd)
Comment on attachment 8504698 [details] [diff] [review]
part 3 - Resolve libc symbols with our linker

Review of attachment 8504698 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/linker/ElfLoader.cpp
@@ +512,2 @@
>    self_elf = nullptr;
> +  libs = nullptr;

typo here.
Attachment #8504695 - Flags: review?(nfroyd) → review+
Comment on attachment 8504696 [details] [diff] [review]
part 2 - Move initialization of self_elf to its own separate class

Review of attachment 8504696 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/linker/BaseElf.cpp
@@ +111,5 @@
> +#endif
> +
> +  Array<Phdr> phdrs(reinterpret_cast<const char *>(ehdr) + ehdr->e_phoff,
> +                    ehdr->e_phnum);
> +  for (auto phdr = phdrs.begin(); phdr < phdrs.end(); ++phdr) {

I thought I had seen this code somewhere before.  Too bad it can't be merged with the loop in CustomElf.cpp.

@@ +137,5 @@
> +   * is either prelinked or a non-PIE executable. The former case is not
> +   * possible, because base_addr would be nullptr and the mincore test above
> +   * would already have made us return.
> +   * For a non-PIE executable, PT_LOADs contain absolute addresses, so in
> +   * practice,* this means min_vaddr should be equal to base_addr. max_vaddr

Looks like you have a * in the middle of this from re-wrapping?

@@ +168,5 @@
> +
> +  DEBUG_LOG("LoadedElf::Create(\"%s\", %p) = %p", path, base_addr,
> +    static_cast<void *>(elf));
> +
> +  ElfLoader::Singleton.Register(elf);

I noticed that CustomElf has this Register call much earlier, which seems like a bug in CustomElf...
Attachment #8504696 - Flags: review?(nfroyd) → review+
Comment on attachment 8504698 [details] [diff] [review]
part 3 - Resolve libc symbols with our linker

Review of attachment 8504698 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice how little code this requires now.
Attachment #8504698 - Flags: review?(nfroyd) → review+
Attachment #8504701 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #8)
> I noticed that CustomElf has this Register call much earlier, which seems
> like a bug in CustomElf...

That's actually required for CustomElf, at least at the moment, because of the segfault handling. Reading the PT_DYNAMIC section requires the lib to be registered for the segfault handler to find it and decompress the missing data. What /could/ be done is to make all that code call mappable->ensure itself instead of relying on the segfault handler, but that's for a separate bug.
You need to log in before you can comment on or make changes to this bug.