[lld-macho]Rework error-checking in peeking at first-member in archive to avoid segfault.

[oontvoo]

Summary

Details:
calling getMemoryBufferRef() on an empty archive can sometimes trigger segfault so the code should check before calling this.
this seems like a bug in the Archive API but that can be fixed separately.

P.S: follow up to D156468

[oontvoo]

It seems maskray might be OOO , could someone else give this a quick look please? :) @int3 @keith ? TIA!

[MaskRay]

calling getMemoryBufferRef() on an empty archive can sometimes trigger segfault so the code should check before calling this. this seems like a bug in the Archive API but that can be fixed separately.

Is there a test case? When is "sometimes"?

[oontvoo]

In D157300#4567865, @MaskRay wrote:

calling getMemoryBufferRef() on an empty archive can sometimes trigger segfault so the code should check before calling this. this seems like a bug in the Archive API but that can be fixed separately.

Is there a test case? When is "sometimes"?

reducing the test-case further, I think it's safe to change "sometimes" => "100% of the time".
Test case is simply linking with at least one empty archive in the input

(empty archive produced by llvm-ar --format=darwin rcs libempty.a
This will produce an empty archive but the ::isEmpty() method on the API doesn't return true, so getBufferRef() would crash.)

[oontvoo]

added trivial test case

[smeenai]

LGTM with comments.

lld/MachO/InputFiles.cpp

2117–2133

Typo: BUffer -> Buffer

I think this would be cleaner as an early return, instead of nesting everything else inside it, since zero symbols should mean the loop below is a no-op as well.

[oontvoo]

addressed review comment

lld/MachO/InputFiles.cpp

2117–2133

Thanks!
Fixed typo and moved the numberOfSymbols check to beginning.
(Still need the nested if,unfortunately, because we dont want to access the child if there was error )

[oontvoo]

rebase

[oontvoo]

Thanks!

lld/MachO/InputFiles.cpp

if (config->applicationExtension && !dylibIsAppExtensionSafe)
     warn("using '-application_extension' with unsafe dylib: " + toString(this));
 }
 
 ArchiveFile::ArchiveFile(std::unique_ptr<object::Archive> &&f, bool forceHidden)
     : InputFile(ArchiveKind, f->getMemoryBufferRef()), file(std::move(f)),
       forceHidden(forceHidden) {}
 
 void ArchiveFile::addLazySymbols() {
-  Error err = Error::success();
-  Expected<MemoryBufferRef> mbOrErr =
-      this->file->child_begin(err)->getMemoryBufferRef();
+  // Avoid calling getMemoryBufferRef() on zero-symbol archive
+  // since that crashes.
+  if (file->isEmpty() || file->getNumberOfSymbols() == 0)
+    return;
 
+  Error err = Error::success();
+  auto child = file->child_begin(err);
   // Ignore the I/O error here - will be reported later.
+  if (!err) {
+    Expected<MemoryBufferRef> mbOrErr = child->getMemoryBufferRef();
     if (!mbOrErr) {
       llvm::consumeError(mbOrErr.takeError());
-  } else if (!err) {
+    } else {
       if (identify_magic(mbOrErr->getBuffer()) == file_magic::macho_object) {
         if (target->wordSize == 8)
           compatArch = compatWithTargetArch(
               this, reinterpret_cast<const LP64::mach_header *>(
                         mbOrErr->getBufferStart()));
         else
           compatArch = compatWithTargetArch(
               this, reinterpret_cast<const ILP32::mach_header *>(
                         mbOrErr->getBufferStart()));
-
         if (!compatArch)
           return;
       }
     }
+  }
+
   for (const object::Archive::Symbol &sym : file->symbols())
     symtab->addLazyArchive(sym.getName(), this, sym);
 }
 
 static Expected<InputFile *>
 loadArchiveMember(MemoryBufferRef mb, uint32_t modTime, StringRef archiveName,
                   uint64_t offsetInArchive, bool forceHidden, bool compatArch) {
   if (config->zeroModTime)

lld/test/MachO/force-load.s

 
 ## Force-loading two different paths w/o conflicting symbols is fine
 # RUN: %lld -lSystem -force_load %t/foo.a -force_load %t/baz.a %t/test.o -o %t/test-two-force-loads
 # RUN: llvm-objdump --syms %t/test-two-force-loads | FileCheck %s --check-prefix=TWICE
 # TWICE-DAG: __TEXT,archive _foo
 # TWICE-DAG: __TEXT,archive _bar
 # TWICE-DAG: __TEXT,archive _baz
 
+## Loading an empty-archive should not crash.
+# RUN: llvm-ar --format=darwin rcs %t/libEmpty.a
+# RUN: %lld -lSystem %t/test.o -force_load %t/libEmpty.a -o %t/loadEmpty.out
+# RUN: llvm-objdump --macho --syms %t/loadEmpty.out | FileCheck %s --check-prefix=EMPTY
+# EMPTY: F __TEXT,__text _main
+
 #--- archive-foo.s
 .section __TEXT,archive
 .globl _foo, _bar
 .weak_definition _foo
 _foo:
 _bar:
 
 #--- archive-baz.s