[llvm] r222124 - Object, COFF: Tighten the object file parser
David Majnemer
david.majnemer at gmail.com
Mon Nov 24 23:51:12 PST 2014
Regarding the contents of your t.patch:
isSectionRequiredForExecution, isSectionZeroInit and isSectionReadOnlyData
have no callers from LLVM (and are therefore untestable), they can probably
be removed from lib/Object altogether.
I replaced the check in COFFObjectFile::toSymb with a much more concise
assertion using checkOffset.
On Mon, Nov 24, 2014 at 11:00 PM, Rafael Espíndola <
rafael.espindola at gmail.com> wrote:
> This, like the other patches, seems almost entirely untested.
>
> No tests fails with the attached partial revert.
>
> Please don't add any more untested code to lib/Object. In particular,
> please don't add untested error handling code. lib/Object has a
> history of truly horrible error checking interfaces and we really have
> to test the error paths to have confidence in refactoring them.
>
>
>
> On 17 November 2014 at 06:17, David Majnemer <david.majnemer at gmail.com>
> wrote:
> > Author: majnemer
> > Date: Mon Nov 17 05:17:17 2014
> > New Revision: 222124
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=222124&view=rev
> > Log:
> > Object, COFF: Tighten the object file parser
> >
> > We were a little lax in a few areas:
> > - We pretended that import libraries were like any old COFF file, they
> > are not. In fact, they aren't really COFF files at all, we should
> > probably grow some specialized functionality to handle them smarter.
> > - Our symbol iterators were more than happy to attempt to go past the
> > end of the symbol table if you had a symbol with a bad list of
> > auxiliary symbols.
> >
> > Modified:
> > llvm/trunk/include/llvm/Object/COFF.h
> > llvm/trunk/lib/Object/COFFObjectFile.cpp
> > llvm/trunk/test/tools/llvm-readobj/file-headers.test
> > llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp
> > llvm/trunk/tools/llvm-readobj/COFFDumper.cpp
> >
> > Modified: llvm/trunk/include/llvm/Object/COFF.h
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Object/COFF.h?rev=222124&r1=222123&r2=222124&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/include/llvm/Object/COFF.h (original)
> > +++ llvm/trunk/include/llvm/Object/COFF.h Mon Nov 17 05:17:17 2014
> > @@ -508,7 +508,8 @@ public:
> > }
> > uint16_t getSizeOfOptionalHeader() const {
> > if (COFFHeader)
> > - return COFFHeader->SizeOfOptionalHeader;
> > + return COFFHeader->isImportLibrary() ? 0
> > + :
> COFFHeader->SizeOfOptionalHeader;
> > // bigobj doesn't have this field.
> > if (COFFBigObjHeader)
> > return 0;
> > @@ -516,7 +517,7 @@ public:
> > }
> > uint16_t getCharacteristics() const {
> > if (COFFHeader)
> > - return COFFHeader->Characteristics;
> > + return COFFHeader->isImportLibrary() ? 0 :
> COFFHeader->Characteristics;
> > // bigobj doesn't have characteristics to speak of,
> > // editbin will silently lie to you if you attempt to set any.
> > if (COFFBigObjHeader)
> > @@ -532,21 +533,22 @@ public:
> > }
> > uint32_t getNumberOfSections() const {
> > if (COFFHeader)
> > - return COFFHeader->NumberOfSections;
> > + return COFFHeader->isImportLibrary() ? 0 :
> COFFHeader->NumberOfSections;
> > if (COFFBigObjHeader)
> > return COFFBigObjHeader->NumberOfSections;
> > llvm_unreachable("no COFF header!");
> > }
> > uint32_t getPointerToSymbolTable() const {
> > if (COFFHeader)
> > - return COFFHeader->PointerToSymbolTable;
> > + return COFFHeader->isImportLibrary() ? 0
> > + :
> COFFHeader->PointerToSymbolTable;
> > if (COFFBigObjHeader)
> > return COFFBigObjHeader->PointerToSymbolTable;
> > llvm_unreachable("no COFF header!");
> > }
> > uint32_t getNumberOfSymbols() const {
> > if (COFFHeader)
> > - return COFFHeader->NumberOfSymbols;
> > + return COFFHeader->isImportLibrary() ? 0 :
> COFFHeader->NumberOfSymbols;
> > if (COFFBigObjHeader)
> > return COFFBigObjHeader->NumberOfSymbols;
> > llvm_unreachable("no COFF header!");
> > @@ -657,7 +659,7 @@ public:
> > return EC;
> > return COFFSymbolRef(Symb);
> > }
> > - llvm_unreachable("no symbol table pointer!");
> > + return object_error::parse_failed;
> > }
> > template <typename T>
> > std::error_code getAuxSymbol(uint32_t index, const T *&Res) const {
> >
> > Modified: llvm/trunk/lib/Object/COFFObjectFile.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/COFFObjectFile.cpp?rev=222124&r1=222123&r2=222124&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/Object/COFFObjectFile.cpp (original)
> > +++ llvm/trunk/lib/Object/COFFObjectFile.cpp Mon Nov 17 05:17:17 2014
> > @@ -54,7 +54,7 @@ static std::error_code checkOffset(Memor
> > template <typename T>
> > static std::error_code getObject(const T *&Obj, MemoryBufferRef M,
> > const void *Ptr,
> > - const size_t Size = sizeof(T)) {
> > + const uint64_t Size = sizeof(T)) {
> > uintptr_t Addr = uintptr_t(Ptr);
> > if (std::error_code EC = checkOffset(M, Addr, Size))
> > return EC;
> > @@ -101,13 +101,10 @@ const coff_symbol_type *COFFObjectFile::
> > const coff_symbol_type *Addr =
> > reinterpret_cast<const coff_symbol_type *>(Ref.p);
> >
> > + assert(!checkOffset(Data, uintptr_t(Addr), sizeof(*Addr)));
> > #ifndef NDEBUG
> > // Verify that the symbol points to a valid entry in the symbol table.
> > uintptr_t Offset = uintptr_t(Addr) - uintptr_t(base());
> > - if (Offset < getPointerToSymbolTable() ||
> > - Offset >= getPointerToSymbolTable() +
> > - (getNumberOfSymbols() * sizeof(coff_symbol_type)))
> > - report_fatal_error("Symbol was outside of symbol table.");
> >
> > assert((Offset - getPointerToSymbolTable()) %
> sizeof(coff_symbol_type) == 0 &&
> > "Symbol did not point to the beginning of a symbol");
> > @@ -133,14 +130,15 @@ const coff_section *COFFObjectFile::toSe
> > }
> >
> > void COFFObjectFile::moveSymbolNext(DataRefImpl &Ref) const {
> > + auto End = reinterpret_cast<uintptr_t>(StringTable);
> > if (SymbolTable16) {
> > const coff_symbol16 *Symb = toSymb<coff_symbol16>(Ref);
> > Symb += 1 + Symb->NumberOfAuxSymbols;
> > - Ref.p = reinterpret_cast<uintptr_t>(Symb);
> > + Ref.p = std::min(reinterpret_cast<uintptr_t>(Symb), End);
> > } else if (SymbolTable32) {
> > const coff_symbol32 *Symb = toSymb<coff_symbol32>(Ref);
> > Symb += 1 + Symb->NumberOfAuxSymbols;
> > - Ref.p = reinterpret_cast<uintptr_t>(Symb);
> > + Ref.p = std::min(reinterpret_cast<uintptr_t>(Symb), End);
> > } else {
> > llvm_unreachable("no symbol table pointer!");
> > }
> > @@ -365,8 +363,12 @@ bool COFFObjectFile::isSectionBSS(DataRe
> > }
> >
> > bool COFFObjectFile::isSectionRequiredForExecution(DataRefImpl Ref)
> const {
> > - // FIXME: Unimplemented
> > - return true;
> > + // Sections marked 'Info', 'Remove', or 'Discardable' aren't required
> for
> > + // execution.
> > + const coff_section *Sec = toSec(Ref);
> > + return !(Sec->Characteristics &
> > + (COFF::IMAGE_SCN_LNK_INFO | COFF::IMAGE_SCN_LNK_REMOVE |
> > + COFF::IMAGE_SCN_MEM_DISCARDABLE));
> > }
> >
> > bool COFFObjectFile::isSectionVirtual(DataRefImpl Ref) const {
> > @@ -375,13 +377,22 @@ bool COFFObjectFile::isSectionVirtual(Da
> > }
> >
> > bool COFFObjectFile::isSectionZeroInit(DataRefImpl Ref) const {
> > - // FIXME: Unimplemented.
> > - return false;
> > + const coff_section *Sec = toSec(Ref);
> > + return Sec->Characteristics & COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA;
> > }
> >
> > bool COFFObjectFile::isSectionReadOnlyData(DataRefImpl Ref) const {
> > - // FIXME: Unimplemented.
> > - return false;
> > + const coff_section *Sec = toSec(Ref);
> > + // Check if it's any sort of data section.
> > + if (!(Sec->Characteristics & (COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA |
> > + COFF::IMAGE_SCN_CNT_INITIALIZED_DATA)))
> > + return false;
> > + // If it's writable or executable or contains code, it isn't
> read-only data.
> > + if (Sec->Characteristics &
> > + (COFF::IMAGE_SCN_CNT_CODE | COFF::IMAGE_SCN_MEM_EXECUTE |
> > + COFF::IMAGE_SCN_MEM_WRITE))
> > + return false;
> > + return true;
> > }
> >
> > bool COFFObjectFile::sectionContainsSymbol(DataRefImpl SecRef,
> > @@ -446,15 +457,15 @@ relocation_iterator COFFObjectFile::sect
> > // Initialize the pointer to the symbol table.
> > std::error_code COFFObjectFile::initSymbolTablePtr() {
> > if (COFFHeader)
> > - if (std::error_code EC =
> > - getObject(SymbolTable16, Data, base() +
> getPointerToSymbolTable(),
> > - getNumberOfSymbols() * getSymbolTableEntrySize()))
> > + if (std::error_code EC = getObject(
> > + SymbolTable16, Data, base() + getPointerToSymbolTable(),
> > + (uint64_t)getNumberOfSymbols() * getSymbolTableEntrySize()))
> > return EC;
> >
> > if (COFFBigObjHeader)
> > - if (std::error_code EC =
> > - getObject(SymbolTable32, Data, base() +
> getPointerToSymbolTable(),
> > - getNumberOfSymbols() * getSymbolTableEntrySize()))
> > + if (std::error_code EC = getObject(
> > + SymbolTable32, Data, base() + getPointerToSymbolTable(),
> > + (uint64_t)getNumberOfSymbols() * getSymbolTableEntrySize()))
> > return EC;
> >
> > // Find string table. The first four byte of the string table
> contains the
> > @@ -681,13 +692,20 @@ COFFObjectFile::COFFObjectFile(MemoryBuf
> > }
> >
> > if ((EC = getObject(SectionTable, Data, base() + CurPtr,
> > - getNumberOfSections() * sizeof(coff_section))))
> > + (uint64_t)getNumberOfSections() *
> sizeof(coff_section))))
> > return;
> >
> > // Initialize the pointer to the symbol table.
> > - if (getPointerToSymbolTable() != 0)
> > + if (getPointerToSymbolTable() != 0) {
> > if ((EC = initSymbolTablePtr()))
> > return;
> > + } else {
> > + // We had better not have any symbols if we don't have a symbol
> table.
> > + if (getNumberOfSymbols() != 0) {
> > + EC = object_error::parse_failed;
> > + return;
> > + }
> > + }
> >
> > // Initialize the pointer to the beginning of the import table.
> > if ((EC = initImportTablePtr()))
> > @@ -843,15 +861,15 @@ COFFObjectFile::getDataDirectory(uint32_
> >
> > std::error_code COFFObjectFile::getSection(int32_t Index,
> > const coff_section *&Result)
> const {
> > - // Check for special index values.
> > + Result = nullptr;
> > if (COFF::isReservedSectionNumber(Index))
> > - Result = nullptr;
> > - else if (Index > 0 && static_cast<uint32_t>(Index) <=
> getNumberOfSections())
> > + return object_error::success;
> > + if (static_cast<uint32_t>(Index) <= getNumberOfSections()) {
> > // We already verified the section table data, so no need to check
> again.
> > Result = SectionTable + (Index - 1);
> > - else
> > - return object_error::parse_failed;
> > - return object_error::success;
> > + return object_error::success;
> > + }
> > + return object_error::parse_failed;
> > }
> >
> > std::error_code COFFObjectFile::getString(uint32_t Offset,
> > @@ -1001,6 +1019,8 @@ std::error_code COFFObjectFile::getReloc
> > symbol_iterator COFFObjectFile::getRelocationSymbol(DataRefImpl Rel)
> const {
> > const coff_relocation *R = toRel(Rel);
> > DataRefImpl Ref;
> > + if (R->SymbolTableIndex >= getNumberOfSymbols())
> > + return symbol_end();
> > if (SymbolTable16)
> > Ref.p = reinterpret_cast<uintptr_t>(SymbolTable16 +
> R->SymbolTableIndex);
> > else if (SymbolTable32)
> >
> > Modified: llvm/trunk/test/tools/llvm-readobj/file-headers.test
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-readobj/file-headers.test?rev=222124&r1=222123&r2=222124&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/test/tools/llvm-readobj/file-headers.test (original)
> > +++ llvm/trunk/test/tools/llvm-readobj/file-headers.test Mon Nov 17
> 05:17:17 2014
> > @@ -335,12 +335,11 @@ COFF-IMPORTLIB-NEXT: Arch: unknown
> > COFF-IMPORTLIB-NEXT: AddressSize: 32bit
> > COFF-IMPORTLIB-NEXT: ImageFileHeader {
> > COFF-IMPORTLIB-NEXT: Machine: IMAGE_FILE_MACHINE_UNKNOWN (0x0)
> > -COFF-IMPORTLIB-NEXT: SectionCount: 65535
> > +COFF-IMPORTLIB-NEXT: SectionCount: 0
> > COFF-IMPORTLIB-NEXT: TimeDateStamp: 1970-09-09 19:52:32 (0x14C0000)
> > -COFF-IMPORTLIB-NEXT: PointerToSymbolTable: 0x528542EB
> > -COFF-IMPORTLIB-NEXT: SymbolCount: 20
> > +COFF-IMPORTLIB-NEXT: PointerToSymbolTable: 0x0
> > +COFF-IMPORTLIB-NEXT: SymbolCount: 0
> > COFF-IMPORTLIB-NEXT: OptionalHeaderSize: 0
> > -COFF-IMPORTLIB-NEXT: Characteristics [ (0x8)
> > -COFF-IMPORTLIB-NEXT: IMAGE_FILE_LOCAL_SYMS_STRIPPED (0x8)
> > +COFF-IMPORTLIB-NEXT: Characteristics [ (0x0)
> > COFF-IMPORTLIB-NEXT: ]
> > COFF-IMPORTLIB-NEXT: }
> >
> > Modified: llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp?rev=222124&r1=222123&r2=222124&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp (original)
> > +++ llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp Mon Nov 17 05:17:17
> 2014
> > @@ -307,8 +307,7 @@ static void DisassembleObject(const Obje
> > }
> >
> > for (const SectionRef &Section : Obj->sections()) {
> > - bool Text = Section.isText();
> > - if (!Text)
> > + if (!Section.isText() || Section.isVirtual())
> > continue;
> >
> > uint64_t SectionAddr = Section.getAddress();
> >
> > Modified: llvm/trunk/tools/llvm-readobj/COFFDumper.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-readobj/COFFDumper.cpp?rev=222124&r1=222123&r2=222124&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/tools/llvm-readobj/COFFDumper.cpp (original)
> > +++ llvm/trunk/tools/llvm-readobj/COFFDumper.cpp Mon Nov 17 05:17:17 2014
> > @@ -825,22 +825,22 @@ void COFFDumper::printSymbols() {
> >
> > void COFFDumper::printDynamicSymbols() { ListScope Group(W,
> "DynamicSymbols"); }
> >
> > -static StringRef getSectionName(const llvm::object::COFFObjectFile *Obj,
> > - COFFSymbolRef Symbol,
> > - const coff_section *Section) {
> > +static ErrorOr<StringRef>
> > +getSectionName(const llvm::object::COFFObjectFile *Obj, int32_t
> SectionNumber,
> > + const coff_section *Section) {
> > if (Section) {
> > StringRef SectionName;
> > - Obj->getSectionName(Section, SectionName);
> > + if (std::error_code EC = Obj->getSectionName(Section, SectionName))
> > + return EC;
> > return SectionName;
> > }
> > - int32_t SectionNumber = Symbol.getSectionNumber();
> > if (SectionNumber == llvm::COFF::IMAGE_SYM_DEBUG)
> > - return "IMAGE_SYM_DEBUG";
> > + return StringRef("IMAGE_SYM_DEBUG");
> > if (SectionNumber == llvm::COFF::IMAGE_SYM_ABSOLUTE)
> > - return "IMAGE_SYM_ABSOLUTE";
> > + return StringRef("IMAGE_SYM_ABSOLUTE");
> > if (SectionNumber == llvm::COFF::IMAGE_SYM_UNDEFINED)
> > - return "IMAGE_SYM_UNDEFINED";
> > - return "";
> > + return StringRef("IMAGE_SYM_UNDEFINED");
> > + return StringRef("");
> > }
> >
> > void COFFDumper::printSymbol(const SymbolRef &Sym) {
> > @@ -858,7 +858,11 @@ void COFFDumper::printSymbol(const Symbo
> > if (Obj->getSymbolName(Symbol, SymbolName))
> > SymbolName = "";
> >
> > - StringRef SectionName = getSectionName(Obj, Symbol, Section);
> > + StringRef SectionName = "";
> > + ErrorOr<StringRef> Res =
> > + getSectionName(Obj, Symbol.getSectionNumber(), Section);
> > + if (Res)
> > + SectionName = *Res;
> >
> > W.printString("Name", SymbolName);
> > W.printNumber("Value", Symbol.getValue());
> > @@ -929,10 +933,14 @@ void COFFDumper::printSymbol(const Symbo
> > if (Section && Section->Characteristics &
> COFF::IMAGE_SCN_LNK_COMDAT
> > && Aux->Selection == COFF::IMAGE_COMDAT_SELECT_ASSOCIATIVE) {
> > const coff_section *Assoc;
> > - StringRef AssocName;
> > - std::error_code EC;
> > - if ((EC = Obj->getSection(AuxNumber, Assoc)) ||
> > - (EC = Obj->getSectionName(Assoc, AssocName))) {
> > + StringRef AssocName = "";
> > + std::error_code EC = Obj->getSection(AuxNumber, Assoc);
> > + ErrorOr<StringRef> Res = getSectionName(Obj, AuxNumber, Assoc);
> > + if (Res)
> > + AssocName = *Res;
> > + if (!EC)
> > + EC = Res.getError();
> > + if (EC) {
> > AssocName = "";
> > error(EC);
> > }
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141125/901da6b7/attachment.html>
More information about the llvm-commits
mailing list