From da7d6a4db8991593af33045f7ba885c4bac9c2f0 Mon Sep 17 00:00:00 2001 From: Michael Layzell Date: Wed, 31 Aug 2016 18:42:12 -0400 Subject: [PATCH] Bug 1299489 - Change nsTArray to use a custom iterator based on index instead of pointers to improve iterator invalidation safety of ranged for loops, r=froydnj MozReview-Commit-ID: CahPOcRYvES --- dom/animation/KeyframeUtils.cpp | 4 +- layout/generic/nsTextFrame.cpp | 2 +- layout/style/RuleProcessorCache.cpp | 4 +- .../components/url-classifier/ChunkSet.cpp | 46 +++--- xpcom/glue/nsTArray.h | 135 ++++++++++++++++-- 5 files changed, 154 insertions(+), 37 deletions(-) diff --git a/dom/animation/KeyframeUtils.cpp b/dom/animation/KeyframeUtils.cpp index fd8a05e88f8b..48b879c9ea21 100644 --- a/dom/animation/KeyframeUtils.cpp +++ b/dom/animation/KeyframeUtils.cpp @@ -512,8 +512,8 @@ KeyframeUtils::ApplySpacing(nsTArray& aKeyframes, } // Fill in remaining missing offsets. - const Keyframe* const last = aKeyframes.cend() - 1; - const RangedPtr begin(aKeyframes.begin(), aKeyframes.Length()); + const Keyframe* const last = &aKeyframes.LastElement(); + const RangedPtr begin(aKeyframes.Elements(), aKeyframes.Length()); RangedPtr keyframeA = begin; while (keyframeA != last) { // Find keyframe A and keyframe B *between* which we will apply spacing. diff --git a/layout/generic/nsTextFrame.cpp b/layout/generic/nsTextFrame.cpp index 980a6089fefa..9ccb13121b11 100644 --- a/layout/generic/nsTextFrame.cpp +++ b/layout/generic/nsTextFrame.cpp @@ -5001,7 +5001,7 @@ nsDisplayText::nsDisplayText(nsDisplayListBuilder* aBuilder, nsTextFrame* aFrame mGlyphs.Clear(); } else { g->glyphs().SetLength(glyphs.size()); - PodCopy(g->glyphs().begin(), glyphs.data(), glyphs.size()); + PodCopy(g->glyphs().Elements(), glyphs.data(), glyphs.size()); g->color() = color; } } diff --git a/layout/style/RuleProcessorCache.cpp b/layout/style/RuleProcessorCache.cpp index 2bf4255a482b..55af7625509d 100644 --- a/layout/style/RuleProcessorCache.cpp +++ b/layout/style/RuleProcessorCache.cpp @@ -143,8 +143,8 @@ RuleProcessorCache::StopTracking(nsCSSRuleProcessor* aRuleProcessor) void RuleProcessorCache::DoRemoveSheet(CSSStyleSheet* aSheet) { - Entry* last = std::remove_if(mEntries.begin(), mEntries.end(), - HasSheet_ThenRemoveRuleProcessors(this, aSheet)); + auto last = std::remove_if(mEntries.begin(), mEntries.end(), + HasSheet_ThenRemoveRuleProcessors(this, aSheet)); mEntries.TruncateLength(last - mEntries.begin()); } diff --git a/toolkit/components/url-classifier/ChunkSet.cpp b/toolkit/components/url-classifier/ChunkSet.cpp index 162a74260c51..075d7a9c2b4e 100644 --- a/toolkit/components/url-classifier/ChunkSet.cpp +++ b/toolkit/components/url-classifier/ChunkSet.cpp @@ -14,15 +14,15 @@ nsresult ChunkSet::Serialize(nsACString& aChunkStr) { aChunkStr.Truncate(); - for (const Range* range = mRanges.begin(); range != mRanges.end(); range++) { - if (range != mRanges.begin()) { + for (const Range& range : mRanges) { + if (&range != &mRanges[0]) { aChunkStr.Append(','); } - aChunkStr.AppendInt((int32_t)range->Begin()); - if (range->Begin() != range->End()) { + aChunkStr.AppendInt((int32_t)range.Begin()); + if (range.Begin() != range.End()) { aChunkStr.Append('-'); - aChunkStr.AppendInt((int32_t)range->End()); + aChunkStr.AppendInt((int32_t)range.End()); } } @@ -75,10 +75,9 @@ ChunkSet::Merge(const ChunkSet& aOther) { size_t oldLen = mRanges.Length(); - for (const Range* mergeRange = aOther.mRanges.begin(); - mergeRange != aOther.mRanges.end(); mergeRange++) { - if (!HasSubrange(*mergeRange)) { - if (!mRanges.InsertElementSorted(*mergeRange, fallible)) { + for (const Range& mergeRange : aOther.mRanges) { + if (!HasSubrange(mergeRange)) { + if (!mRanges.InsertElementSorted(mergeRange, fallible)) { return NS_ERROR_OUT_OF_MEMORY; } } @@ -103,8 +102,8 @@ uint32_t ChunkSet::Length() const { uint32_t len = 0; - for (const Range* range = mRanges.begin(); range != mRanges.end(); range++) { - len += range->Length(); + for (const Range& range : mRanges) { + len += range.Length(); } return len; @@ -113,24 +112,23 @@ ChunkSet::Length() const nsresult ChunkSet::Remove(const ChunkSet& aOther) { - for (const Range* removalRange = aOther.mRanges.begin(); - removalRange != aOther.mRanges.end(); removalRange++) { + for (const Range& removalRange : aOther.mRanges) { if (mRanges.Length() == 0) { return NS_OK; } - if (mRanges.LastElement().End() < removalRange->Begin() || + if (mRanges.LastElement().End() < removalRange.Begin() || aOther.mRanges.LastElement().End() < mRanges[0].Begin()) { return NS_OK; } size_t intersectionIdx; while (BinarySearchIf(mRanges, 0, mRanges.Length(), - Range::IntersectionComparator(*removalRange), &intersectionIdx)) { + Range::IntersectionComparator(removalRange), &intersectionIdx)) { ChunkSet remains; - nsresult rv = mRanges[intersectionIdx].Remove(*removalRange, remains); + nsresult rv = mRanges[intersectionIdx].Remove(removalRange, remains); if (NS_FAILED(rv)) { return rv; @@ -158,8 +156,8 @@ ChunkSet::Write(nsIOutputStream* aOut) { nsTArray chunks(IO_BUFFER_SIZE); - for (const Range* range = mRanges.begin(); range != mRanges.end(); range++) { - for (uint32_t chunk = range->Begin(); chunk <= range->End(); chunk++) { + for (const Range& range : mRanges) { + for (uint32_t chunk = range.Begin(); chunk <= range.End(); chunk++) { chunks.AppendElement(chunk); if (chunks.Length() == chunks.Capacity()) { @@ -201,8 +199,8 @@ ChunkSet::Read(nsIInputStream* aIn, uint32_t aNumElements) aNumElements -= numToRead; - for (const uint32_t* c = chunks.begin(); c != chunks.end(); c++) { - rv = Set(*c); + for (uint32_t c : chunks) { + rv = Set(c); if (NS_FAILED(rv)) { return rv; @@ -216,11 +214,11 @@ ChunkSet::Read(nsIInputStream* aIn, uint32_t aNumElements) bool ChunkSet::HasSubrange(const Range& aSubrange) const { - for (const Range* range = mRanges.begin(); range != mRanges.end(); range++) { - if (range->Contains(aSubrange)) { + for (const Range& range : mRanges) { + if (range.Contains(aSubrange)) { return true; - } else if (!(aSubrange.Begin() > range->End() || - range->Begin() > aSubrange.End())) { + } else if (!(aSubrange.Begin() > range.End() || + range.Begin() > aSubrange.End())) { // In this case, aSubrange overlaps this range but is not a subrange. // because the ChunkSet implementation ensures that there are no // overlapping ranges, this means that aSubrange cannot be a subrange of diff --git a/xpcom/glue/nsTArray.h b/xpcom/glue/nsTArray.h index 859eadcf7f8f..d627e024c0e0 100644 --- a/xpcom/glue/nsTArray.h +++ b/xpcom/glue/nsTArray.h @@ -31,6 +31,7 @@ #include #include #include +#include namespace JS { template @@ -355,6 +356,124 @@ struct nsTArray_SafeElementAtHelper, Derived> } }; +// We have implemented a custom iterator class for nsTArray rather than using +// raw pointers into the backing storage to improve the safety of C++11-style +// range based iteration in the presence of array mutation, or script execution +// (bug 1299489). +// +// Mutating an array which is being iterated is still wrong, and will either +// cause elements to be missed or firefox to crash, but will not trigger memory +// safety problems due to the release-mode bounds checking found in ElementAt. +// +// This iterator implements the full standard random access iterator spec, and +// can be treated in may ways as though it is a pointer. +template +class nsTArrayIterator +{ +public: + typedef nsTArray::Type> array_type; + typedef nsTArrayIterator iterator_type; + typedef typename array_type::index_type index_type; + typedef Element value_type; + typedef ptrdiff_t difference_type; + typedef value_type* pointer; + typedef value_type& reference; + typedef std::random_access_iterator_tag iterator_category; + +private: + const array_type* mArray; + index_type mIndex; + +public: + nsTArrayIterator() : mArray(nullptr), mIndex(0) {} + nsTArrayIterator(const iterator_type& aOther) + : mArray(aOther.mArray), mIndex(aOther.mIndex) {} + nsTArrayIterator(const array_type& aArray, index_type aIndex) + : mArray(&aArray), mIndex(aIndex) {} + + iterator_type& operator=(const iterator_type& aOther) { + mArray = aOther.mArray; + mIndex = aOther.mIndex; + return *this; + } + + bool operator==(const iterator_type& aRhs) const { + return mIndex == aRhs.mIndex; + } + bool operator!=(const iterator_type& aRhs) const { + return !(*this == aRhs); + } + bool operator<(const iterator_type& aRhs) const { + return mIndex < aRhs.mIndex; + } + bool operator>(const iterator_type& aRhs) const { + return mIndex > aRhs.mIndex; + } + bool operator<=(const iterator_type& aRhs) const { + return mIndex <= aRhs.mIndex; + } + bool operator>=(const iterator_type& aRhs) const { + return mIndex >= aRhs.mIndex; + } + + // These operators depend on the release mode bounds checks in + // nsTArray::ElementAt for safety. + value_type* operator->() const { + return const_cast(&(*mArray)[mIndex]); + } + value_type& operator*() const { + return const_cast((*mArray)[mIndex]); + } + + iterator_type& operator++() { + ++mIndex; + return *this; + } + iterator_type operator++(int) { + iterator_type it = *this; + ++*this; + return it; + } + iterator_type& operator--() { + --mIndex; + return *this; + } + iterator_type operator--(int) { + iterator_type it = *this; + --*this; + return it; + } + + iterator_type& operator+=(difference_type aDiff) { + mIndex += aDiff; + return *this; + } + iterator_type& operator-=(difference_type aDiff) { + mIndex -= aDiff; + return *this; + } + + iterator_type operator+(difference_type aDiff) const { + iterator_type it = *this; + it += aDiff; + return it; + } + iterator_type operator-(difference_type aDiff) const { + iterator_type it = *this; + it -= aDiff; + return it; + } + + difference_type operator-(const iterator_type& aOther) const { + return static_cast(mIndex) - + static_cast(aOther.mIndex); + } + + value_type& operator[](difference_type aIndex) const { + return *this->operator+(aIndex); + } +}; + // Servo bindings. extern "C" void Gecko_EnsureTArrayCapacity(void* aArray, size_t aCapacity, @@ -857,10 +976,10 @@ public: typedef nsTArray_Impl self_type; typedef nsTArrayElementTraits elem_traits; typedef nsTArray_SafeElementAtHelper safeelementat_helper_type; - typedef elem_type* iterator; - typedef const elem_type* const_iterator; - typedef mozilla::ReverseIterator reverse_iterator; - typedef mozilla::ReverseIterator const_reverse_iterator; + typedef nsTArrayIterator iterator; + typedef nsTArrayIterator const_iterator; + typedef mozilla::ReverseIterator reverse_iterator; + typedef mozilla::ReverseIterator const_reverse_iterator; using safeelementat_helper_type::SafeElementAt; using base_type::EmptyHdr; @@ -1098,11 +1217,11 @@ public: } // Methods for range-based for loops. - iterator begin() { return Elements(); } - const_iterator begin() const { return Elements(); } + iterator begin() { return iterator(*this, 0); } + const_iterator begin() const { return const_iterator(*this, 0); } const_iterator cbegin() const { return begin(); } - iterator end() { return Elements() + Length(); } - const_iterator end() const { return Elements() + Length(); } + iterator end() { return iterator(*this, Length()); } + const_iterator end() const { return const_iterator(*this, Length()); } const_iterator cend() const { return end(); } // Methods for reverse iterating.