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