From cc234ff4304d3a60dc2163f77a4214ecdbd88d5c Mon Sep 17 00:00:00 2001 From: wolfbeast Date: Fri, 2 Nov 2018 14:59:25 +0100 Subject: Add overflow checks for extending nsTArrays. Surprisingly, this was previously not done. Also, some of this code seems to be incorrect or, at the very least, wasn't clear what it was trying to do. --- xpcom/glue/nsTArray-inl.h | 32 ++++++++++++++++++++++---------- xpcom/glue/nsTArray.h | 42 ++++++++++++++++++++++++++++-------------- 2 files changed, 50 insertions(+), 24 deletions(-) diff --git a/xpcom/glue/nsTArray-inl.h b/xpcom/glue/nsTArray-inl.h index af57c9866..7e667a327 100644 --- a/xpcom/glue/nsTArray-inl.h +++ b/xpcom/glue/nsTArray-inl.h @@ -108,6 +108,23 @@ nsTArray_base::UsesAutoArrayBuffer() const bool IsTwiceTheRequiredBytesRepresentableAsUint32(size_t aCapacity, size_t aElemSize); +template +template +typename ActualAlloc::ResultTypeProxy +nsTArray_base::ExtendCapacity(size_type aLength, + size_type aCount, + size_type aElemSize) +{ + mozilla::CheckedInt newLength = aLength; + newLength += aCount; + + if (!newLength.isValid()) { + return ActualAlloc::FailureResult(); + } + + return this->EnsureCapacity(newLength.value(), aElemSize); +} + template template typename ActualAlloc::ResultTypeProxy @@ -275,26 +292,21 @@ nsTArray_base::ShiftData(index_type aStart, template template -bool +typename ActualAlloc::ResultTypeProxy nsTArray_base::InsertSlotsAt(index_type aIndex, size_type aCount, size_type aElemSize, size_t aElemAlign) { MOZ_ASSERT(aIndex <= Length(), "Bogus insertion index"); - size_type newLen = Length() + aCount; - - EnsureCapacity(newLen, aElemSize); - - // Check for out of memory conditions - if (Capacity() < newLen) { - return false; + if (!ActualAlloc::Successful(this->ExtendCapacity(Length(), aCount, aElemSize))) { + return ActualAlloc::FailureResult(); } - + // Move the existing elements as needed. Note that this will // change our mLength, so no need to call IncrementLength. ShiftData(aIndex, 0, aCount, aElemSize, aElemAlign); - return true; + return ActualAlloc::SuccessResult(); } // nsTArray_base::IsAutoArrayRestorer is an RAII class which takes diff --git a/xpcom/glue/nsTArray.h b/xpcom/glue/nsTArray.h index c86772a8e..82586a79a 100644 --- a/xpcom/glue/nsTArray.h +++ b/xpcom/glue/nsTArray.h @@ -12,6 +12,7 @@ #include "mozilla/Assertions.h" #include "mozilla/Attributes.h" #include "mozilla/BinarySearch.h" +#include "mozilla/CheckedInt.h" #include "mozilla/fallible.h" #include "mozilla/Function.h" #include "mozilla/MathAlgorithms.h" @@ -421,6 +422,17 @@ protected: typename ActualAlloc::ResultTypeProxy EnsureCapacity(size_type aCapacity, size_type aElemSize); + // Extend the storage to accommodate aCount extra elements. + // @param aLength The current size of the array. + // @param aCount The number of elements to add. + // @param aElemSize The size of an array element. + // @return False if insufficient memory is available or the new length + // would overflow; true otherwise. + template + typename ActualAlloc::ResultTypeProxy ExtendCapacity(size_type aLength, + size_type aCount, + size_type aElemSize); + // Tries to resize the storage to the minimum required amount. If this fails, // the array is left as-is. // @param aElemSize The size of an array element. @@ -462,8 +474,9 @@ protected: // @param aElementSize the size of an array element. // @param aElemAlign the alignment in bytes of an array element. template - bool InsertSlotsAt(index_type aIndex, size_type aCount, - size_type aElementSize, size_t aElemAlign); + typename ActualAlloc::ResultTypeProxy + InsertSlotsAt(index_type aIndex, size_type aCount, + size_type aElementSize, size_t aElemAlign); template typename ActualAlloc::ResultTypeProxy @@ -1655,8 +1668,8 @@ public: protected: template elem_type* AppendElements(size_type aCount) { - if (!ActualAlloc::Successful(this->template EnsureCapacity( - Length() + aCount, sizeof(elem_type)))) { + if (!ActualAlloc::Successful(this->template ExtendCapacity( + Length(), aCount, sizeof(elem_type)))) { return nullptr; } elem_type* elems = Elements() + Length(); @@ -1872,9 +1885,8 @@ protected: template elem_type* InsertElementsAt(index_type aIndex, size_type aCount) { - if (!base_type::template InsertSlotsAt(aIndex, aCount, - sizeof(elem_type), - MOZ_ALIGNOF(elem_type))) { + if (!ActualAlloc::Successful(this->template InsertSlotsAt( + aIndex, aCount, sizeof(elem_type), MOZ_ALIGNOF(elem_type)))) { return nullptr; } @@ -2047,9 +2059,8 @@ auto nsTArray_Impl::InsertElementsAt(index_type aIndex, size_type aCount, const Item& aItem) -> elem_type* { - if (!base_type::template InsertSlotsAt(aIndex, aCount, - sizeof(elem_type), - MOZ_ALIGNOF(elem_type))) { + if (!ActualAlloc::Successful(this->template InsertSlotsAt( + aIndex, aCount, sizeof(elem_type), MOZ_ALIGNOF(elem_type)))) { return nullptr; } @@ -2068,6 +2079,7 @@ template auto nsTArray_Impl::InsertElementAt(index_type aIndex) -> elem_type* { + // Length() + 1 is guaranteed to not overflow, so EnsureCapacity is OK. if (!ActualAlloc::Successful(this->template EnsureCapacity( Length() + 1, sizeof(elem_type)))) { return nullptr; @@ -2084,6 +2096,7 @@ template auto nsTArray_Impl::InsertElementAt(index_type aIndex, Item&& aItem) -> elem_type* { + // Length() + 1 is guaranteed to not overflow, so EnsureCapacity is OK. if (!ActualAlloc::Successful(this->template EnsureCapacity( Length() + 1, sizeof(elem_type)))) { return nullptr; @@ -2100,8 +2113,8 @@ template auto nsTArray_Impl::AppendElements(const Item* aArray, size_type aArrayLen) -> elem_type* { - if (!ActualAlloc::Successful(this->template EnsureCapacity( - Length() + aArrayLen, sizeof(elem_type)))) { + if (!ActualAlloc::Successful(this->template ExtendCapacity( + Length(), aArrayLen, sizeof(elem_type)))) { return nullptr; } index_type len = Length(); @@ -2123,8 +2136,8 @@ nsTArray_Impl::AppendElements(nsTArray_Impl&& aArray) index_type len = Length(); index_type otherLen = aArray.Length(); - if (!Alloc::Successful(this->template EnsureCapacity( - len + otherLen, sizeof(elem_type)))) { + if (!Alloc::Successful(this->template ExtendCapacity( + len, otherLen, sizeof(elem_type)))) { return nullptr; } copy_type::MoveNonOverlappingRegion(Elements() + len, aArray.Elements(), otherLen, @@ -2140,6 +2153,7 @@ template auto nsTArray_Impl::AppendElement(Item&& aItem) -> elem_type* { + // Length() + 1 is guaranteed to not overflow, so EnsureCapacity is OK. if (!ActualAlloc::Successful(this->template EnsureCapacity( Length() + 1, sizeof(elem_type)))) { return nullptr; -- cgit v1.2.3