From e26049f1414cc44d4ba05bf4415f4c5ecf919fcd Mon Sep 17 00:00:00 2001 From: jules Date: Mon, 13 Aug 2018 10:22:23 +0100 Subject: [PATCH] Added assertions to Array to catch situations where a reference to a member is passed into an add() method. Also changed the form of StringArray method params to avoid this problem. --- modules/juce_core/containers/juce_Array.h | 2 +- modules/juce_core/containers/juce_ArrayBase.h | 19 ++++++++++++++ modules/juce_core/text/juce_StringArray.cpp | 25 +++++++++++-------- modules/juce_core/text/juce_StringArray.h | 9 +++---- .../values/juce_ValueWithDefault.h | 1 + 5 files changed, 38 insertions(+), 18 deletions(-) diff --git a/modules/juce_core/containers/juce_Array.h b/modules/juce_core/containers/juce_Array.h index ec76fea6a1..50e45339db 100644 --- a/modules/juce_core/containers/juce_Array.h +++ b/modules/juce_core/containers/juce_Array.h @@ -1083,7 +1083,7 @@ public: private: //============================================================================== - ArrayBase values; + ArrayBase values; void removeInternal (int indexToRemove) { diff --git a/modules/juce_core/containers/juce_ArrayBase.h b/modules/juce_core/containers/juce_ArrayBase.h index 39acf48884..e4604e96b3 100644 --- a/modules/juce_core/containers/juce_ArrayBase.h +++ b/modules/juce_core/containers/juce_ArrayBase.h @@ -178,12 +178,14 @@ public: //============================================================================== void add (const ElementType& newElement) { + checkSourceIsNotAMember (&newElement); ensureAllocatedSize (numUsed + 1); addAssumingCapacityIsReady (newElement); } void add (ElementType&& newElement) { + checkSourceIsNotAMember (&newElement); ensureAllocatedSize (numUsed + 1); addAssumingCapacityIsReady (std::move (newElement)); } @@ -191,6 +193,7 @@ public: template void add (const ElementType& firstNewElement, OtherElements... otherElements) { + checkSourceIsNotAMember (&firstNewElement); ensureAllocatedSize (numUsed + 1 + (int) sizeof... (otherElements)); addAssumingCapacityIsReady (firstNewElement, otherElements...); } @@ -198,6 +201,7 @@ public: template void add (ElementType&& firstNewElement, OtherElements... otherElements) { + checkSourceIsNotAMember (&firstNewElement); ensureAllocatedSize (numUsed + 1 + (int) sizeof... (otherElements)); addAssumingCapacityIsReady (std::move (firstNewElement), otherElements...); } @@ -223,6 +227,7 @@ public: template void addArray (const OtherArrayType& arrayToAddFrom) { + jassert ((const void*) this != (const void*) &arrayToAddFrom); // can't add from our own elements! ensureAllocatedSize (numUsed + (int) arrayToAddFrom.size()); for (auto& e : arrayToAddFrom) @@ -234,6 +239,8 @@ public: addArray (const OtherArrayType& arrayToAddFrom, int startIndex, int numElementsToAdd = -1) { + jassert ((const void*) this != (const void*) &arrayToAddFrom); // can't add from our own elements! + if (startIndex < 0) { jassertfalse; @@ -251,6 +258,7 @@ public: //============================================================================== void insert (int indexToInsertAt, ParameterType newElement, int numberOfTimesToInsertIt) { + checkSourceIsNotAMember (&newElement); auto* space = createInsertSpace (indexToInsertAt, numberOfTimesToInsertIt); for (int i = 0; i < numberOfTimesToInsertIt; ++i) @@ -502,6 +510,17 @@ private: new (destination) ElementType (std::move (source)); } + void checkSourceIsNotAMember (const ElementType* element) + { + // when you pass a reference to an existing element into a method like add() which + // may need to reallocate the array to make more space, the incoming reference may + // be deleted indirectly during the reallocation operation! To work around this, + // make a local copy of the item you're trying to add (and maybe use std::move to + // move it into the add() method to avoid any extra overhead) + jassert (element < begin() || element >= end()); + ignoreUnused (element); + } + //============================================================================== HeapBlock elements; int numAllocated = 0, numUsed = 0; diff --git a/modules/juce_core/text/juce_StringArray.cpp b/modules/juce_core/text/juce_StringArray.cpp index b2e4fa1510..2ccc159134 100644 --- a/modules/juce_core/text/juce_StringArray.cpp +++ b/modules/juce_core/text/juce_StringArray.cpp @@ -132,19 +132,18 @@ String& StringArray::getReference (int index) noexcept return strings.getReference (index); } -void StringArray::add (const String& newString) +void StringArray::add (String newString) { - strings.add (newString); + // NB: the local temp copy is to avoid a dangling pointer if the + // argument being passed-in is a reference into this array. + strings.add (std::move (newString)); } -void StringArray::add (String&& stringToAdd) +void StringArray::insert (int index, String newString) { - strings.add (static_cast (stringToAdd)); -} - -void StringArray::insert (int index, const String& newString) -{ - strings.insert (index, newString); + // NB: the local temp copy is to avoid a dangling pointer if the + // argument being passed-in is a reference into this array. + strings.insert (index, std::move (newString)); } bool StringArray::addIfNotAlreadyThere (const String& newString, bool ignoreCase) @@ -158,6 +157,8 @@ bool StringArray::addIfNotAlreadyThere (const String& newString, bool ignoreCase void StringArray::addArray (const StringArray& otherArray, int startIndex, int numElementsToAdd) { + jassert (this != &otherArray); // can't add from our own elements! + if (startIndex < 0) { jassertfalse; @@ -173,13 +174,15 @@ void StringArray::addArray (const StringArray& otherArray, int startIndex, int n void StringArray::mergeArray (const StringArray& otherArray, bool ignoreCase) { + jassert (this != &otherArray); // can't add from our own elements! + for (auto& s : otherArray) addIfNotAlreadyThere (s, ignoreCase); } -void StringArray::set (int index, const String& newString) +void StringArray::set (int index, String newString) { - strings.set (index, newString); + strings.set (index, std::move (newString)); } bool StringArray::contains (StringRef stringToLookFor, bool ignoreCase) const diff --git a/modules/juce_core/text/juce_StringArray.h b/modules/juce_core/text/juce_StringArray.h index 904182910d..4f0885c398 100644 --- a/modules/juce_core/text/juce_StringArray.h +++ b/modules/juce_core/text/juce_StringArray.h @@ -187,10 +187,7 @@ public: //============================================================================== /** Appends a string at the end of the array. */ - void add (const String& stringToAdd); - - /** Appends a string at the end of the array. */ - void add (String&& stringToAdd); + void add (String stringToAdd); /** Inserts a string into the array. @@ -199,7 +196,7 @@ public: If the index is less than zero or greater than the size of the array, the new string will be added to the end of the array. */ - void insert (int index, const String& stringToAdd); + void insert (int index, String stringToAdd); /** Adds a string to the array as long as it's not already in there. The search can optionally be case-insensitive. @@ -213,7 +210,7 @@ public: If the index is higher than the array's size, the new string will be added to the end of the array; if it's less than zero nothing happens. */ - void set (int index, const String& newString); + void set (int index, String newString); /** Appends some strings from another array to the end of this one. diff --git a/modules/juce_data_structures/values/juce_ValueWithDefault.h b/modules/juce_data_structures/values/juce_ValueWithDefault.h index 211473ee78..d30dfc039e 100644 --- a/modules/juce_data_structures/values/juce_ValueWithDefault.h +++ b/modules/juce_data_structures/values/juce_ValueWithDefault.h @@ -209,6 +209,7 @@ private: jassert (delimiter.isNotEmpty()); StringArray elements; + for (auto& v : input) elements.add (v.toString());