[PATCH] D13989: This removes the eating of the error in Archive::Child::getSize() when the charactersin the size field in the archive header for the member is not a number.
Rafael Espíndola via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 27 09:33:52 PDT 2015
I implemented some, but not all, the suggestions while reviewing the
patch. It should make it easier to finish it up.
On 27 October 2015 at 09:30, Rafael Ávila de Espíndola
<llvm-commits at lists.llvm.org> wrote:
> rafael added inline comments.
>
> ================
> Comment at: include/llvm/Object/Archive.h:111
> @@ -109,3 +110,3 @@
> class child_iterator {
> - Child child;
> + ErrorOr<Child> child;
>
> ----------------
> I don't think the iterator should include the possibility of being at error. Everywhere else we just push the error outwards.
>
> This should still be just "Child child;" and then when appropriate we should use a ErrorOr<child_iterator>. For example, child_begin should return that.
>
> ================
> Comment at: lib/Object/Archive.cpp:181
> @@ -180,3 +201,1 @@
> if (name.substr(1).rtrim(" ").getAsInteger(10, offset))
> - llvm_unreachable("Long name offset is not an integer");
> - const char *addr = Parent->StringTable->Data.begin()
> ----------------
> This changes are nice but independent. Please leave them for another path.
>
>
> http://reviews.llvm.org/D13989
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
-------------- next part --------------
diff --git a/include/llvm/Object/Archive.h b/include/llvm/Object/Archive.h
index 32c72a0..1ec0976 100644
--- a/include/llvm/Object/Archive.h
+++ b/include/llvm/Object/Archive.h
@@ -65,7 +65,7 @@ public:
bool isThinMember() const;
public:
- Child(const Archive *Parent, const char *Start);
+ Child(const Archive *Parent, const char *Start, std::error_code *EC);
bool operator ==(const Child &other) const {
assert(Parent == other.Parent);
@@ -77,7 +77,7 @@ public:
}
const Archive *getParent() const { return Parent; }
- Child getNext() const;
+ ErrorOr<Child> getNext() const;
ErrorOr<StringRef> getName() const;
StringRef getRawName() const { return getHeader()->getName(); }
@@ -93,9 +93,9 @@ public:
return getHeader()->getAccessMode();
}
/// \return the size of the archive member without the header or padding.
- uint64_t getSize() const;
+ ErrorOr<uint64_t> getSize() const;
/// \return the size in the archive header for this member.
- uint64_t getRawSize() const;
+ ErrorOr<uint64_t> getRawSize() const;
ErrorOr<StringRef> getBuffer() const;
uint64_t getChildOffset() const;
@@ -110,7 +110,7 @@ public:
Child child;
public:
- child_iterator() : child(Child(nullptr, nullptr)) {}
+ child_iterator() : child(Child(nullptr, nullptr, nullptr)) {}
child_iterator(const Child &c) : child(c) {}
const Child *operator->() const { return &child; }
const Child &operator*() const { return child; }
@@ -127,9 +127,11 @@ public:
return child < other.child;
}
- child_iterator &operator++() { // Preincrement
- child = child.getNext();
- return *this;
+ ErrorOr<child_iterator> getNext() {
+ ErrorOr<Child> ChildOrError = child.getNext();
+ if (std::error_code EC = ChildOrError.getError())
+ return EC;
+ return child_iterator(*ChildOrError);
}
};
@@ -186,12 +188,8 @@ public:
Kind kind() const { return (Kind)Format; }
bool isThin() const { return IsThin; }
- child_iterator child_begin(bool SkipInternal = true) const;
+ ErrorOr<child_iterator> child_begin(bool SkipInternal = true) const;
child_iterator child_end() const;
- iterator_range<child_iterator> children(bool SkipInternal = true) const {
- return iterator_range<child_iterator>(child_begin(SkipInternal),
- child_end());
- }
symbol_iterator symbol_begin() const;
symbol_iterator symbol_end() const;
diff --git a/include/llvm/Support/ErrorOr.h b/include/llvm/Support/ErrorOr.h
index 46b6192..ca6ede7 100644
--- a/include/llvm/Support/ErrorOr.h
+++ b/include/llvm/Support/ErrorOr.h
@@ -91,6 +91,7 @@ private:
typedef typename std::remove_reference<T>::type &reference;
typedef const typename std::remove_reference<T>::type &const_reference;
typedef typename std::remove_reference<T>::type *pointer;
+ typedef const typename std::remove_reference<T>::type *const_pointer;
public:
template <class E>
@@ -183,10 +184,14 @@ public:
return toPointer(getStorage());
}
+ const_pointer operator->() const { return toPointer(getStorage()); }
+
reference operator *() {
return *getStorage();
}
+ const_reference operator*() const { return *getStorage(); }
+
private:
template <class OtherT>
void copyConstruct(const ErrorOr<OtherT> &Other) {
@@ -246,10 +251,14 @@ private:
return Val;
}
+ const_pointer toPointer(const_pointer Val) const { return Val; }
+
pointer toPointer(wrap *Val) {
return &Val->get();
}
+ const_pointer toPointer(const wrap *Val) const { return &Val->get(); }
+
storage_type *getStorage() {
assert(!HasError && "Cannot get value when an error exists!");
return reinterpret_cast<storage_type*>(TStorage.buffer);
diff --git a/lib/Object/Archive.cpp b/lib/Object/Archive.cpp
index 667732b..4c8e1b4 100644
--- a/lib/Object/Archive.cpp
+++ b/lib/Object/Archive.cpp
@@ -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,10 @@ 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 ((*EC = MemberSize.getError()))
+ return;
+ Size += MemberSize.get();
Data = StringRef(Start, Size);
}
@@ -106,20 +110,20 @@ Archive::Child::Child(const Archive *Parent, const char *Start)
}
}
-uint64_t Archive::Child::getSize() const {
+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 +133,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 +152,29 @@ 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 + Pad;
- const char *NextLoc = Data.data() + SpaceToSkip;
+ // 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);
+ std::error_code EC;
+ Child Ret(Parent, NextLoc, &EC);
+ if (EC)
+ return EC;
+ return Ret;
}
uint64_t Archive::Child::getChildOffset() const {
@@ -183,12 +201,14 @@ ErrorOr<StringRef> Archive::Child::getName() const {
+ sizeof(ArchiveMemberHeader)
+ offset;
// Verify it.
- if (Parent->StringTable == Parent->child_end()
- || addr < (Parent->StringTable->Data.begin()
- + sizeof(ArchiveMemberHeader))
- || addr > (Parent->StringTable->Data.begin()
- + sizeof(ArchiveMemberHeader)
- + Parent->StringTable->getSize()))
+ ErrorOr<uint32_t> Size = Parent->StringTable->getSize();
+ if (std::error_code EC = Size.getError())
+ return EC;
+ if (Parent->StringTable == Parent->child_end() ||
+ addr <
+ (Parent->StringTable->Data.begin() + sizeof(ArchiveMemberHeader)) ||
+ addr > (Parent->StringTable->Data.begin() +
+ sizeof(ArchiveMemberHeader) + Size.get()))
return object_error::parse_failed;
// GNU long file names end with a "/\n".
@@ -253,7 +273,10 @@ Archive::Archive(MemoryBufferRef Source, std::error_code &ec)
}
// Get the special members.
- child_iterator i = child_begin(false);
+ ErrorOr<child_iterator> IOrErr = child_begin(false);
+ if ((ec = IOrErr.getError()))
+ return;
+ child_iterator i = *IOrErr;
child_iterator e = child_end();
if (i == e) {
@@ -282,12 +305,20 @@ Archive::Archive(MemoryBufferRef Source, std::error_code &ec)
// seem to create the third member if there's no member whose filename
// exceeds 15 characters. So the third member is optional.
+ auto Increment = [&]() {
+ ErrorOr<child_iterator> Next = i.getNext();
+ if ((ec = Next.getError()))
+ return true;
+ i = *Next;
+ return false;
+ };
+
if (Name == "__.SYMDEF") {
Format = K_BSD;
SymbolTable = i;
- ++i;
+ if (Increment())
+ return;
FirstRegular = i;
- ec = std::error_code();
return;
}
@@ -301,7 +332,8 @@ Archive::Archive(MemoryBufferRef Source, std::error_code &ec)
Name = NameOrErr.get();
if (Name == "__.SYMDEF SORTED" || Name == "__.SYMDEF") {
SymbolTable = i;
- ++i;
+ if (Increment())
+ return;
}
FirstRegular = i;
return;
@@ -318,7 +350,8 @@ Archive::Archive(MemoryBufferRef Source, std::error_code &ec)
if (Name == "/SYM64/")
has64SymTable = true;
- ++i;
+ if (Increment())
+ return;
if (i == e) {
ec = std::error_code();
return;
@@ -329,7 +362,8 @@ Archive::Archive(MemoryBufferRef Source, std::error_code &ec)
if (Name == "//") {
Format = has64SymTable ? K_MIPS64 : K_GNU;
StringTable = i;
- ++i;
+ if (Increment())
+ return;
FirstRegular = i;
ec = std::error_code();
return;
@@ -350,7 +384,8 @@ Archive::Archive(MemoryBufferRef Source, std::error_code &ec)
Format = K_COFF;
SymbolTable = i;
- ++i;
+ if (Increment())
+ return;
if (i == e) {
FirstRegular = i;
ec = std::error_code();
@@ -361,14 +396,15 @@ Archive::Archive(MemoryBufferRef Source, std::error_code &ec)
if (Name == "//") {
StringTable = i;
- ++i;
+ if (Increment())
+ return;
}
FirstRegular = i;
ec = std::error_code();
}
-Archive::child_iterator Archive::child_begin(bool SkipInternal) const {
+ErrorOr<Archive::child_iterator> Archive::child_begin(bool SkipInternal) const {
if (Data.getBufferSize() == 8) // empty archive.
return child_end();
@@ -376,12 +412,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;
+ std::error_code EC;
+ Child c(this, Loc, &EC);
+ if (EC)
+ return EC;
+ 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.
+ std::error_code EC;
+ Child c(this, nullptr, &EC);
+ if (EC)
+ llvm_unreachable("Can't create Archive::child_end().");
+ return child_iterator(c);
}
StringRef Archive::Symbol::getName() const {
@@ -433,7 +477,11 @@ ErrorOr<Archive::child_iterator> Archive::Symbol::getMember() const {
}
const char *Loc = Parent->getData().begin() + Offset;
- child_iterator Iter(Child(Parent, Loc));
+ std::error_code EC;
+ Child c(Parent, Loc, &EC);
+ if (EC)
+ return EC;
+ child_iterator Iter(c);
return Iter;
}
diff --git a/lib/Object/ArchiveWriter.cpp b/lib/Object/ArchiveWriter.cpp
index dae0188..db97291 100644
--- a/lib/Object/ArchiveWriter.cpp
+++ b/lib/Object/ArchiveWriter.cpp
@@ -412,8 +412,11 @@ llvm::writeArchive(StringRef ArcName,
Status.getSize());
} else {
object::Archive::child_iterator OldMember = I.getOld();
+ ErrorOr<uint32_t> Size = OldMember->getSize();
+ if (std::error_code EC = Size.getError())
+ return std::make_pair("", EC);
printMemberHeader(Out, Kind, Thin, I.getName(), StringMapIndexIter,
- ModTime, UID, GID, Perms, OldMember->getSize());
+ ModTime, UID, GID, Perms, *Size);
}
if (!Thin)
diff --git a/test/tools/llvm-objdump/Inputs/malformed-archives/libbogus1.a b/test/tools/llvm-objdump/Inputs/malformed-archives/libbogus1.a
new file mode 100644
index 0000000..510c145
--- /dev/null
+++ b/test/tools/llvm-objdump/Inputs/malformed-archives/libbogus1.a
@@ -0,0 +1,13 @@
+!<arch>
+hello.c 1444941273 124 0 100644 10% `
+#include <stdio.h>
+#include <stdlib.h>
+int
+main()
+{
+ printf("Hello World\n");
+ return EXIT_SUCCESS;
+}
+foo.c 1444941645 124 0 100644 1% `
+void foo(void){}
+
diff --git a/test/tools/llvm-objdump/Inputs/malformed-archives/libbogus2.a b/test/tools/llvm-objdump/Inputs/malformed-archives/libbogus2.a
new file mode 100644
index 0000000..2ccb7f3
--- /dev/null
+++ b/test/tools/llvm-objdump/Inputs/malformed-archives/libbogus2.a
@@ -0,0 +1,13 @@
+!<arch>
+hello.c 1444941273 124 0 100644 102 `
+#include <stdio.h>
+#include <stdlib.h>
+int
+main()
+{
+ printf("Hello World\n");
+ return EXIT_SUCCESS;
+}
+foo.c 1444941645 124 0 100644 1% `
+void foo(void){}
+
diff --git a/test/tools/llvm-objdump/Inputs/malformed-archives/libbogus3.a b/test/tools/llvm-objdump/Inputs/malformed-archives/libbogus3.a
new file mode 100644
index 0000000..f15a732
--- /dev/null
+++ b/test/tools/llvm-objdump/Inputs/malformed-archives/libbogus3.a
@@ -0,0 +1,16 @@
+!<arch>
+hello.c 1444941273 124 0 100644 102 `
+#include <stdio.h>
+#include <stdlib.h>
+int
+main()
+{
+ printf("Hello World\n");
+ return EXIT_SUCCESS;
+}
+foo.c 1444941645 124 0 100644 171 `
+void foo(void){}
+
+bar.c 1445026190 124 0 100644 17 `
+void foo(void){}
+
diff --git a/test/tools/llvm-objdump/malformed-archives.test b/test/tools/llvm-objdump/malformed-archives.test
new file mode 100644
index 0000000..5066cb5
--- /dev/null
+++ b/test/tools/llvm-objdump/malformed-archives.test
@@ -0,0 +1,20 @@
+// These test checks that llvm-objdump will not crash with malformed Archive
+// files. So the check line is not all that important but the bug fixes to
+// make sure llvm-objdump is robust is what matters.
+# RUN: llvm-objdump -macho -archive-headers \
+# RUN: %p/Inputs/malformed-archives/libbogus1.a \
+# RUN: 2>&1 | FileCheck -check-prefix=bogus1 %s
+
+# bogus1: Invalid data was encountered while parsing the file
+
+# RUN: llvm-objdump -macho -archive-headers \
+# RUN: %p/Inputs/malformed-archives/libbogus2.a \
+# RUN: 2>&1 | FileCheck -check-prefix=bogus2 %s
+
+# bogus2: hello.c
+
+# RUN: llvm-objdump -macho -archive-headers \
+# RUN: %p/Inputs/malformed-archives/libbogus3.a \
+# RUN: 2>&1 | FileCheck -check-prefix=bogus3 %s
+
+# bogus3: foo.c
diff --git a/tools/llvm-ar/llvm-ar.cpp b/tools/llvm-ar/llvm-ar.cpp
index ec3cfcb..3492396 100644
--- a/tools/llvm-ar/llvm-ar.cpp
+++ b/tools/llvm-ar/llvm-ar.cpp
@@ -338,7 +338,9 @@ static void doDisplayTable(StringRef Name, const object::Archive::Child &C) {
printMode(Mode & 007);
outs() << ' ' << C.getUID();
outs() << '/' << C.getGID();
- outs() << ' ' << format("%6llu", C.getSize());
+ ErrorOr<uint32_t> Size = C.getSize();
+ failIfError(Size.getError());
+ outs() << ' ' << format("%6llu", *Size);
outs() << ' ' << C.getLastModified().str();
outs() << ' ';
}
@@ -395,6 +397,12 @@ static bool shouldCreateArchive(ArchiveOperation Op) {
llvm_unreachable("Missing entry in covered switch.");
}
+static void Increment(object::Archive::child_iterator &I) {
+ ErrorOr<object::Archive::child_iterator> Next = I.getNext();
+ failIfError(Next.getError());
+ I = *Next;
+}
+
static void performReadOperation(ArchiveOperation Operation,
object::Archive *OldArchive) {
if (Operation == Extract && OldArchive->isThin()) {
@@ -403,7 +411,11 @@ static void performReadOperation(ArchiveOperation Operation,
}
bool Filter = !Members.empty();
- for (const object::Archive::Child &C : OldArchive->children()) {
+ ErrorOr<Archive::child_iterator> IOrErr = OldArchive->child_begin();
+ failIfError(IOrErr.getError());
+
+ for (Archive::child_iterator I = *IOrErr, E = OldArchive->child_end(); I != E;
+ Increment(I)) {
ErrorOr<StringRef> NameOrErr = C.getName();
failIfError(NameOrErr.getError());
StringRef Name = NameOrErr.get();
More information about the llvm-commits
mailing list