This removes the eating of the error in Archive::Child::getSize() when the characters

in the size field in the archive header for the member is not a number.  To do this we
have all of the needed methods return ErrorOr to push them up until we get out of lib.
Then the tools and can handle the error in whatever way is appropriate for that tool.

So the solution is to plumb all the ErrorOr stuff through everything that touches archives.
This include its iterators as one can create an Archive object but the first or any other
Child object may fail to be created due to a bad size field in its header.

Thanks to Lang Hames on the changes making child_iterator contain an
ErrorOr<Child> instead of a Child and the needed changes to ErrorOr.h to add
operator overloading for * and -> .

We don’t want to use llvm_unreachable() as it calls abort() and is produces a “crash”
and using report_fatal_error() to move the error checking will cause the program to
stop, neither of which are really correct in library code. There are still some uses of
these that should be cleaned up in this library code for other than the size field.

Also corrected the code where the size gets us to the “at the end of the archive”
which is OK but past the end of the archive will return object_error::parse_failed now.

The test cases use archives with text files so one can see the non-digit character,
in this case a ‘%’, in the size field.

llvm-svn: 250906

1 files changed, 107 insertions, 37 deletions

diff --git a/llvm/lib/Object/Archive.cpp b/llvm/lib/Object/Archive.cpp
index 667732b..1cc3c48 100644
--- a/llvm/lib/Object/Archive.cpp
+++ b/llvm/lib/Object/Archive.cpp
@@ -46,7 +46,7 @@ StringRef ArchiveMemberHeader::getName() const {
 ErrorOr<uint32_t> ArchiveMemberHeader::getSize() const {
   uint32_t Ret;
   if (llvm::StringRef(Size, sizeof(Size)).rtrim(" ").getAsInteger(10, Ret))
-    return object_error::parse_failed;
+    return object_error::parse_failed; // Size is not a decimal number.
   return Ret;
 }
 
@@ -82,7 +82,8 @@ unsigned ArchiveMemberHeader::getGID() const {
   return Ret;
 }
 
-Archive::Child::Child(const Archive *Parent, const char *Start)
+Archive::Child::Child(const Archive *Parent, const char *Start,
+                      std::error_code *EC)
     : Parent(Parent) {
   if (!Start)
     return;
@@ -90,7 +91,13 @@ Archive::Child::Child(const Archive *Parent, const char *Start)
   uint64_t Size = sizeof(ArchiveMemberHeader);
   Data = StringRef(Start, Size);
   if (!isThinMember()) {
-    Size += getRawSize();
+    ErrorOr<uint64_t> MemberSize = getRawSize();
+    if (MemberSize.getError()) {
+      assert (EC && "Error must be caught");
+      *EC = MemberSize.getError();
+      return;
+    }
+    Size += MemberSize.get();
     Data = StringRef(Start, Size);
   }
 
@@ -100,26 +107,38 @@ Archive::Child::Child(const Archive *Parent, const char *Start)
   StringRef Name = getRawName();
   if (Name.startswith("#1/")) {
     uint64_t NameSize;
-    if (Name.substr(3).rtrim(" ").getAsInteger(10, NameSize))
-      llvm_unreachable("Long name length is not an integer");
+    if (Name.substr(3).rtrim(" ").getAsInteger(10, NameSize)) {
+      if (EC)
+        *EC = object_error::parse_failed; // Long name offset is not an integer.
+      return;
+    }
     StartOfFile += NameSize;
   }
 }
 
-uint64_t Archive::Child::getSize() const {
+ErrorOr<std::unique_ptr<Archive::Child>> Archive::Child::create(
+  const Archive *Parent, const char *Start) {
+  std::error_code EC;
+  std::unique_ptr<Archive::Child> Ret(new Archive::Child(Parent, Start, &EC));
+  if (EC)
+    return EC;
+  return std::move(Ret);
+}
+
+ErrorOr<uint64_t> Archive::Child::getSize() const {
   if (Parent->IsThin) {
     ErrorOr<uint32_t> Size = getHeader()->getSize();
-    if (Size.getError())
-      return 0;
+    if (std::error_code EC = Size.getError())
+      return EC;
     return Size.get();
   }
   return Data.size() - StartOfFile;
 }
 
-uint64_t Archive::Child::getRawSize() const {
+ErrorOr<uint64_t> Archive::Child::getRawSize() const {
   ErrorOr<uint32_t> Size = getHeader()->getSize();
-  if (Size.getError())
-    return 0;
+  if (std::error_code EC = Size.getError())
+    return EC;
   return Size.get();
 }
 
@@ -129,8 +148,12 @@ bool Archive::Child::isThinMember() const {
 }
 
 ErrorOr<StringRef> Archive::Child::getBuffer() const {
-  if (!isThinMember())
-    return StringRef(Data.data() + StartOfFile, getSize());
+  if (!isThinMember()) {
+    ErrorOr<uint32_t> Size = getSize();
+    if (std::error_code EC = Size.getError())
+      return EC;
+    return StringRef(Data.data() + StartOfFile, Size.get());
+  }
   ErrorOr<StringRef> Name = getName();
   if (std::error_code EC = Name.getError())
     return EC;
@@ -144,19 +167,28 @@ ErrorOr<StringRef> Archive::Child::getBuffer() const {
   return Parent->ThinBuffers.back()->getBuffer();
 }
 
-Archive::Child Archive::Child::getNext() const {
+ErrorOr<Archive::Child> Archive::Child::getNext() const {
   size_t SpaceToSkip = Data.size();
   // If it's odd, add 1 to make it even.
+  size_t Pad = 0;
   if (SpaceToSkip & 1)
-    ++SpaceToSkip;
+    Pad++;
 
-  const char *NextLoc = Data.data() + SpaceToSkip;
+  const char *NextLoc = Data.data() + SpaceToSkip + Pad;
+
+  // Check to see if this is at the end of the archive.
+  if (NextLoc == Parent->Data.getBufferEnd() ||
+      NextLoc == Parent->Data.getBufferEnd() - Pad )
+    return Child(Parent, nullptr, nullptr);
 
   // Check to see if this is past the end of the archive.
-  if (NextLoc >= Parent->Data.getBufferEnd())
-    return Child(Parent, nullptr);
+  if (NextLoc > Parent->Data.getBufferEnd())
+    return object_error::parse_failed;
 
-  return Child(Parent, NextLoc);
+  auto ChildOrErr = Child::create(Parent, NextLoc);
+  if (std::error_code EC = ChildOrErr.getError())
+    return EC;
+  return std::move(*ChildOrErr.get());
 }
 
 uint64_t Archive::Child::getChildOffset() const {
@@ -178,17 +210,23 @@ ErrorOr<StringRef> Archive::Child::getName() const {
     // Get the offset.
     std::size_t offset;
     if (name.substr(1).rtrim(" ").getAsInteger(10, offset))
-      llvm_unreachable("Long name offset is not an integer");
-    const char *addr = Parent->StringTable->Data.begin()
+      return object_error::parse_failed; // Long name offset is not an integer.
+    // Check for bad stringtable iterator.
+    if (std::error_code EC = Parent->StringTable->getError())
+      return EC;
+    const char *addr = (*Parent->StringTable)->Data.begin()
                        + sizeof(ArchiveMemberHeader)
                        + offset;
     // Verify it.
+    auto Size = (*Parent->StringTable)->getSize();
+    if (std::error_code EC = Size.getError())
+      return EC;
     if (Parent->StringTable == Parent->child_end()
-        || addr < (Parent->StringTable->Data.begin()
+        || addr < ((*Parent->StringTable)->Data.begin()
                    + sizeof(ArchiveMemberHeader))
-        || addr > (Parent->StringTable->Data.begin()
+        || addr > ((*Parent->StringTable)->Data.begin()
                    + sizeof(ArchiveMemberHeader)
-                   + Parent->StringTable->getSize()))
+                   + Size.get()))
       return object_error::parse_failed;
 
     // GNU long file names end with a "/\n".
@@ -200,7 +238,7 @@ ErrorOr<StringRef> Archive::Child::getName() const {
   } else if (name.startswith("#1/")) {
     uint64_t name_size;
     if (name.substr(3).rtrim(" ").getAsInteger(10, name_size))
-      llvm_unreachable("Long name length is not an ingeter");
+      return object_error::parse_failed; // Long name offset is not an integer.
     return Data.substr(sizeof(ArchiveMemberHeader), name_size)
         .rtrim(StringRef("\0", 1));
   }
@@ -256,12 +294,12 @@ Archive::Archive(MemoryBufferRef Source, std::error_code &ec)
   child_iterator i = child_begin(false);
   child_iterator e = child_end();
 
-  if (i == e) {
-    ec = std::error_code();
+  if (!*i || i == e) {
+    ec = i->getError();
     return;
   }
 
-  StringRef Name = i->getRawName();
+  StringRef Name = (*i)->getRawName();
 
   // Below is the pattern that is used to figure out the archive format
   // GNU archive format
@@ -286,6 +324,11 @@ Archive::Archive(MemoryBufferRef Source, std::error_code &ec)
     Format = K_BSD;
     SymbolTable = i;
     ++i;
+    if (!*i) {
+      ec = i->getError();
+      return;
+    }
+
     FirstRegular = i;
     ec = std::error_code();
     return;
@@ -294,7 +337,7 @@ Archive::Archive(MemoryBufferRef Source, std::error_code &ec)
   if (Name.startswith("#1/")) {
     Format = K_BSD;
     // We know this is BSD, so getName will work since there is no string table.
-    ErrorOr<StringRef> NameOrErr = i->getName();
+    ErrorOr<StringRef> NameOrErr = (*i)->getName();
     ec = NameOrErr.getError();
     if (ec)
       return;
@@ -302,6 +345,10 @@ Archive::Archive(MemoryBufferRef Source, std::error_code &ec)
     if (Name == "__.SYMDEF SORTED" || Name == "__.SYMDEF") {
       SymbolTable = i;
       ++i;
+      if (!*i) {
+        ec = i->getError();
+        return;
+      }
     }
     FirstRegular = i;
     return;
@@ -319,17 +366,21 @@ Archive::Archive(MemoryBufferRef Source, std::error_code &ec)
       has64SymTable = true;
 
     ++i;
-    if (i == e) {
-      ec = std::error_code();
+    if (!*i || i == e) {
+      ec = i->getError();
       return;
     }
-    Name = i->getRawName();
+    Name = (*i)->getRawName();
   }
 
   if (Name == "//") {
     Format = has64SymTable ? K_MIPS64 : K_GNU;
     StringTable = i;
     ++i;
+    if (!*i) {
+      ec = i->getError();
+      return;
+    }
     FirstRegular = i;
     ec = std::error_code();
     return;
@@ -351,17 +402,25 @@ Archive::Archive(MemoryBufferRef Source, std::error_code &ec)
   SymbolTable = i;
 
   ++i;
+  if (!*i) {
+    ec = i->getError();
+    return;
+  }
   if (i == e) {
     FirstRegular = i;
     ec = std::error_code();
     return;
   }
 
-  Name = i->getRawName();
+  Name = (*i)->getRawName();
 
   if (Name == "//") {
     StringTable = i;
     ++i;
+    if (!*i) {
+      ec = i->getError();
+      return;
+    }
   }
 
   FirstRegular = i;
@@ -376,12 +435,20 @@ Archive::child_iterator Archive::child_begin(bool SkipInternal) const {
     return FirstRegular;
 
   const char *Loc = Data.getBufferStart() + strlen(Magic);
-  Child c(this, Loc);
-  return c;
+  auto ChildOrErr = Child::create(this, Loc);
+  if (std::error_code EC = ChildOrErr.getError())
+    return child_iterator(EC);
+  Child c = *(ChildOrErr.get());
+  return child_iterator(c);
 }
 
 Archive::child_iterator Archive::child_end() const {
-  return Child(this, nullptr);
+  // This with a second argument of nullptr can't return an Error.
+  auto ChildOrErr = Child::create(this, nullptr);
+  if (ChildOrErr.getError())
+    llvm_unreachable("Can't create Archive::child_end().");
+  Child c = *(ChildOrErr.get());
+  return child_iterator(c);
 }
 
 StringRef Archive::Symbol::getName() const {
@@ -433,7 +500,10 @@ ErrorOr<Archive::child_iterator> Archive::Symbol::getMember() const {
   }
 
   const char *Loc = Parent->getData().begin() + Offset;
-  child_iterator Iter(Child(Parent, Loc));
+  auto ChildOrErr = Child::create(Parent, Loc);
+  if (std::error_code EC = ChildOrErr.getError())
+    return EC;
+  child_iterator Iter(std::move(*ChildOrErr.get()));
   return Iter;
 }