[BinaryFormat] Limit COFF file detection with unknown machine type

[sbc100]

Summary

When the machine type is zero (first two bytes) also check that we
have a non-zero number of sections (second two bytes). This avoids
mistaking any file that has two leaving zeros as a COFF file.

Fixes PR44683

[sbc100]

update comment

[merge_guards_bot]

Unit tests: fail. 62143 tests passed, 6 failed and 811 were skipped.

failed: LLVM.tools/llvm-readobj/COFF/file-headers.test
failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[merge_guards_bot]

Unit tests: fail. 62142 tests passed, 7 failed and 811 were skipped.

failed: LLVM.tools/llvm-readobj/COFF/file-headers.test
failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp
failed: libc++.std/thread/futures/futures_async/async.pass.cpp

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[efriedma]

llvm::identify_magic is not really a general-purpose file identification routine; it's specifically designed to sniff out the format of files which are interesting to LLVM tools, and it doesn't try very hard to figure out whether the input is valid. If it's returning file_magic::unknown, probably something has gone wrong elsewhere. That said, it would be nice to use more than two bytes to make a positive identification, if there's some other reliable indicator for a COFF object.

Are PE object files with IMAGE_FILE_MACHINE_UNKNOWN actually something that shows up in practice (besides the special case of "\0\0\xFF\xFF")? What would such a file contain? Can any LLVM tools do anything interesting with it?

If we're going to try to sniff out whether the COFF header is valid, should we do it for all coff_object files?

Is it reasonable to assume all COFF object files have at least one section? It's sort of degenerate, but it's possible to construct an empty object file.

[sbc100]

In D73502#1847393, @efriedma wrote:

llvm::identify_magic is not really a general-purpose file identification routine; it's specifically designed to sniff out the format of files which are interesting to LLVM tools, and it doesn't try very hard to figure out whether the input is valid. If it's returning file_magic::unknown, probably something has gone wrong elsewhere. That said, it would be nice to use more than two bytes to make a positive identification, if there's some other reliable indicator for a COFF object.

But tools like llvm-nm do end up using identify_magic to decide if they should decode an object. Running llvm-nm on ar archives is supposed to be robust against non-object files being in the archive I believe.

Are PE object files with IMAGE_FILE_MACHINE_UNKNOWN actually something that shows up in practice (besides the special case of "\0\0\xFF\xFF")? What would such a file contain? Can any LLVM tools do anything interesting with it?

It seem very unlikely that there are any uses for such an object.

If we're going to try to sniff out whether the COFF header is valid, should we do it for all coff_object files?

Yes, I thought that was what this change was doing but maybe it should happen elsewhere too.

Is it reasonable to assume all COFF object files have at least one section? It's sort of degenerate, but it's possible to construct an empty object file.

I think it is reasonable, although this change did uncover a test file that creates and emtpy object file from yaml. Note that in my experiments it is *not* possible create such an empty object file using clang or llvm tools.

[sbc100]

In the mean time, it looks like rust has worked around this issue upstream so I'm tempted to simply close this issue and abandon this change: https://github.com/rust-lang/rust/pull/66235

llvm/lib/BinaryFormat/Magic.cpp

if (startswith(Magic, "\0\0\xFF\xFF")) {
         return file_magic::coff_cl_gl_object;
       return file_magic::coff_import_library;
     }
     // Windows resource file
     if (Magic.size() >= sizeof(COFF::WinResMagic) &&
         memcmp(Magic.data(), COFF::WinResMagic, sizeof(COFF::WinResMagic)) == 0)
       return file_magic::windows_resource;
     // 0x0000 = COFF unknown machine type
-    if (Magic[1] == 0)
+    if (Magic[1] == 0) {
+      // We don't want to mis-identify files that simply start with 2 leading
+      // null bytes as a COFF object.  One such example is the rust.metadata.bin
+      // file which the rust toolchain embedds in ar archives. This file starts
+      // with 4 null bytes.  To work around this, also check the following two
+      // bytes which represet the number of COFF sections and shoudl be
+      // non-zero.
+      if (Magic[2] != 0 || Magic[3] != 0)
         return file_magic::coff_object;
+    }
     if (startswith(Magic, "\0asm"))
       return file_magic::wasm_object;
     break;
   }
 
   case 0x01:
     // XCOFF format
     if (startswith(Magic, "\x01\xDF"))

llvm/unittests/BinaryFormat/TestFileMagic.cpp

const char windows_resource[] =
     "\x00\x00\x00\x00\x020\x00\x00\x00\xff\xff\x00\x00\xff\xff\x00\x00";
 const char macho_dynamically_linked_shared_lib_stub[] =
     "\xfe\xed\xfa\xce........\x00\x00\x00\x09............";
 const char ms_dos_stub_broken[] = "\x4d\x5a\x20\x20";
 const char pdb[] = "Microsoft C/C++ MSF 7.00\r\n\x1a"
                    "DS\x00\x00\x00";
 const char tapi_file[] = "--- !tapi-tbd-v1\n";
 const char tapi_file_tbd_v1[] = "---\narchs: [";
+const char all_zeros[] = "\x00\x00\x00\x00";
 
 TEST_F(MagicTest, Magic) {
   struct type {
     const char *filename;
     const char *magic_str;
     size_t magic_str_len;
     file_magic magic;
   } types[] = {
#define DEFINE(magic) {#magic, magic, sizeof(magic), file_magic::magic}
       DEFINE(macho_kext_bundle),
       DEFINE(windows_resource),
       DEFINE(pdb),
       {"ms_dos_stub_broken", ms_dos_stub_broken, sizeof(ms_dos_stub_broken),
        file_magic::unknown},
       DEFINE(tapi_file),
       {"tapi_file_tbd_v1", tapi_file_tbd_v1, sizeof(tapi_file_tbd_v1),
        file_magic::tapi_file},
+      // A file containing all zeros should not be recognized as a valid object
+      {"all_zeros", all_zeros, sizeof(all_zeros), file_magic::unknown},
 #undef DEFINE
   };
 
   // Create some files filled with magic.
   for (type *i = types, *e = types + (sizeof(types) / sizeof(type)); i != e;
        ++i) {
     SmallString<128> file_pathname(TestDirectory);
     llvm::sys::path::append(file_pathname, i->filename);