This is an archive of the discontinued LLVM Phabricator instance.

lld: Fix initial Mach-O load commands size calculation omitting LC_FUNCTION_STARTS
ClosedPublic

Authored by comex on Feb 11 2019, 4:07 PM.

Details

Reviewers
pete
lhames
ruiu
Summary

The Mach-O writer calculates the size of load commands multiple times.

First, Util::assignAddressesToSections() (in MachONormalizedFileFromAtoms.cpp)
calculates the size using headerAndLoadCommandsSize() (in
MachONormalizedFileBinaryWriter.cpp), which creates a temporary MachOFileLayout
for the NormalizedFile, only to retrieve its headerAndLoadCommandsSize. Later,
writeBinary() (in MachONormalizedFileBinaryWriter.cpp) creates a new layout and
uses the offsets from that layout to actually write out everything in the
NormalizedFile.

But the NormalizedFile changes between the first computation and the second.
When Util::assignAddressesToSections is called, file.functionStarts is always
empty because Util::addFunctionStarts has not yet been called. Yet
MachOFileLayout decides whether to include a LC_FUNCTION_STARTS command based
on whether file.functionStarts is nonempty. Therefore, the initial computation
always omits it.

Because padding for the __TEXT segment (to make its size a multiple of the page
size) is added between the load commands and the first section, LLD still
generates a valid binary as long as the amount of padding happens to be large
enough to fit LC_FUNCTION_STARTS command, which it usually is.

However, it's easy to reproduce the issue by adding a section of a precise
size. Given foo.c:

__attribute__((section("__TEXT,__foo")))
char foo[0xd78] = {0};

Run:

clang -dynamiclib -o foo.dylib foo.c -fuse-ld=lld -install_name /usr/lib/foo.dylib
otool -lvv foo.dylib

This should produce:

truncated or malformed object (offset field of section 1 in LC_SEGMENT_64
command 0 not past the headers of the file)

This commit:

  • Changes MachOFileLayout to always assume LC_FUNCTION_STARTS is present for the initial computation, as long as generating LC_FUNCTION_STARTS is enabled. It would be slightly better to check whether there are actually any functions, since no LC_FUNCTION_STARTS will be generated if not, but it doesn't cause a problem if the initial computation is too high.
  • Adds a test.
  • Adds an assert in MachOFileLayout::writeSectionContent() that we are not writing section content into the load commands region (which would happen if the offset was calculated too low due to the initial load commands size calculation being too low).
  • Adds an assert in MachOFileLayout::writeLoadCommands to validate a similar situation where two size-of-load-commands computations are expected to be equivalent.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

comex created this revision.Feb 11 2019, 4:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2019, 4:07 PM
comex added a comment.Mar 25 2019, 8:57 PM

Second ping.

Hi Comex,

I'm out on vacation this week, but can review this next week when I'm back in the office.

Cheers,
Lang.

lhames accepted this revision.Apr 14 2019, 10:09 AM

Hi Comex,

Sorry for the belated review. This looks great! Thank you very much for the detailed analysis and fix. Please go ahead and commit if you have access, or let me know if you do not and I can commit it on your behalf.

Cheers,
Lang.

This revision is now accepted and ready to land.Apr 14 2019, 10:09 AM
comex added a comment.Apr 16 2019, 4:49 PM

Hi,

Thanks for the review. I don't have commit access.

lhames closed this revision.Apr 25 2019, 12:23 PM

Looks like Rui already committed in r358545. Thanks Rui!