1081034 - Read symbols from libc.so directly

Status: RESOLVED FIXED

(Core :: mozglue, defect)

Description

[Mike Hommey [:glandium]]

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.

Comment 1

[Mike Hommey [:glandium]]

(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)

Comment 2

[Mike Hommey [:glandium]]

Attachment: Move initialization of self_elf to its own separate class

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.

Comment 3

[Mike Hommey [:glandium]]

Attachment: part 1 - Move GetSymbolPtr, Contains and FindExidx from CustomElf to BaseElf

Comment 4

[Mike Hommey [:glandium]]

Attachment: part 2 - Move initialization of self_elf to its own separate class

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.

Comment 5

[Mike Hommey [:glandium]]

Attachment: part 3 - Resolve libc symbols with our linker

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.

Comment 6

[Mike Hommey [:glandium]]

Attachment: part 4 - Remove wrapping for weak libc symbols

This effectively backs out bug 791419, a part of bug 850332, and bug 1001703.

Comment 7

[Mike Hommey [:glandium]]

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.

Comment 8

[Nathan Froyd [:froydnj]]

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...

Comment 9

[Nathan Froyd [:froydnj]]

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.

Comment 10

[Mike Hommey [:glandium]]

(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.

Comment 11

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab905a93d0ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/349536e12ec0
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdf75d54f631
https://hg.mozilla.org/integration/mozilla-inbound/rev/0004a6330d53

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ab905a93d0ea
https://hg.mozilla.org/mozilla-central/rev/349536e12ec0
https://hg.mozilla.org/mozilla-central/rev/fdf75d54f631
https://hg.mozilla.org/mozilla-central/rev/0004a6330d53

Comment 13

[Mike Hommey [:glandium]]

Fixup for desktop:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0f3e4607276

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e0f3e4607276