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