[Archive] Don't throw away errors for malformed archive members

[jhenderson]

Summary

When adding an archive member with a problem, e.g. a new bitcode with an old archiver, containing an unsupported attribute, or an ELF file with a malformed symbol table, the archiver would throw away the error and simply add the member to the archive without any symbol entries. This meant that the resultant archive could be silently unusable when not using --whole-archive, and result in unexpected undefined symbols.

This change fixes this issue by addressing two FIXMEs and only throwing away not-an-object errors. However, this meant that some LLD tests which didn't need symbol tables and were using invalid members deliberately to test the linker's malformed input handling no longer worked, so this patch also stops the archiver from looking for symbols in an object if it doesn't require a symbol table, and updates the tests accordingly.

[jhenderson]

Fix test comment.

[grimar]

llvm/lib/Object/ArchiveWriter.cpp

381

At line 362 we call identify_magic(Buf.getBuffer()). I wonder if it will be cleaner to
add a new static method to SymbolicFile, e.g:

bool SymbolicFile::isSymbolicFile(file_magic Type);

and then to use it from here and also from SymbolicFile::createSymbolicFile,
instead of the code it has:

case file_magic::unknown:
case file_magic::archive:
case file_magic::coff_cl_gl_object:
case file_magic::macho_universal_binary:
case file_magic::windows_resource:
case file_magic::pdb:
case file_magic::minidump:
case file_magic::tapi_file:
  return errorCodeToError(object_error::invalid_file_type);

With that you'll don't need to handle error code in this way, what is probably better because
I think the general direction is to get rid of them in favor of Expected<> etc. What do you think?

llvm/test/Object/archive-unknown-filetype.test

2

Did you mean this?

[jhenderson]

llvm/test/Object/archive-unknown-filetype.test

2

I fixed this one already (think you must have missed my patch update).

[jhenderson]

Refactor to remove reliance on error code.

[grimar]

Minor nit. Otherwise looks good to me. Lets see what others think.

llvm/lib/Object/ArchiveWriter.cpp

363

I'd use const.

[rupprecht]

There's a build warning that needs to be fixed, but otherwise LG.

llvm/lib/Object/ArchiveWriter.cpp

499–501

nit: looks like tab characters were inserted, can you run clang-format?

llvm/lib/Object/SymbolicFile.cpp

47

Splitting up the switch leads to -Wswitch errors because now each switch statement is incomplete:

