[[ Bug 21992 ]] Fix memory leaks when sorting using MCSortnodes

runrevmark · runrevmark · commit b984d53b540d · 2019-04-18T16:25:52.000+01:00
This patch fixes memory leaks which can occur when using engine features
which perform sorting using MCSortnodes. Specifically, when sorting
fields, sorting cards of a stack, sorting the selection list for grouping
and sorting the selection list for cloning.

The leaks occur because of the presumption that MCSortnodes are POD types
and thus require no specific construction or destruction. However,
MCSortnodes fail to be POD as they have a destructor which releases any
MCValueRef sort key which they hold.

To fix the issue a new auto-class MCAutoArrayZeroedNonPod has been
added to foundation-auto. This is direct replication of MCAutoArray
but tailored to the case where the element type has a destructor and
that destructor does nothing if the element is zeroed. This auto-class
has then been used to replace previous uses of MCAutoArray in MCStack::sort,
MCField::sort and MCSellst::sort.

Additionally, the no longer used MCStringsExecSortOld function has been
removed.
diff --git a/docs/notes/bugfix-21992.md b/docs/notes/bugfix-21992.md
@@ -0,0 +1 @@
+# Fix memory leaks when sorting cards, fields, or object selection
diff --git a/engine/src/exec-strings.cpp b/engine/src/exec-strings.cpp
@@ -2252,41 +2252,6 @@ void MCStringsSortAddItem(MCExecContext &ctxt, MCSortnode *items, uint4 &nitems,
 	nitems++;
 }
 
