Page MenuHomePhabricator

[MACH-O] Fix the ASM code generated for __stub_helpers section
ClosedPublic

Authored by ltr_ on Jul 13 2017, 3:06 PM.

Details

Summary

I discovered that lld for darwin is generating the wrong code for lazy bindings in the __stub_helper section (at least for osx 10.12). This is the way i can reproduce this problem, using this program:

#include <stdio.h>

int main(int argc, char **argv) {
    printf("C: printf!\n");
    puts("C: puts!\n");
    return 0;
}

Then I link it using i have tested it in 3.9, 4.0 and 4.1 versions:

$ clang -c hello.c
$ lld -flavor darwin hello.o -o h1  -lc

When i execute the binary h1 the system gives me the following error:

C: printf!
dyld: lazy symbol binding failed: BIND_OPCODE_SET_SEGMENT_AND_OFFSET_ULEB
has segment 4 which is too large (0..3)
dyld: BIND_OPCODE_SET_SEGMENT_AND_OFFSET_ULEB has segment 4 which is too
large (0..3)
Trace/BPT trap: 5

Investigating the code, it seems that the problem is that the asm code generated in the file StubPass.cpp, specifically in the line 323,when it adds, what it seems an arbitrary number (12) to the offset into the lazy bind opcodes section, but it should be calculated depending on the MachONormalizedFileBinaryWrite::lazyBindingInfo result.

I confirmed this bug by patching the code manually in the binary and writing the right offset in the asm code (__stub_helper).

This patch fixes the content of the atom that contains the assembly code when the offset is known.

Diff Detail

Repository
rL LLVM

Event Timeline

ltr_ created this revision.Jul 13 2017, 3:06 PM
ltr_ retitled this revision from Fix the ASM code generated for __stub_helpers section to [MACH-O] Fix the ASM code generated for __stub_helpers section.Jul 13 2017, 3:06 PM
ltr_ edited the summary of this revision. (Show Details)
ltr_ added a reviewer: lhames.Jul 14 2017, 8:21 AM
hans added a reviewer: ruiu.Jul 26 2017, 9:13 AM
hans added a subscriber: hans.

Rui: can you help find a reviewer for this? It was requested for 5.0.

ruiu edited edge metadata.Jul 26 2017, 1:52 PM

I'm not familiar with the code and the behavior of the macOS loader. Lang, could you review?

hans added a comment.Jul 28 2017, 2:22 PM
In D35387#822016, @ruiu wrote:

I'm not familiar with the code and the behavior of the macOS loader. Lang, could you review?

Lang: ping?

Rui: in case Lang is ooo, do you know anyone else who might be able to review this?

ruiu added a comment.Jul 31 2017, 1:34 AM

It is probably me, but I don't believe this is a new bug in 5.0.

lhames edited edge metadata.Jul 31 2017, 10:44 AM

Sorry - looking in to this now...

I've made a couple of notes, and it would be great to get a test-case for this too, but otherwise it's looking good. Thanks so much for working on this!

lib/ReaderWriter/MachO/ArchHandler_arm.cpp
70–73 ↗(On Diff #106543)

Given that we're returning lazyImmediateLocation on all platforms, can we just use this as the default implementation in ArchHandler::lazyImmediateLocationKind()?

lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp
1489 ↗(On Diff #106543)

It would be good to add a comment for this function to explain what it's doing.

1503–1514 ↗(On Diff #106543)

It would be good to create the target section outside the loop so that we can apply all patches in one go -- right now this looks like it will eat up a lot of memory maintaining partially-patched copies of the section that are no longer referenced.

ltr_ added inline comments.Aug 7 2017, 1:52 PM
lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp
1503–1514 ↗(On Diff #106543)

Ill do this tomorrow.
+ the comments for this function

Bump.

  • Can we have this even without tests?
  • Can we have this in 5.0.0?
ruiu added a comment.Aug 22 2017, 5:10 PM

Lang,

What do you think of this patch? If you think this is okay to commit and should be backported to 5.0, you might want to do that, since 5.0 release is happening really soon.

If you are busy, you can just give us a green light, so that I (or maybe Andrew Kelley) can do that for you.

lhames accepted this revision.Aug 23 2017, 11:13 AM

It's probably better to have without tests than to not have at all.

ltr_ - Do you have commit access?

This revision is now accepted and ready to land.Aug 23 2017, 11:13 AM
ltr_ added a comment.Aug 23 2017, 11:22 AM

No i do not have commit access. anyways Ill be unable to do a commit right now until the weekend =/

hans added a comment.Aug 24 2017, 10:24 AM
In D35387#849537, @ruiu wrote:

Lang,

What do you think of this patch? If you think this is okay to commit and should be backported to 5.0, you might want to do that, since 5.0 release is happening really soon.

If you are busy, you can just give us a green light, so that I (or maybe Andrew Kelley) can do that for you.

It sounds like Lang ok'd it, and ltr_ isn't around to commit at the moment.

Rui, would you mind committing this so we get it into the release?

This revision was automatically updated to reflect the committed changes.