From d947f15959b72249bbe60ce44d5648b169ca62fe Mon Sep 17 00:00:00 2001 From: Zachary Turner Date: Wed, 3 May 2017 05:34:00 +0000 Subject: [PATCH] Fix use after free in BinaryStream library. This was reported by the ASAN bot, and it turned out to be a fairly fundamental problem with the design of VarStreamArray and the way it passes context information to the extractor. The fix was cumbersome, and I'm not entirely pleased with it, so I plan to revisit this design in the future when I'm not pressed to get the bots green again. For now, this fixes the issue by storing the context information by value instead of by reference, and introduces some impossibly-confusing template magic to make things "work". git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@301999 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/llvm/DebugInfo/CodeView/CVRecord.h | 2 +- .../ModuleDebugFileChecksumFragment.h | 6 +- .../CodeView/ModuleDebugFragmentRecord.h | 8 +- .../ModuleDebugInlineeLinesFragment.h | 5 +- .../CodeView/ModuleDebugLineFragment.h | 4 +- .../PDB/Native/DbiModuleDescriptor.h | 2 +- include/llvm/Support/BinaryStreamArray.h | 232 +++++++++++------- include/llvm/Support/BinaryStreamReader.h | 24 +- .../ModuleDebugFileChecksumFragment.cpp | 2 +- .../ModuleDebugInlineeLinesFragment.cpp | 7 +- .../PDB/Native/DbiModuleDescriptorBuilder.cpp | 8 + unittests/Support/BinaryStreamTest.cpp | 14 +- 12 files changed, 198 insertions(+), 116 deletions(-) diff --git a/include/llvm/DebugInfo/CodeView/CVRecord.h b/include/llvm/DebugInfo/CodeView/CVRecord.h index 086d6dff11c..ac8aaafeadc 100644 --- a/include/llvm/DebugInfo/CodeView/CVRecord.h +++ b/include/llvm/DebugInfo/CodeView/CVRecord.h @@ -53,7 +53,7 @@ struct VarStreamArrayExtractor> { typedef void ContextType; static Error extract(BinaryStreamRef Stream, uint32_t &Len, - codeview::CVRecord &Item, void *Ctx) { + codeview::CVRecord &Item) { using namespace codeview; const RecordPrefix *Prefix = nullptr; BinaryStreamReader Reader(Stream); diff --git a/include/llvm/DebugInfo/CodeView/ModuleDebugFileChecksumFragment.h b/include/llvm/DebugInfo/CodeView/ModuleDebugFileChecksumFragment.h index a5a3b851b84..10cea27cc92 100644 --- a/include/llvm/DebugInfo/CodeView/ModuleDebugFileChecksumFragment.h +++ b/include/llvm/DebugInfo/CodeView/ModuleDebugFileChecksumFragment.h @@ -35,7 +35,7 @@ public: typedef void ContextType; static Error extract(BinaryStreamRef Stream, uint32_t &Len, - codeview::FileChecksumEntry &Item, void *Ctx); + codeview::FileChecksumEntry &Item); }; } @@ -55,8 +55,8 @@ public: Error initialize(BinaryStreamReader Reader); - Iterator begin() const { return Checksums.begin(); } - Iterator end() const { return Checksums.end(); } + Iterator begin() { return Checksums.begin(); } + Iterator end() { return Checksums.end(); } const FileChecksumArray &getArray() const { return Checksums; } diff --git a/include/llvm/DebugInfo/CodeView/ModuleDebugFragmentRecord.h b/include/llvm/DebugInfo/CodeView/ModuleDebugFragmentRecord.h index b98c8605592..f68f21b224f 100644 --- a/include/llvm/DebugInfo/CodeView/ModuleDebugFragmentRecord.h +++ b/include/llvm/DebugInfo/CodeView/ModuleDebugFragmentRecord.h @@ -57,8 +57,6 @@ private: ModuleDebugFragment &Frag; }; -typedef VarStreamArray ModuleDebugFragmentArray; - } // namespace codeview template <> @@ -66,13 +64,17 @@ struct VarStreamArrayExtractor { typedef void ContextType; static Error extract(BinaryStreamRef Stream, uint32_t &Length, - codeview::ModuleDebugFragmentRecord &Info, void *Ctx) { + codeview::ModuleDebugFragmentRecord &Info) { if (auto EC = codeview::ModuleDebugFragmentRecord::initialize(Stream, Info)) return EC; Length = Info.getRecordLength(); return Error::success(); } }; + +namespace codeview { +typedef VarStreamArray ModuleDebugFragmentArray; +} } // namespace llvm #endif // LLVM_DEBUGINFO_CODEVIEW_MODULEDEBUGFRAGMENTRECORD_H diff --git a/include/llvm/DebugInfo/CodeView/ModuleDebugInlineeLinesFragment.h b/include/llvm/DebugInfo/CodeView/ModuleDebugInlineeLinesFragment.h index 177367c111c..36675f49825 100644 --- a/include/llvm/DebugInfo/CodeView/ModuleDebugInlineeLinesFragment.h +++ b/include/llvm/DebugInfo/CodeView/ModuleDebugInlineeLinesFragment.h @@ -42,11 +42,10 @@ struct InlineeSourceLine { } template <> struct VarStreamArrayExtractor { - typedef codeview::ModuleDebugInlineeLineFragmentRef ContextType; + typedef bool ContextType; static Error extract(BinaryStreamRef Stream, uint32_t &Len, - codeview::InlineeSourceLine &Item, - ContextType *Fragment); + codeview::InlineeSourceLine &Item, bool HasExtraFiles); }; namespace codeview { diff --git a/include/llvm/DebugInfo/CodeView/ModuleDebugLineFragment.h b/include/llvm/DebugInfo/CodeView/ModuleDebugLineFragment.h index dcfe86dd850..dfd8b7c2921 100644 --- a/include/llvm/DebugInfo/CodeView/ModuleDebugLineFragment.h +++ b/include/llvm/DebugInfo/CodeView/ModuleDebugLineFragment.h @@ -61,10 +61,10 @@ struct LineColumnEntry { class LineColumnExtractor { public: - typedef const LineFragmentHeader ContextType; + typedef const LineFragmentHeader *ContextType; static Error extract(BinaryStreamRef Stream, uint32_t &Len, - LineColumnEntry &Item, const LineFragmentHeader *Header); + LineColumnEntry &Item, const LineFragmentHeader *Ctx); }; class ModuleDebugLineFragmentRef final : public ModuleDebugFragmentRef { diff --git a/include/llvm/DebugInfo/PDB/Native/DbiModuleDescriptor.h b/include/llvm/DebugInfo/PDB/Native/DbiModuleDescriptor.h index 879cb4285cd..d1f791b9dae 100644 --- a/include/llvm/DebugInfo/PDB/Native/DbiModuleDescriptor.h +++ b/include/llvm/DebugInfo/PDB/Native/DbiModuleDescriptor.h @@ -66,7 +66,7 @@ struct ModuleInfoEx { template <> struct VarStreamArrayExtractor { typedef void ContextType; static Error extract(BinaryStreamRef Stream, uint32_t &Length, - pdb::DbiModuleDescriptor &Info, void *Ctx) { + pdb::DbiModuleDescriptor &Info) { if (auto EC = pdb::DbiModuleDescriptor::initialize(Stream, Info)) return EC; Length = Info.getRecordLength(); diff --git a/include/llvm/Support/BinaryStreamArray.h b/include/llvm/Support/BinaryStreamArray.h index 748a62be231..93de1359766 100644 --- a/include/llvm/Support/BinaryStreamArray.h +++ b/include/llvm/Support/BinaryStreamArray.h @@ -42,99 +42,34 @@ namespace llvm { /// having to specify a second template argument to VarStreamArray (documented /// below). template struct VarStreamArrayExtractor { - typedef void Context; + struct ContextType {}; // Method intentionally deleted. You must provide an explicit specialization - // with the following method implemented. - static Error extract(BinaryStreamRef Stream, uint32_t &Len, T &Item, - Context *Ctx) = delete; -}; - -/// VarStreamArray represents an array of variable length records backed by a -/// stream. This could be a contiguous sequence of bytes in memory, it could -/// be a file on disk, or it could be a PDB stream where bytes are stored as -/// discontiguous blocks in a file. Usually it is desirable to treat arrays -/// as contiguous blocks of memory, but doing so with large PDB files, for -/// example, could mean allocating huge amounts of memory just to allow -/// re-ordering of stream data to be contiguous before iterating over it. By -/// abstracting this out, we need not duplicate this memory, and we can -/// iterate over arrays in arbitrarily formatted streams. Elements are parsed -/// lazily on iteration, so there is no upfront cost associated with building -/// or copying a VarStreamArray, no matter how large it may be. -/// -/// You create a VarStreamArray by specifying a ValueType and an Extractor type. -/// If you do not specify an Extractor type, you are expected to specialize -/// VarStreamArrayExtractor for your ValueType. -/// -/// The default extractor type is stateless, but by specializing -/// VarStreamArrayExtractor or defining your own custom extractor type and -/// adding the appropriate ContextType typedef to the class, you can pass a -/// context field during construction of the VarStreamArray that will be -/// passed to each call to extract. -/// -template -class VarStreamArrayIterator; - -template > -class VarStreamArray { -public: - typedef typename ExtractorType::ContextType ContextType; - typedef VarStreamArrayIterator Iterator; - friend Iterator; + // with one of the following two methods implemented. + static Error extract(BinaryStreamRef Stream, uint32_t &Len, T &Item) = delete; - VarStreamArray() = default; - - explicit VarStreamArray(BinaryStreamRef Stream, - ContextType *Context = nullptr) - : Stream(Stream), Context(Context) {} - - VarStreamArray(const VarStreamArray &Other) - : Stream(Other.Stream), Context(Other.Context) {} - - Iterator begin(bool *HadError = nullptr) const { - if (empty()) - return end(); - - return Iterator(*this, Context, HadError); - } - - Iterator end() const { return Iterator(); } - - bool empty() const { return Stream.getLength() == 0; } - - /// \brief given an offset into the array's underlying stream, return an - /// iterator to the record at that offset. This is considered unsafe - /// since the behavior is undefined if \p Offset does not refer to the - /// beginning of a valid record. - Iterator at(uint32_t Offset) const { - return Iterator(*this, Context, Stream.drop_front(Offset), nullptr); - } - - BinaryStreamRef getUnderlyingStream() const { return Stream; } - -private: - BinaryStreamRef Stream; - ContextType *Context = nullptr; + static Error extract(BinaryStreamRef Stream, uint32_t &Len, T &Item, + const ContextType &Ctx) = delete; }; -template +template class VarStreamArrayIterator : public iterator_facade_base< - VarStreamArrayIterator, - std::forward_iterator_tag, ValueType> { - typedef typename ExtractorType::ContextType ContextType; - typedef VarStreamArrayIterator IterType; - typedef VarStreamArray ArrayType; + VarStreamArrayIterator, + std::forward_iterator_tag, Value> { + typedef VarStreamArrayIterator + IterType; public: - VarStreamArrayIterator(const ArrayType &Array, ContextType *Context, + VarStreamArrayIterator() = default; + VarStreamArrayIterator(const ArrayType &Array, const WrappedCtx &Ctx, BinaryStreamRef Stream, bool *HadError = nullptr) - : IterRef(Stream), Context(Context), Array(&Array), HadError(HadError) { + : IterRef(Stream), Ctx(&Ctx), Array(&Array), HadError(HadError) { if (IterRef.getLength() == 0) moveToEnd(); else { - auto EC = ExtractorType::extract(IterRef, ThisLen, ThisValue, Context); + auto EC = Ctx.template invoke(IterRef, ThisLen, ThisValue); if (EC) { consumeError(std::move(EC)); markError(); @@ -142,11 +77,13 @@ public: } } - VarStreamArrayIterator(const ArrayType &Array, ContextType *Context, + VarStreamArrayIterator(const ArrayType &Array, const WrappedCtx &Ctx, bool *HadError = nullptr) - : VarStreamArrayIterator(Array, Context, Array.Stream, HadError) {} + : VarStreamArrayIterator(Array, Ctx, Array.Stream, HadError) {} + + VarStreamArrayIterator(const WrappedCtx &Ctx) : Ctx(&Ctx) {} + VarStreamArrayIterator(const VarStreamArrayIterator &Other) = default; - VarStreamArrayIterator() = default; ~VarStreamArrayIterator() = default; bool operator==(const IterType &R) const { @@ -164,12 +101,12 @@ public: return false; } - const ValueType &operator*() const { + const Value &operator*() const { assert(Array && !HasError); return ThisValue; } - ValueType &operator*() { + Value &operator*() { assert(Array && !HasError); return ThisValue; } @@ -185,7 +122,7 @@ public: moveToEnd(); } else { // There is some data after the current record. - auto EC = ExtractorType::extract(IterRef, ThisLen, ThisValue, Context); + auto EC = Ctx->template invoke(IterRef, ThisLen, ThisValue); if (EC) { consumeError(std::move(EC)); markError(); @@ -210,15 +147,134 @@ private: *HadError = true; } - ValueType ThisValue; + Value ThisValue; BinaryStreamRef IterRef; - ContextType *Context{nullptr}; + const WrappedCtx *Ctx{nullptr}; const ArrayType *Array{nullptr}; uint32_t ThisLen{0}; bool HasError{false}; bool *HadError{nullptr}; }; +template struct ContextWrapper { + ContextWrapper() = default; + + explicit ContextWrapper(Context &&Ctx) : Ctx(Ctx) {} + + template + Error invoke(BinaryStreamRef Stream, uint32_t &Len, T &Item) const { + return Extractor::extract(Stream, Len, Item, Ctx); + } + + Context Ctx; +}; + +template struct ContextWrapper { + ContextWrapper() = default; + + template + Error invoke(BinaryStreamRef Stream, uint32_t &Len, T &Item) const { + return Extractor::extract(Stream, Len, Item); + } +}; + +/// VarStreamArray represents an array of variable length records backed by a +/// stream. This could be a contiguous sequence of bytes in memory, it could +/// be a file on disk, or it could be a PDB stream where bytes are stored as +/// discontiguous blocks in a file. Usually it is desirable to treat arrays +/// as contiguous blocks of memory, but doing so with large PDB files, for +/// example, could mean allocating huge amounts of memory just to allow +/// re-ordering of stream data to be contiguous before iterating over it. By +/// abstracting this out, we need not duplicate this memory, and we can +/// iterate over arrays in arbitrarily formatted streams. Elements are parsed +/// lazily on iteration, so there is no upfront cost associated with building +/// or copying a VarStreamArray, no matter how large it may be. +/// +/// You create a VarStreamArray by specifying a ValueType and an Extractor type. +/// If you do not specify an Extractor type, you are expected to specialize +/// VarStreamArrayExtractor for your ValueType. +/// +/// The default extractor type is stateless, but by specializing +/// VarStreamArrayExtractor or defining your own custom extractor type and +/// adding the appropriate ContextType typedef to the class, you can pass a +/// context field during construction of the VarStreamArray that will be +/// passed to each call to extract. +/// +template +class VarStreamArrayBase { + typedef VarStreamArrayBase MyType; + +public: + typedef VarStreamArrayIterator Iterator; + friend Iterator; + + VarStreamArrayBase() = default; + + VarStreamArrayBase(BinaryStreamRef Stream, const WrappedCtx &Ctx) + : Stream(Stream), Ctx(Ctx) {} + + VarStreamArrayBase(const MyType &Other) + : Stream(Other.Stream), Ctx(Other.Ctx) {} + + Iterator begin(bool *HadError = nullptr) const { + if (empty()) + return end(); + + return Iterator(*this, Ctx, Stream, HadError); + } + + Iterator end() const { return Iterator(Ctx); } + + bool empty() const { return Stream.getLength() == 0; } + + /// \brief given an offset into the array's underlying stream, return an + /// iterator to the record at that offset. This is considered unsafe + /// since the behavior is undefined if \p Offset does not refer to the + /// beginning of a valid record. + Iterator at(uint32_t Offset) const { + return Iterator(*this, Ctx, Stream.drop_front(Offset), nullptr); + } + + BinaryStreamRef getUnderlyingStream() const { return Stream; } + +private: + BinaryStreamRef Stream; + WrappedCtx Ctx; +}; + +template +class VarStreamArrayImpl + : public VarStreamArrayBase> { + typedef ContextWrapper WrappedContext; + typedef VarStreamArrayImpl MyType; + typedef VarStreamArrayBase BaseType; + +public: + typedef Context ContextType; + + VarStreamArrayImpl() = default; + VarStreamArrayImpl(BinaryStreamRef Stream, Context &&Ctx) + : BaseType(Stream, WrappedContext(std::forward(Ctx))) {} +}; + +template +class VarStreamArrayImpl + : public VarStreamArrayBase> { + typedef ContextWrapper WrappedContext; + typedef VarStreamArrayImpl MyType; + typedef VarStreamArrayBase BaseType; + +public: + VarStreamArrayImpl() = default; + VarStreamArrayImpl(BinaryStreamRef Stream) + : BaseType(Stream, WrappedContext()) {} +}; + +template > +using VarStreamArray = + VarStreamArrayImpl; + template class FixedStreamArrayIterator; /// FixedStreamArray is similar to VarStreamArray, except with each record diff --git a/include/llvm/Support/BinaryStreamReader.h b/include/llvm/Support/BinaryStreamReader.h index 207c4ab034d..77738077f5f 100644 --- a/include/llvm/Support/BinaryStreamReader.h +++ b/include/llvm/Support/BinaryStreamReader.h @@ -173,13 +173,29 @@ public: /// \returns a success error code if the data was successfully read, otherwise /// returns an appropriate error code. template - Error - readArray(VarStreamArray &Array, uint32_t Size, - typename VarStreamArray::ContextType *Context = nullptr) { + Error readArray(VarStreamArray &Array, uint32_t Size) { BinaryStreamRef S; if (auto EC = readStreamRef(S, Size)) return EC; - Array = VarStreamArray(S, Context); + Array = VarStreamArray(S); + return Error::success(); + } + + /// Read a VarStreamArray of size \p Size bytes and store the result into + /// \p Array. Updates the stream's offset to point after the newly read + /// array. Never causes a copy (although iterating the elements of the + /// VarStreamArray may, depending upon the implementation of the underlying + /// stream). + /// + /// \returns a success error code if the data was successfully read, otherwise + /// returns an appropriate error code. + template + Error readArray(VarStreamArray &Array, uint32_t Size, + ContextType &&Context) { + BinaryStreamRef S; + if (auto EC = readStreamRef(S, Size)) + return EC; + Array = VarStreamArray(S, std::move(Context)); return Error::success(); } diff --git a/lib/DebugInfo/CodeView/ModuleDebugFileChecksumFragment.cpp b/lib/DebugInfo/CodeView/ModuleDebugFileChecksumFragment.cpp index c349e7ecce9..87763248391 100644 --- a/lib/DebugInfo/CodeView/ModuleDebugFileChecksumFragment.cpp +++ b/lib/DebugInfo/CodeView/ModuleDebugFileChecksumFragment.cpp @@ -25,7 +25,7 @@ struct FileChecksumEntryHeader { }; Error llvm::VarStreamArrayExtractor::extract( - BinaryStreamRef Stream, uint32_t &Len, FileChecksumEntry &Item, void *Ctx) { + BinaryStreamRef Stream, uint32_t &Len, FileChecksumEntry &Item) { BinaryStreamReader Reader(Stream); const FileChecksumEntryHeader *Header; diff --git a/lib/DebugInfo/CodeView/ModuleDebugInlineeLinesFragment.cpp b/lib/DebugInfo/CodeView/ModuleDebugInlineeLinesFragment.cpp index 483f7cb5c5a..c54fb2d784a 100644 --- a/lib/DebugInfo/CodeView/ModuleDebugInlineeLinesFragment.cpp +++ b/lib/DebugInfo/CodeView/ModuleDebugInlineeLinesFragment.cpp @@ -17,13 +17,13 @@ using namespace llvm::codeview; Error VarStreamArrayExtractor::extract( BinaryStreamRef Stream, uint32_t &Len, InlineeSourceLine &Item, - ContextType *Fragment) { + bool HasExtraFiles) { BinaryStreamReader Reader(Stream); if (auto EC = Reader.readObject(Item.Header)) return EC; - if (Fragment->hasExtraFiles()) { + if (HasExtraFiles) { uint32_t ExtraFileCount; if (auto EC = Reader.readInteger(ExtraFileCount)) return EC; @@ -42,7 +42,8 @@ Error ModuleDebugInlineeLineFragmentRef::initialize(BinaryStreamReader Reader) { if (auto EC = Reader.readEnum(Signature)) return EC; - if (auto EC = Reader.readArray(Lines, Reader.bytesRemaining(), this)) + if (auto EC = + Reader.readArray(Lines, Reader.bytesRemaining(), hasExtraFiles())) return EC; assert(Reader.bytesRemaining() == 0); diff --git a/lib/DebugInfo/PDB/Native/DbiModuleDescriptorBuilder.cpp b/lib/DebugInfo/PDB/Native/DbiModuleDescriptorBuilder.cpp index f994b4538ef..867864e47dc 100644 --- a/lib/DebugInfo/PDB/Native/DbiModuleDescriptorBuilder.cpp +++ b/lib/DebugInfo/PDB/Native/DbiModuleDescriptorBuilder.cpp @@ -89,6 +89,14 @@ uint32_t DbiModuleDescriptorBuilder::calculateSerializedLength() const { return alignTo(L + M + O, sizeof(uint32_t)); } +template struct Foo { + explicit Foo(T &&Answer) : Answer(Answer) {} + + T Answer; +}; + +template Foo makeFoo(T &&t) { return Foo(std::move(t)); } + void DbiModuleDescriptorBuilder::finalize() { Layout.FileNameOffs = 0; // TODO: Fix this Layout.Flags = 0; // TODO: Fix this diff --git a/unittests/Support/BinaryStreamTest.cpp b/unittests/Support/BinaryStreamTest.cpp index 74c51e382d9..41567dad622 100644 --- a/unittests/Support/BinaryStreamTest.cpp +++ b/unittests/Support/BinaryStreamTest.cpp @@ -358,14 +358,14 @@ TEST_F(BinaryStreamTest, VarStreamArray) { struct StringExtractor { public: - typedef uint32_t ContextType; + typedef uint32_t &ContextType; static Error extract(BinaryStreamRef Stream, uint32_t &Len, StringRef &Item, - uint32_t *Index) { - if (*Index == 0) + uint32_t &Index) { + if (Index == 0) Len = strlen("1. Test"); - else if (*Index == 1) + else if (Index == 1) Len = strlen("2. Longer Test"); - else if (*Index == 2) + else if (Index == 2) Len = strlen("3. Really Long Test"); else Len = strlen("4. Super Extra Longest Test Of All"); @@ -374,14 +374,14 @@ TEST_F(BinaryStreamTest, VarStreamArray) { return EC; Item = StringRef(reinterpret_cast(Bytes.data()), Bytes.size()); - ++(*Index); + ++Index; return Error::success(); } }; for (auto &Stream : Streams) { uint32_t Context = 0; - VarStreamArray Array(*Stream.Input, &Context); + VarStreamArray Array(*Stream.Input, Context); auto Iter = Array.begin(); ASSERT_EQ("1. Test", *Iter++); ASSERT_EQ("2. Longer Test", *Iter++); -- 2.50.1