Skip to content

[llvm-objcopy] Always update indirectsymoff in MachO #117726

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 29, 2024

Conversation

RIscRIpt
Copy link
Member

@RIscRIpt RIscRIpt commented Nov 26, 2024

Let's say we've run llvm-strip over some MachO. The resulting MachO became smaller and there are no indirect symbols anymore.

Then according to previous code we would not update indirectsymoff field. This would lead to MachOWriter::totalSize() report size that is larger than the new MachO. As a result we would get MachO that has zero bytes past contents of the very last load command.

Codesign has a strict check that size of MachO file must be equal to

lastLoadCommand.offset + lastLoadCommand.size

If this is not satisfied codesign reports the following error

main executable failed strict validation

Fixes #117723

@llvmbot
Copy link
Member

llvmbot commented Nov 26, 2024

@llvm/pr-subscribers-llvm-binary-utilities

Author: Richard Dzenis (RIscRIpt)

Changes

Let's say we've run llvm-strip over some MachO. The resulting MachO became smaller and there are no indirect symbols anymore.

Then according to previous code we would not update indirectsymoff field. This would lead to MachOWriter::totalSize() report size that is larger than the new MachO. As a result we would get MachO that has zero bytes past contents of the very last load command.

Codesign has a strict check that size of MachO file must be equal to

lastLoadCommand.offset + lastLoadCommand.size

If this is not satisfied codesign reports the following error

main executable failed strict validation

Full diff: https://github.com/llvm/llvm-project/pull/117726.diff

1 Files Affected:

  • (modified) llvm/lib/ObjCopy/MachO/MachOLayoutBuilder.cpp (+4-5)
diff --git a/llvm/lib/ObjCopy/MachO/MachOLayoutBuilder.cpp b/llvm/lib/ObjCopy/MachO/MachOLayoutBuilder.cpp
index a3d4ba3a94f7ac..551ff4501a66fb 100644
--- a/llvm/lib/ObjCopy/MachO/MachOLayoutBuilder.cpp
+++ b/llvm/lib/ObjCopy/MachO/MachOLayoutBuilder.cpp
@@ -361,11 +361,10 @@ Error MachOLayoutBuilder::layoutTail(uint64_t Offset) {
         return createStringError(llvm::errc::not_supported,
                                  "shared library is not yet supported");
 
-      if (!O.IndirectSymTable.Symbols.empty()) {
-        MLC.dysymtab_command_data.indirectsymoff = StartOfIndirectSymbols;
-        MLC.dysymtab_command_data.nindirectsyms =
-            O.IndirectSymTable.Symbols.size();
-      }
+      MLC.dysymtab_command_data.nindirectsyms =
+          O.IndirectSymTable.Symbols.size();
+      MLC.dysymtab_command_data.indirectsymoff =
+          O.IndirectSymTable.Symbols.empty() ? 0 : StartOfIndirectSymbols;
 
       updateDySymTab(MLC);
       break;

@jh7370
Copy link
Collaborator

jh7370 commented Nov 26, 2024

Thanks for the fix. Please add/update a test case to cover this behaviour.

As this is fixing an issue, please add "Fixes: #XXXXXX" (where XXXXXX is the issue number) to the PR description and remove the reference from the PR title.

Please could you add some reviewers who have been involved in modifying the Mach-O code in llvm-objcopy, either officially as reviewers, or by pinging them in the comments, depending on whether you have commit access or not.

@RIscRIpt RIscRIpt changed the title [llvm-objcopy] Always update indirectsymoff in MachO (#117723) [llvm-objcopy] Always update indirectsymoff in MachO Nov 26, 2024
Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test case for a binary that has 0 and >0 indirect symbols.

@RIscRIpt
Copy link
Member Author

Please add/update a test case to cover this behaviour.

I find it difficult generating a MachO reliably so that all the nuances are preserved to observe the desired effect.

I try to investigate how to craft such MachO if reviewers think I should add a regression test.

Please could you add some reviewers who have been involved in modifying the Mach-O code in llvm-objcopy, either officially as reviewers, or by pinging them in the comments, depending on whether you have commit access or not.

As far as I can see @nuta who was the author of this file was not contributing to llvm-project lately (as per git log --author=nuta), so I've added people who were reviewing his changes.

@compnerd
Copy link
Member

I find it difficult generating a MachO reliably so that all the nuances are preserved to observe the desired effect.

Take a look at obj2yaml - that will allow you to generate the code once and then trim down the MachO binary to the shape that we need for the test.

I try to investigate how to craft such MachO if reviewers think I should add a regression test.

Yes, please add the regression tests for this.

@alexander-shaposhnikov
Copy link
Collaborator

Thanks for the fix, i second @compnerd's suggestions

@RIscRIpt
Copy link
Member Author

Take a look at obj2yaml - that will allow you to generate the code once and then trim down the MachO binary to the shape that we need for the test.

I didn't know yaml2obj exists, thanks!

Yes, please add the regression tests for this.

I've added a very small test, please see if it's enough.

@RIscRIpt RIscRIpt requested a review from compnerd November 27, 2024 08:11
Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it would be possible to add a test case where there are indirect symbols (or is there an existing test case for that?)

@RIscRIpt
Copy link
Member Author

RIscRIpt commented Nov 27, 2024

Do you think it would be possible to add a test case where there are indirect symbols (or is there an existing test case for that?)

There are some:

❯ cd llvm/test/tools/llvm-objcopy/MachO
❯ rg "indirectsymoff:\\s*[1-9]"
linkedit-order-1.test
254:    indirectsymoff:  49424

Inputs/strip-all-with-codesignature.yaml
194:    indirectsymoff:  8424

linkedit-order-2.test
198:    indirectsymoff:  33000

symtbl-zero-indirectsymoff.test
39:    indirectsymoff:  42

remove-lc-index-update.test
194:    indirectsymoff:  8264

symbol-table.test
329:    indirectsymoff:  8480

Edit: most notably symbol-table.test, where different kinds of symbols are checked, including one indirect symbol.

@RIscRIpt RIscRIpt requested a review from compnerd November 27, 2024 17:00
@RIscRIpt RIscRIpt requested a review from compnerd November 27, 2024 21:04
@compnerd
Copy link
Member

@alexander-shaposhnikov - any additional feedback?

@RIscRIpt
Copy link
Member Author

Many thanks for the review! If there would be no additional feedback, I'll merge it in a couple of days, when I have more time to be online in case of test failures in main.

Copy link
Collaborator

@alexander-shaposhnikov alexander-shaposhnikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG

Let's say we've run llvm-strip over some MachO. The resulting MachO
became smaller and there are no indirect symbols anymore.

Then according to previous code we would not update indirectsymoff
field. This would lead to `MachOWriter::totalSize()` report size that is
larger than the new MachO. As a result we would get MachO that has zero
bytes past contents of the very last load command.

Codesign has a strict check that size of MachO file must be equal to

    lastLoadCommand.offset + lastLoadCommand.size

If this is not satisfied codesign reports the following error

    main executable failed strict validation

Fixes llvm#117723
@RIscRIpt RIscRIpt merged commit 9c97aa5 into llvm:main Nov 29, 2024
8 checks passed
@RIscRIpt RIscRIpt deleted the 117723-fix-objcopy branch December 1, 2024 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Artificial MachO binary processed by llvm-objcopy/strip is not accepted by codesign
6 participants