/home/rupprecht/src/llvm-project/llvm/lib/Object/SymbolicFile.cpp:47:11: warning: 8 enumeration values not handled in switch: 'unknown', 'archive', 'macho_universal_binary'... [-Wswitch]
  switch (Type) {
          ^
/home/rupprecht/src/llvm-project/llvm/lib/Object/SymbolicFile.cpp:97:11: warning: 23 enumeration values not handled in switch: 'bitcode', 'elf', 'elf_relocatable'... [-Wswitch]
  switch (Type) {
          ^

Normally the right fix is to add a default switch case to handle all the unused cases. If you're concerned about the list of file_magic::unknown, file_magic::archive, ... in each method going out of sync in the two definitions (and new enums accidentally being handled with the default case), you could define that list in a macro, e.g.

#define NOT_SYMBOLIC_FILES \
  case file_magic::unknown: \
  case file_magic::archive: \
  case file_magic::coff_cl_gl_object: \
...
  case file_magic::tapi_file

  switch (Type) {
    NOT_SYMBOLIC_FILES:
      return false;
    default:
      return true;
  }

(note no trailing : in the last case you can write NOT_SYMBOLIC_FILES: and not confused IDEs)

[MaskRay]

llvm/lib/Object/SymbolicFile.cpp

106

default:
  return true;

resolves a clang warning: Object/SymbolicFile.cpp:97:11: warning: 23 enumeration values not handled in switch: 'bitcode', 'elf', 'elf_relocatable'... [-Wswitch]

[rupprecht]

llvm/lib/Object/SymbolicFile.cpp

106

Yes, but see my other comment, because that is addressing a symptom instead of fixing the cause. Having an explicit switch in the previous switch ensures that a new file type added doesn't accidentally get included in the default switch branch.

[MaskRay]

Checked lld/test/ELF updates, GNU ar silently allows invalid 'symbolic' files. I think being more rigid in llvm-ar is fine.

An alternative to the new isSymbolicFile is by checking whether createSymbolicFile returns an ECError whose error_code (ECError::convertToErrorCode) is object_error::invalid_file_type.

[jhenderson]

In D88288#2295523, @MaskRay wrote:

Checked lld/test/ELF updates, GNU ar silently allows invalid 'symbolic' files. I think being more rigid in llvm-ar is fine.

An alternative to the new isSymbolicFile is by checking whether createSymbolicFile returns an ECError whose error_code (ECError::convertToErrorCode) is object_error::invalid_file_type.

That's what I originally did, but a) the code is not particularly clean, and b) it's brittle to changes in how the error is reported (relies on on the error code being set, rather than e.g. changing to a normal LLVM error). @grimar previously suggested to change this (see the inline comment).

llvm/lib/Object/ArchiveWriter.cpp

499–501

That's an unfortunate UI issue in Phabricator that was recently introduced. Code that has been indented now shows up this way. There are no tab characters here.

llvm/lib/Object/SymbolicFile.cpp

47

I don't particularly like macros. They are a pain to debug and not particularly readable, in my opinion. Maybe the way to resolve this is to flip the switch in isSymbolicFile on its head, so that any new file_magic types will return false (specified via a default label), until this function is modified. This means you wouldn't be able to create a symbolic file for them, which I don't think is unreasonable. This switch here then gains a default: llvm_unreachable() line, since it would only be reachable if the isSymbolicFile method returned true.

[jhenderson]

Invert isSymbolFIle switch, and add const.

[grimar]

llvm/lib/Object/SymbolicFile.cpp

51

I think the Content is always non-null here now?

[jhenderson]

llvm/lib/Object/SymbolicFile.cpp

51

Yeah, I think this can be simplified. I'm not going to have a chance to update the patch again today, but will fix in the commit, if you're otherwise happy.

It will become:

case file_magic::bitcode:
  return IRObjectFile::create(Object, *Context);

[grimar]

llvm/lib/Object/SymbolicFile.cpp

51

The rest looks fine to me. Please wait to see @MaskRay/@rupprecht opinions too

[MaskRay]

llvm/lib/Object/SymbolicFile.cpp

51

Context is always non-null. This is however a hidden fact only clear after I read all the callers..

llvm/test/Object/archive-unknown-filetype.test

6

Using echo something > %t can be more robust. touch %t does not update the file content. If %t preexists as a different file type, this may fail spuriously.

[rupprecht]

Requesting changes mostly because this is marked as accepted but I still see build errors; overall this looks fine though.

llvm/lib/Object/SymbolicFile.cpp

47–48

I'm still seeing -Wswitch errors:

/home/rupprecht/src/llvm-project/llvm/lib/Object/SymbolicFile.cpp:47:11: warning: 8 enumeration values not handled in switch: 'unknown', 'archive', 'macho_universal_binary'... [-Wswitch]
  switch (Type) {

51

I found this thread hard to reconcile with the code: IIUC, Context is only non-null *in this case statement*: if Context is null and this is a bitcode file, isSymbolicFile returns false, and an error is returned above, so we don't reach this line. However, if this is a wasm_object (just one example), isSymbolicFile returns true even if Context is null. Context can be null in this switch statement, but not this particular case.

I think it's fine to make that simplification, but with a comment that isSymbolicFile verifies that Context is non-null when the type is bitcode. Otherwise, the nullness of Context is very difficult to reason with -- the baseline is much easier to understand in that regard.

[jhenderson]

Add missing default; simplify bitcode object checking; add comment about Context being non-null; use echo instead of touch in the test.

llvm/lib/Object/SymbolicFile.cpp

47–48

Oops, sorry. Forgot to add the necessary default label.

[grimar]

LGTM

[rupprecht]

Thanks!

lld/test/ELF/invalid/data-encoding.test

 # REQUIRES: x86
 
 # The object in the archive has invalid data encoding.
 # Check we report this.
 
 # RUN: yaml2obj %s -o %t.o
-# RUN: llvm-ar rcs %t.a %t.o
+# RUN: llvm-ar rcS %t.a %t.o
 
 # RUN: not ld.lld --whole-archive %t.a -o /dev/null 2>&1 | FileCheck %s
 # CHECK: {{.*}}.a({{.*}}.o): corrupted ELF file: invalid data encoding
 
 --- !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATANONE
   Type:            ET_REL
   Machine:         EM_X86_64

lld/test/ELF/invalid/invalid-file-class.test

 # REQUIRES: x86
 # RUN: rm -rf %t && mkdir -p %t
 
 ## In this test, we check that able to report objects with
 ## invalid ELF class type. There are 2 cases we want to check:
 ## when the object is a regular input and when it is fetched from the archive.
 
 ## Create a broken ELF object with ELFCLASSNONE class using echo. We need to fill
 ## first 18 bytes which are used by LLD to check the object.
 ## 0x7f, 'E', 'L', 'F', ELFCLASSNONE(0), ELFDATA2LSB(1),
 ## EV_CURRENT(1), ELFOSABI_LINUX(3), <padding zero bytes>, ET_REL(1), EM_NONE(0)
 # RUN: echo -e -n "\x7f\x45\x4c\x46\x00\x01\x01\x03\x00\x00\x00\x00\x00\x00\x00\x00\x01\x00" > %t/invalid.o
 
-# RUN: llvm-ar --format=gnu cr %t/invalid-class.a %t/invalid.o
+# RUN: llvm-ar --format=gnu crS %t/invalid-class.a %t/invalid.o
 # RUN: not ld.lld -whole-archive %t/invalid-class.a -o /dev/null 2>&1 | FileCheck %s
 # CHECK: invalid-class.a(invalid.o): corrupted ELF file: invalid file class
 
 # RUN: not ld.lld %t/invalid.o -o /dev/null 2>&1 | FileCheck %s --check-prefix=OBJ
 # OBJ: invalid.o: corrupted ELF file: invalid file class

llvm/include/llvm/Object/SymbolicFile.h

createSymbolicFile(MemoryBufferRef Object) {
     return createSymbolicFile(Object, llvm::file_magic::unknown, nullptr);
   }
   static Expected<OwningBinary<SymbolicFile>>
   createSymbolicFile(StringRef ObjectPath);
 
   static bool classof(const Binary *v) {
     return v->isSymbolic();
   }
+
+  static bool isSymbolicFile(file_magic Type);
 };
 
 inline BasicSymbolRef::BasicSymbolRef(DataRefImpl SymbolP,
                                       const SymbolicFile *Owner)
     : SymbolPimpl(SymbolP), OwningObject(Owner) {}
 
 inline bool BasicSymbolRef::operator==(const BasicSymbolRef &Other) const {
   return SymbolPimpl == Other.SymbolPimpl;

llvm/lib/Object/ArchiveWriter.cpp

 static Expected<std::vector<unsigned>>
 getSymbols(MemoryBufferRef Buf, raw_ostream &SymNames, bool &HasObject) {
   std::vector<unsigned> Ret;
 
   // In the scenario when LLVMContext is populated SymbolicFile will contain a
   // reference to it, thus SymbolicFile should be destroyed first.
   LLVMContext Context;
   std::unique_ptr<object::SymbolicFile> Obj;
-  if (identify_magic(Buf.getBuffer()) == file_magic::bitcode) {
+
+  file_magic Type = identify_magic(Buf.getBuffer());
+  // Treat unsupported file types as having no symbols.
+  if (!object::SymbolicFile::isSymbolicFile(Type))
+    return Ret;
+  if (Type == file_magic::bitcode) {
     auto ObjOrErr = object::SymbolicFile::createSymbolicFile(
         Buf, file_magic::bitcode, &Context);
-    if (!ObjOrErr) {
-      // FIXME: check only for "not an object file" errors.
-      consumeError(ObjOrErr.takeError());
-      return Ret;
-    }
+    if (!ObjOrErr)
+      return ObjOrErr.takeError();
     Obj = std::move(*ObjOrErr);
   } else {
     auto ObjOrErr = object::SymbolicFile::createSymbolicFile(Buf);
-    if (!ObjOrErr) {
-      // FIXME: check only for "not an object file" errors.
-      consumeError(ObjOrErr.takeError());
-      return Ret;
-    }
+    if (!ObjOrErr)
+      return ObjOrErr.takeError();
     Obj = std::move(*ObjOrErr);
   }
 
   HasObject = true;
   for (const object::BasicSymbolRef &S : Obj->symbols()) {
     if (!isArchiveSymbol(S))
       continue;
     Ret.push_back(SymNames.tell());
     if (Error E = S.printName(SymNames))
       return std::move(E);
     SymNames << '\0';
   }
   return Ret;
 }
 
 static Expected<std::vector<MemberData>>
 computeMemberData(raw_ostream &StringTable, raw_ostream &SymNames,
                   object::Archive::Kind Kind, bool Thin, bool Deterministic,
-                  ArrayRef<NewArchiveMember> NewMembers) {
+                  bool NeedSymbols, ArrayRef<NewArchiveMember> NewMembers) {
   static char PaddingData[8] = {'\n', '\n', '\n', '\n', '\n', '\n', '\n', '\n'};
 
   // This ignores the symbol table, but we only need the value mod 8 and the
   // symbol table is aligned to be a multiple of 8 bytes
   uint64_t Pos = 0;
 
   std::vector<MemberData> Ret;
   bool HasObject = false;
if (Size > object::Archive::MaxMemberSize) {
       return make_error<object::GenericBinaryError>(
           std::move(StringMsg), object::object_error::parse_failed);
     }
 
     printMemberHeader(Out, Pos, StringTable, MemberNames, Kind, Thin, M,
                       ModTime, Size);
     Out.flush();
 
-    Expected<std::vector<unsigned>> Symbols =
+    std::vector<unsigned> Symbols;
+    if (NeedSymbols) {
+      Expected<std::vector<unsigned>> SymbolsOrErr =
           getSymbols(Buf, SymNames, HasObject);
-    if (auto E = Symbols.takeError())
+      if (auto E = SymbolsOrErr.takeError())
         return std::move(E);
+      Symbols = std::move(*SymbolsOrErr);
+    }
 
     Pos += Header.size() + Data.size() + Padding.size();
-    Ret.push_back({std::move(*Symbols), std::move(Header), Data, Padding});
+    Ret.push_back({std::move(Symbols), std::move(Header), Data, Padding});
   }
   // If there are no symbols, emit an empty symbol table, to satisfy Solaris
   // tools, older versions of which expect a symbol table in a non-empty
   // archive, regardless of whether there are any symbols in it.
   if (HasObject && SymNames.tell() == 0)
     SymNames << '\0' << '\0' << '\0';
   return Ret;
 }
static Error writeArchiveToStream(raw_ostream &Out,
                                   bool Deterministic, bool Thin) {
   assert((!Thin || !isBSDLike(Kind)) && "Only the gnu format has a thin mode");
 
   SmallString<0> SymNamesBuf;
   raw_svector_ostream SymNames(SymNamesBuf);
   SmallString<0> StringTableBuf;
   raw_svector_ostream StringTable(StringTableBuf);
 
-  Expected<std::vector<MemberData>> DataOrErr = computeMemberData(
-      StringTable, SymNames, Kind, Thin, Deterministic, NewMembers);
+  Expected<std::vector<MemberData>> DataOrErr =
+      computeMemberData(StringTable, SymNames, Kind, Thin, Deterministic,
+                        WriteSymtab, NewMembers);
   if (Error E = DataOrErr.takeError())
     return E;
   std::vector<MemberData> &Data = *DataOrErr;
 
   if (!StringTableBuf.empty())
     Data.insert(Data.begin(), computeStringTable(StringTableBuf));
 
   // We would like to detect if we need to switch to a 64-bit symbol table.

llvm/lib/Object/SymbolicFile.cpp

 
 Expected<std::unique_ptr<SymbolicFile>>
 SymbolicFile::createSymbolicFile(MemoryBufferRef Object, file_magic Type,
                                  LLVMContext *Context) {
   StringRef Data = Object.getBuffer();
   if (Type == file_magic::unknown)
     Type = identify_magic(Data);
 
+  if (!isSymbolicFile(Type))
+    return errorCodeToError(object_error::invalid_file_type);
+
   switch (Type) {
   case file_magic::bitcode:
     if (Context)
       return IRObjectFile::create(Object, *Context);
-    LLVM_FALLTHROUGH;
-  case file_magic::unknown:
-  case file_magic::archive:
-  case file_magic::coff_cl_gl_object:
-  case file_magic::macho_universal_binary:
-  case file_magic::windows_resource:
-  case file_magic::pdb:
-  case file_magic::minidump:
-  case file_magic::tapi_file:
     return errorCodeToError(object_error::invalid_file_type);
   case file_magic::elf:
   case file_magic::elf_executable:
   case file_magic::elf_shared_object:
   case file_magic::elf_core:
   case file_magic::macho_executable:
   case file_magic::macho_fixed_virtual_memory_shared_lib:
   case file_magic::macho_core:
   case file_magic::macho_preload_executable:
case file_magic::coff_object: {
 
     return IRObjectFile::create(
         MemoryBufferRef(BCData->getBuffer(), Object.getBufferIdentifier()),
         *Context);
   }
   }
   llvm_unreachable("Unexpected Binary File Type");
 }
+
+bool SymbolicFile::isSymbolicFile(file_magic Type) {
+  switch (Type) {
+  case file_magic::unknown:
+  case file_magic::archive:
+  case file_magic::coff_cl_gl_object:
+  case file_magic::macho_universal_binary:
+  case file_magic::windows_resource:
+  case file_magic::pdb:
+  case file_magic::minidump:
+  case file_magic::tapi_file:
+    return false;
+  }
+  return true;
+}

llvm/test/Object/archive-malformed-object.test

+## Show that the archive library emits error messages when adding malformed
+## objects.
+
+# RUN: rm -rf %t.dir
+# RUN: split-file %s %t.dir
+# RUN: cd %t.dir
+
+## Malformed bitcode object.
+# RUN: llvm-as input.ll -o input.bc
+# RUN: %python -c "with open('input.bc', 'a') as f: f.truncate(10)"
+# RUN: not llvm-ar rc bad.a input.bc 2>&1 | FileCheck %s --check-prefix=ERR1
+
+# ERR1: error: bad.a: Invalid bitcode signature
+
+## Non-bitcode malformed file.
+# RUN: yaml2obj input.yaml -o input.o
+# RUN: not llvm-ar rc bad.a input.o 2>&1 | FileCheck %s --check-prefix=ERR2
+
+# ERR2: error: bad.a: section header table goes past the end of the file: e_shoff = 0x9999
+
+## Don't emit an error if the symbol table is not required.
+# RUN: llvm-ar rcS good.a input.o input.bc
+# RUN: llvm-ar t good.a | FileCheck %s --check-prefix=CONTENTS
+
+# CONTENTS:      input.o
+# CONTENTS-NEXT: input.bc
+
+#--- input.ll
+target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-linux"
+
+#--- input.yaml
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  EShOff:  0x9999

llvm/test/Object/archive-unknown-filetype.test

+## Show that the archive library does not emit an error or add any symbols to
+## the archive symbol table, when it encounters an unknown file type, but still
+## adds the file to the archive.
+
+# RUN: touch %t
+# RUN: rm -f %t.a
+# RUN: llvm-ar rc %t.a %t
+# RUN: llvm-ar t %t.a | FileCheck %s --check-prefix=CONTENTS -DFILE=%basename_t
+# RUN: llvm-nm --print-armap %t.a | FileCheck %s --allow-empty --implicit-check-not={{.}}
+
+# CONTENTS: [[FILE]]