[llvm-install-name-tool] Error on non-Mach-O binaries

[keith]

Previously if you passed an ELF binary it would be silently copied with no changes.

[llvmbot]

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

Author: Keith Smiley (keith)

Changes

Previously if you passed an ELF binary it would be silently copied with no changes.

---

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

2 Files Affected:

• (added) llvm/test/tools/llvm-objcopy/MachO/install-name-tool.test (+26)
• (modified) llvm/tools/llvm-objcopy/ObjcopyOptions.cpp (+11)

diff --git a/llvm/test/tools/llvm-objcopy/MachO/install-name-tool.test b/llvm/test/tools/llvm-objcopy/MachO/install-name-tool.test
new file mode 100644
index 00000000000000..fb4cd4ae9f5e28
--- /dev/null
+++ b/llvm/test/tools/llvm-objcopy/MachO/install-name-tool.test
@@ -0,0 +1,26 @@
+## This test checks general llvm-install-name-tool behavior
+
+# RUN: yaml2obj %s -o %t
+
+## Passing a non-Mach-O binary
+# RUN: not llvm-install-name-tool -add_rpath foo %t 2>&1 | FileCheck %s --check-prefix=NON_MACH_O
+
+# NON_MACH_O: llvm-install-name-tool: error: input file: {{.*}} is not a Mach-O file
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .bss
+    Type:            SHT_NOBITS
+    Flags:           [ SHF_ALLOC ]
+    AddressAlign:    0x0000000000000010
+    Size:            64
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x0000000000000010
+    Content:         "00000000"
diff --git a/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp b/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
index 70e85460d3df0d..929ccbeace64f4 100644
--- a/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
+++ b/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
@@ -15,6 +15,7 @@
 #include "llvm/ObjCopy/CommonConfig.h"
 #include "llvm/ObjCopy/ConfigManager.h"
 #include "llvm/ObjCopy/MachO/MachOConfig.h"
+#include "llvm/Object/Binary.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/CRC.h"
@@ -1242,6 +1243,16 @@ objcopy::parseInstallNameToolOptions(ArrayRef<const char *> ArgsArr) {
   Config.InputFilename = Positional[0];
   Config.OutputFilename = Positional[0];
 
+  Expected<llvm::object::OwningBinary<llvm::object::Binary>> BinaryOrErr =
+      llvm::object::createBinary(Config.InputFilename);
+  if (!BinaryOrErr)
+    return createFileError(Config.InputFilename, BinaryOrErr.takeError());
+  auto *Binary = (*BinaryOrErr).getBinary();
+  if (!Binary->isMachO() && !Binary->isMachOUniversalBinary())
+    return createStringError(errc::invalid_argument,
+                             "input file: %s is not a Mach-O file",
+                             Config.InputFilename.str().c_str());
+
   DC.CopyConfigs.push_back(std::move(ConfigMgr));
   return std::move(DC);
 }

[jh7370]

I forget, does this invocation just normally replace %t inline, or is an output file created? If an output file could be created, it might be worth validating that the file doesn't exist afterwards (you should make sure to delete it beforehand, in case one is left over from a previous run).

[keith]

this tool overwrites the passed output, so it has to exist before and still will after

[keith]

Thanks for the review! I addressed your feedback

[jh7370]

LGTM.

Please try to avoid force pushes in the future, as it makes the reviewing experience worse (and consequently is against the LLVM guidelines). If you need to rebase, you can do a merge, but in general this isn't necessary.

[jh7370]

Pre-merge checks are failing on Windows. I don't immediately see the issue though :/

[keith]

the issue is i was validating the executable name and not accounting for .exe, fixed by removing it from the assertion

[jh7370]

The fix LGTM, but probably best to let the pre-merge check finish before merging this.