<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Jul 29, 2016 at 10:51 AM Kevin Enderby via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: enderby<br>
Date: Fri Jul 29 12:44:13 2016<br>
New Revision: 277177<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=277177&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=277177&view=rev</a><br>
Log:<br>
The next step along the way to getting good error messages for bad archives.<br>
<br>
As mentioned in commit log for r276686 this next step is adding a new<br>
method in the ArchiveMemberHeader class to get the full name that<br>
does proper error checking, and can be use for error messages.<br>
<br>
To do this the name of ArchiveMemberHeader::getName() is changed to<br>
ArchiveMemberHeader::getRawName() to be consistent with<br>
Archive::Child::getRawName(). Then the “new” method is the addition<br>
of a new implementation of ArchiveMemberHeader::getName() which gets<br>
the full name and provides proper error checking. Which is mostly a rewrite<br>
of what was Archive::Child::getName() and cleaning up incorrect uses of<br>
llvm_unreachable() in the code which were actually just cases of errors<br>
in the input Archives.<br>
<br>
Then Archive::Child::getName() is changed to return Expected<> and use<br>
the new implementation of ArchiveMemberHeader::getName() .<br>
<br>
Also needed to change Archive::getMemoryBufferRef() with these<br>
changes to return Expected<> as well to propagate Errors up.<br>
As well as changing Archive::isThinMember() to return Expected<> .<br>
<br>
Added:<br>
llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus10.a<br>
llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus6.a (with props)<br>
llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus7.a<br>
llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus8.a<br>
llvm/trunk/test/tools/llvm-objdump/Inputs/libbogus9.a<br>
Modified:<br>
llvm/trunk/include/llvm/Object/Archive.h<br>
llvm/trunk/lib/Object/Archive.cpp<br>
llvm/trunk/lib/Object/ArchiveWriter.cpp<br>
llvm/trunk/test/tools/llvm-objdump/malformed-archives.test<br>
llvm/trunk/tools/dsymutil/BinaryHolder.cpp<br>
llvm/trunk/tools/llvm-ar/llvm-ar.cpp<br>
llvm/trunk/tools/llvm-nm/llvm-nm.cpp<br>
llvm/trunk/tools/llvm-objdump/MachODump.cpp<br>
llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp<br>
llvm/trunk/tools/llvm-size/llvm-size.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/Object/Archive.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Object/Archive.h?rev=277177&r1=277176&r2=277177&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Object/Archive.h?rev=277177&r1=277176&r2=277177&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/Object/Archive.h (original)<br>
+++ llvm/trunk/include/llvm/Object/Archive.h Fri Jul 29 12:44:13 2016<br>
@@ -36,7 +36,10 @@ public:<br>
// ArchiveMemberHeader() = default;<br>
<br>
/// Get the name without looking up long names.<br>
- llvm::StringRef getName() const;<br>
+ Expected<llvm::StringRef> getRawName() const;<br>
+<br>
+ /// Get the name looking up long names.<br>
+ Expected<llvm::StringRef> getName(uint64_t Size) const;<br>
<br>
/// Members are not larger than 4GB.<br>
Expected<uint32_t> getSize() const;<br>
@@ -82,7 +85,7 @@ public:<br>
/// \brief Offset from Data to the start of the file.<br>
uint16_t StartOfFile;<br>
<br>
- bool isThinMember() const;<br>
+ Expected<bool> isThinMember() const;<br>
<br>
public:<br>
Child(const Archive *Parent, const char *Start, Error *Err);<br>
@@ -96,9 +99,9 @@ public:<br>
const Archive *getParent() const { return Parent; }<br>
Expected<Child> getNext() const;<br>
<br>
- ErrorOr<StringRef> getName() const;<br>
+ Expected<StringRef> getName() const;<br>
ErrorOr<std::string> getFullName() const;<br>
- StringRef getRawName() const { return Header.getName(); }<br>
+ Expected<StringRef> getRawName() const { return Header.getRawName(); }<br>
sys::TimeValue getLastModified() const {<br>
return Header.getLastModified();<br>
}<br>
@@ -118,7 +121,7 @@ public:<br>
ErrorOr<StringRef> getBuffer() const;<br>
uint64_t getChildOffset() const;<br>
<br>
- ErrorOr<MemoryBufferRef> getMemoryBufferRef() const;<br>
+ Expected<MemoryBufferRef> getMemoryBufferRef() const;<br>
<br>
Expected<std::unique_ptr<Binary>><br>
getAsBinary(LLVMContext *Context = nullptr) const;<br>
@@ -238,6 +241,7 @@ public:<br>
<br>
bool hasSymbolTable() const;<br>
StringRef getSymbolTable() const { return SymbolTable; }<br>
+ StringRef getStringTable() const { return StringTable; }<br>
uint32_t getNumberOfSymbols() const;<br>
<br>
std::vector<std::unique_ptr<MemoryBuffer>> takeThinBuffers() {<br>
<br>
Modified: llvm/trunk/lib/Object/Archive.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/Archive.cpp?rev=277177&r1=277176&r2=277177&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/Archive.cpp?rev=277177&r1=277176&r2=277177&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Object/Archive.cpp (original)<br>
+++ llvm/trunk/lib/Object/Archive.cpp Fri Jul 29 12:44:13 2016<br>
@@ -43,17 +43,17 @@ ArchiveMemberHeader::ArchiveMemberHeader<br>
return;<br>
ErrorAsOutParameter ErrAsOutParam(Err);<br>
<br>
- // TODO: For errors messages with the ArchiveMemberHeader class use the<br>
- // archive member name instead of the the offset to the archive member header.<br>
- // When there is also error getting the member name then use the offset to<br>
- // the member in the message.<br>
-<br>
if (Size < sizeof(ArMemHdrType)) {<br>
if (Err) {<br>
- uint64_t Offset = RawHeaderPtr - Parent->getData().data();<br>
- *Err = malformedError("remaining size of archive too small for next "<br>
- "archive member header at offset " +<br>
- Twine(Offset));<br>
+ Twine Msg("remaining size of archive too small for next archive member "<br>
+ "header ");<br>
+ Expected<StringRef> NameOrErr = getName(Size);<br>
+ if (!NameOrErr) {<br>
+ consumeError(NameOrErr.takeError());<br>
+ uint64_t Offset = RawHeaderPtr - Parent->getData().data();<br>
+ *Err = malformedError(Msg + "at offset " + Twine(Offset));<br>
+ } else<br>
+ *Err = malformedError(Msg + "for " + Twine(NameOrErr.get()));<br>
}<br>
return;<br>
}<br>
@@ -64,18 +64,35 @@ ArchiveMemberHeader::ArchiveMemberHeader<br>
OS.write_escaped(llvm::StringRef(ArMemHdr->Terminator,<br>
sizeof(ArMemHdr->Terminator)));<br>
OS.flush();<br>
- uint64_t Offset = RawHeaderPtr - Parent->getData().data();<br>
- *Err = malformedError("terminator characters in archive member \"" + Buf +<br>
- "\" not the correct \"`\\n\" values for the "<br>
- "archive member header at offset " + Twine(Offset));<br>
+ Twine Msg("terminator characters in archive member \"" + Buf + "\" not "<br>
+ "the correct \"`\\n\" values for the archive member header ");<br>
+ Expected<StringRef> NameOrErr = getName(Size);<br>
+ if (!NameOrErr) {<br>
+ consumeError(NameOrErr.takeError());<br>
+ uint64_t Offset = RawHeaderPtr - Parent->getData().data();<br>
+ *Err = malformedError(Msg + "at offset " + Twine(Offset));<br>
+ } else<br>
+ *Err = malformedError(Msg + "for " + Twine(NameOrErr.get()));<br>
}<br>
return;<br>
}<br>
}<br>
<br>
-StringRef ArchiveMemberHeader::getName() const {<br>
+// This gets the raw name from the ArMemHdr->Name field and checks that it is<br>
+// valid for the kind of archive. If it is not valid it returns an Error.<br>
+Expected<StringRef> ArchiveMemberHeader::getRawName() const {<br>
char EndCond;<br>
- if (ArMemHdr->Name[0] == '/' || ArMemHdr->Name[0] == '#')<br>
+ auto Kind = Parent->kind();<br>
+ if (Kind == Archive::K_BSD || Kind == Archive::K_DARWIN64) {<br>
+ if (ArMemHdr->Name[0] == ' ') {<br>
+ uint64_t Offset = reinterpret_cast<const char *>(ArMemHdr) -<br>
+ Parent->getData().data();<br>
+ return malformedError("name contains a leading space for archive member "<br>
+ "header at offset " + Twine(Offset));<br>
+ }<br>
+ EndCond = ' ';<br>
+ }<br>
+ else if (ArMemHdr->Name[0] == '/' || ArMemHdr->Name[0] == '#')<br>
EndCond = ' ';<br>
else<br>
EndCond = '/';<br>
@@ -88,6 +105,105 @@ StringRef ArchiveMemberHeader::getName()<br>
return llvm::StringRef(ArMemHdr->Name, end);<br>
}<br>
<br>
+// This gets the name looking up long names. Size is the size of the archive<br>
+// member including the header, so the size of any name following the header<br>
+// is checked to make sure it does not overflow.<br>
+Expected<StringRef> ArchiveMemberHeader::getName(uint64_t Size) const {<br>
+<br>
+ // This can be called from the ArchiveMemberHeader constructor when the<br>
+ // archive header is truncated to produce an error message with the name.<br>
+ // Make sure the name field is not truncated.<br>
+ if (Size < offsetof(ArMemHdrType, Name) + sizeof(ArMemHdr->Name)) {<br>
+ uint64_t ArchiveOffset = reinterpret_cast<const char *>(ArMemHdr) -<br>
+ Parent->getData().data();<br>
+ return malformedError("archive header truncated before the name field "<br>
+ "for archive member header at offset " +<br>
+ Twine(ArchiveOffset));<br>
+ }<br>
+<br>
+ // The raw name itself can be invalid.<br>
+ Expected<StringRef> NameOrErr = getRawName();<br>
+ if (!NameOrErr)<br>
+ return NameOrErr.takeError();<br>
+ StringRef Name = NameOrErr.get();<br>
+<br>
+ // Check if it's a special name.<br>
+ if (Name[0] == '/') {<br>
+ if (Name.size() == 1) // Linker member.<br>
+ return Name;<br>
+ if (Name.size() == 2 && Name[1] == '/') // String table.<br>
+ return Name;<br>
+ // It's a long name.<br>
+ // Get the string table offset.<br>
+ std::size_t StringOffset;<br>
+ if (Name.substr(1).rtrim(' ').getAsInteger(10, StringOffset)) {<br>
+ std::string Buf;<br>
+ raw_string_ostream OS(Buf);<br>
+ OS.write_escaped(Name.substr(1).rtrim(' '),<br>
+ sizeof(Name.substr(1)).rtrim(' '));<br></blockquote><div>Is this a bug? Do you mean to write .length()? Or .empty()? Regardless of whether the logic is correct, this gives warning on MSVC because it's converting an integer to a bool. So you should make it explicit with !! or something.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ OS.flush();<br>
+ uint64_t ArchiveOffset = reinterpret_cast<const char *>(ArMemHdr) -<br>
+ Parent->getData().data();<br>
+ return malformedError("long name offset characters after the '/' are "<br>
+ "not all decimal numbers: '" + Buf + "' for "<br>
+ "archive member header at offset " +<br>
+ Twine(ArchiveOffset));<br>
+ }<br>
+<br>
+ // Verify it.<br>
+ if (StringOffset >= Parent->getStringTable().size()) {<br>
+ uint64_t ArchiveOffset = reinterpret_cast<const char *>(ArMemHdr) -<br>
+ Parent->getData().data();<br>
+ return malformedError("long name offset " + Twine(StringOffset) + " past "<br>
+ "the end of the string table for archive member "<br>
+ "header at offset " + Twine(ArchiveOffset));<br>
+ }<br>
+ const char *addr = Parent->getStringTable().begin() + StringOffset;<br>
+<br>
+ // GNU long file names end with a "/\n".<br>
+ if (Parent->kind() == Archive::K_GNU ||<br>
+ Parent->kind() == Archive::K_MIPS64) {<br>
+ StringRef::size_type End = StringRef(addr).find('\n');<br>
+ return StringRef(addr, End - 1);<br>
+ }<br>
+ return StringRef(addr);<br>
+ } else if (Name.startswith("#1/")) {<br>
+ uint64_t NameLength;<br>
+ if (Name.substr(3).rtrim(' ').getAsInteger(10, NameLength)) {<br>
+ std::string Buf;<br>
+ raw_string_ostream OS(Buf);<br>
+ OS.write_escaped(Name.substr(3).rtrim(' '),<br>
+ sizeof(Name.substr(3)).rtrim(' '));<br></blockquote><div>Same here.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ OS.flush();<br>
+ uint64_t ArchiveOffset = reinterpret_cast<const char *>(ArMemHdr) -<br>
+ Parent->getData().data();<br>
+ return malformedError("long name length characters after the #1/ are "<br>
+ "not all decimal numbers: '" + Buf + "' for "<br>
+ "archive member header at offset " +<br>
+ Twine(ArchiveOffset));<br>
+ }<br>
+ if (getSizeOf() + NameLength > Size) {<br>
+ uint64_t ArchiveOffset = reinterpret_cast<const char *>(ArMemHdr) -<br>
+ Parent->getData().data();<br>
+ return malformedError("long name length: " + Twine(NameLength) +<br>
+ " extends past the end of the member or archive "<br>
+ "for archive member header at offset " +<br>
+ Twine(ArchiveOffset));<br>
+ }<br>
+ return StringRef(reinterpret_cast<const char *>(ArMemHdr) + getSizeOf(),<br>
+ NameLength).rtrim('\0');<br>
+ } else {<br>
+ // It is not a long name so trim the blanks at the end of the name.<br>
+ if (Name[Name.size() - 1] != '/') {<br>
+ return Name.rtrim(' ');<br>
+ }<br>
+ }<br>
+ // It's a simple name.<br>
+ if (Name[Name.size() - 1] == '/')<br>
+ return Name.substr(0, Name.size() - 1);<br>
+ return Name;<br>
+}<br>
+<br>
Expected<uint32_t> ArchiveMemberHeader::getSize() const {<br>
uint32_t Ret;<br>
if (llvm::StringRef(ArMemHdr->Size,<br>
@@ -166,7 +282,14 @@ Archive::Child::Child(const Archive *Par<br>
<br>
uint64_t Size = Header.getSizeOf();<br>
Data = StringRef(Start, Size);<br>
- if (!isThinMember()) {<br>
+ Expected<bool> isThinOrErr = isThinMember();<br>
+ if (!isThinOrErr) {<br>
+ if (Err)<br>
+ *Err = isThinOrErr.takeError();<br>
+ return;<br>
+ }<br>
+ bool isThin = isThinOrErr.get();<br>
+ if (!isThin) {<br>
Expected<uint64_t> MemberSize = getRawSize();<br>
if (!MemberSize) {<br>
if (Err)<br>
@@ -180,11 +303,30 @@ Archive::Child::Child(const Archive *Par<br>
// Setup StartOfFile and PaddingBytes.<br>
StartOfFile = Header.getSizeOf();<br>
// Don't include attached name.<br>
- StringRef Name = getRawName();<br>
+ Expected<StringRef> NameOrErr = getRawName();<br>
+ if (!NameOrErr){<br>
+ if (Err)<br>
+ *Err = NameOrErr.takeError();<br>
+ return;<br>
+ }<br>
+ StringRef Name = NameOrErr.get();<br>
if (Name.startswith("#1/")) {<br>
uint64_t NameSize;<br>
- if (Name.substr(3).rtrim(' ').getAsInteger(10, NameSize))<br>
- llvm_unreachable("Long name length is not an integer");<br>
+ if (Name.substr(3).rtrim(' ').getAsInteger(10, NameSize)) {<br>
+ if (Err) {<br>
+ std::string Buf;<br>
+ raw_string_ostream OS(Buf);<br>
+ OS.write_escaped(Name.substr(3).rtrim(' '),<br>
+ sizeof(Name.substr(3)).rtrim(' '));<br></blockquote><div>And here.</div><div> </div></div></div>