-void MCStringsExecSortOld(MCExecContext& ctxt, Sort_type p_dir, Sort_type p_form, MCStringRef *p_strings_array, uindex_t p_count, MCExpression *p_by, MCStringRef*& r_sorted_array, uindex_t& r_sorted_count)
-{
-	// OK-2008-12-11: [[Bug 7503]] - If there are 0 items in the string, don't carry out the search,
-	// this keeps the behavior consistent with previous versions of Revolution.
-	if (p_count < 1)
-	{
-        r_sorted_count = 0;
-        return;
-	}
-    
-	// Now we know the item count, we can allocate an array of MCSortnodes to store them.
-	MCAutoArray<MCSortnode> t_items;
-	t_items.Extend(p_count + 1);
-    uindex_t t_added = 0;
-    
-	// Next, populate the MCSortnodes with all the items to be sorted
-    for (uindex_t i = 0; i < p_count; i++)
-    {
-        MCStringsSortAddItem(ctxt, t_items . Ptr(), t_added, p_form, p_strings_array[i], p_by);
-        t_items[t_added - 1] . data = (void *)p_strings_array[i];
-    }
-
-    MCStringsSort(t_items . Ptr(), t_added, p_dir, p_form, ctxt . GetStringComparisonType());
-
-    MCAutoArray<MCStringRef> t_sorted;
-    
- 	for (uindex_t i = 0; i < t_added; i++)
-    {
-        t_sorted . Push((MCStringRef)t_items[i] . data);
-        MCValueRelease(t_items[i] . svalue);
-    }
-    
-    t_sorted . Take(r_sorted_array, r_sorted_count);
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 
 typedef bool (*comparator_t)(void *context, uindex_t left, uindex_t right);
diff --git a/engine/src/fields.cpp b/engine/src/fields.cpp
@@ -85,7 +85,7 @@ Exec_stat MCField::sort(MCExecContext &ctxt, uint4 parid, Chunk_term type,
         return ES_NORMAL;
 	}
 
-    MCAutoArray<MCSortnode> items;
+    MCAutoArrayZeroedNonPod<MCSortnode> items;
 	uint4 nitems = 0;
 	MCParagraph *pgptr;
     
diff --git a/engine/src/sellst.cpp b/engine/src/sellst.cpp
@@ -238,7 +238,7 @@ void MCSellist::sort()
     Clean();
     
     MCSelnode *optr = objects;
-	MCAutoArray<MCSortnode> items;
+	MCAutoArrayZeroedNonPod<MCSortnode> items;
 	uint4 nitems = 0;
 	MCCard *cptr = optr->m_ref->getcard();
 	do
diff --git a/engine/src/stack3.cpp b/engine/src/stack3.cpp
@@ -1903,7 +1903,7 @@ bool MCStack::sort(MCExecContext &ctxt, Sort_type dir, Sort_type form,
 	MCStack *olddefault = MCdefaultstackptr;
 	MCdefaultstackptr = this;
 	MCCard *cptr = curcard;
-	MCAutoArray<MCSortnode> items;
+	MCAutoArrayZeroedNonPod<MCSortnode> items;
 	uint4 nitems = 0;
 	MCerrorlock++;
     
diff --git a/libfoundation/include/foundation-auto.h b/libfoundation/include/foundation-auto.h
@@ -1137,6 +1137,118 @@ template<typename T> class MCAutoArray
 	uindex_t m_size = 0;
 };
 
+/* Use the MCAutoArrayZeroedNonPod class when T has a destructor which will
+ * only delete non-null members. */
+template<typename T> class MCAutoArrayZeroedNonPod
+{
+public:
+    constexpr MCAutoArrayZeroedNonPod() = default;
+    
+    ~MCAutoArrayZeroedNonPod(void)
+    {
+        for(uindex_t i = 0; i < m_size; i++)
+            m_ptr[i].~T();
+        MCMemoryDeleteArray(m_ptr);
+    }
+    
+    //////////
+    
+    T* Ptr()
+    {
+        return m_ptr;
+    }
+    
+    uindex_t Size() const
+    {
+        return m_size;
+    }
+    
+    //////////
+    
+    bool New(uindex_t p_size)
+    {
+        MCAssert(m_ptr == nil);
+        return MCMemoryNewArray(p_size, m_ptr, m_size);
+    }
+    
+    void Delete(void)
+    {
+        MCMemoryDeleteArray(m_ptr);
+        m_ptr = nil;
+        m_size = 0;
+    }
+    
+    //////////
+    
+    bool Resize(uindex_t p_new_size)
+    {
+        return MCMemoryResizeArray(p_new_size, m_ptr, m_size);
+    }
+    
+    bool Extend(uindex_t p_new_size)
+    {
+        MCAssert(p_new_size >= m_size);
+        return Resize(p_new_size);
+    }
+    
+    void Shrink(uindex_t p_new_size)
+    {
+        MCAssert(p_new_size <= m_size);
+        Resize(p_new_size);
+    }
+    
+    //////////
+    
+    bool Push(T p_value)
+    {
+        if (!Extend(m_size + 1))
+            return false;
+        m_ptr[m_size - 1] = p_value;
+        return true;
+    }
+    
+    //////////
+    
+    T*& PtrRef()
+    {
+        MCAssert(m_ptr == nil);
+        return m_ptr;
+    }
+    
+    uindex_t& SizeRef()
+    {
+        MCAssert(m_size == 0);
+        return m_size;
+    }
+    
+    //////////
+    
+    void Take(T*& r_array, uindex_t& r_count)
+    {
+        r_array = m_ptr;
+        r_count = m_size;
+        
+        m_ptr = nil;
+        m_size = 0;
+    }
+    
+    //////////
+    
+    T& operator [] (uindex_t p_index)
+    {
+        return m_ptr[p_index];
+    }
+    
+    const T& operator [] (uindex_t p_index) const
+    {
+        return m_ptr[p_index];
+    }
+    
+private:
+    T *m_ptr = nullptr;
+    uindex_t m_size = 0;
+};
+
 // Version of MCAutoArray that applies the provided deallocator to each element of the array when freed
 template <typename T, void (*FREE)(T)> class MCAutoCustomPointerArray
 {

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+# Fix memory leaks when sorting cards, fields, or object selection`
Original file line number	Diff line number	Diff line change
`@@ -85,7 +85,7 @@ Exec_stat MCField::sort(MCExecContext &ctxt, uint4 parid, Chunk_term type,`
`85`	`85`	`return ES_NORMAL;`
`86`	`86`	`}`
`87`	`87`
`88`		`- MCAutoArray<MCSortnode> items;`
	`88`	`+ MCAutoArrayZeroedNonPod<MCSortnode> items;`
`89`	`89`	`uint4 nitems = 0;`
`90`	`90`	`MCParagraph *pgptr;`
`91`	`91`