[CID 16369] MCCard::relayer(): Restructure code to prevent possible memory leak

peter-b · peter-b · commit 5ad78abecd2d · 2017-01-05T22:08:10.000Z
Coverity detected a possible codepath by which `MCCard::relayer()`
could leak a `MCObjptr` instance.  This patch restructures part of the
relayering code to first calculate an insertion position, then
allocate the `MCObjptr`, before inserting it.  It ensures that there
is no possible code path that permits a new instance to be allocated
without moving its ownership to the `MCCard` instance.

Coverity-ID: 16369
diff --git a/engine/src/card.cpp b/engine/src/card.cpp
@@ -1742,54 +1742,73 @@ Exec_stat MCCard::relayer(MCControl *optr, uint2 newlayer)
 	}
 	else
 	{
-		MCObjptr *newptr = new (nothrow) MCObjptr;
-		newptr->setparent(this);
-		newptr->setref(optr);
-		optr->setparent(this);
-		getstack()->appendcontrol(optr);
-		if (foundobj == NULL)
-			if (newlayer <= 1)
-				newptr->insertto(objptrs);
-			else
-				newptr->appendto(objptrs);
-		else
-		{
-			Boolean g = False;
-			if (!MCrelayergrouped)
-			{
-				while (foundobj->getparent() != this)
-				{
-					foundobj = (MCControl *)foundobj->getparent();
-					g = True;
-				}
-			}
-			tptr = objptrs;
-			do
-			{
-				if (tptr->getref() == foundobj)
-				{
-					if (g)
-					{
-						uint2 tnum;
-						count(CT_LAYER, CT_UNDEFINED, tptr->next()->getref(), tnum, True);
-						if (tptr->next() == objptrs || newlayer < tnum)
-						{
-							if (tptr == objptrs)
-								newptr->insertto(objptrs);
-							else
-								tptr->prev()->append(newptr);
-							break;
-						}
-					}
-					tptr->append(newptr);
-					break;
-				}
-				tptr = tptr->next();
-			}
-			while (tptr != objptrs);
-		}
+        /* Find place in the object pointer list at which the
+         * relayered object should be placed.  If t_before, the object
+         * should be placed immediately before the iterator. */
+        MCObjptr *t_insert_iter = objptrs;
+        bool t_before = false;
+        if (foundobj == nullptr)
+        {
+            t_before = (newlayer <= 1);
+        }
+        else
+        {
+            /* If there's a target object for relayering, and group
+             * relayering is banned, then we need to find the outer
+             * group of the target object. */
+            bool t_found_in_group = false;
+            while (!MCrelayergrouped &&
+                   foundobj->getparent() != this)
+            {
+                foundobj = MCObjectCast<MCControl>(foundobj->getparent());
+                t_found_in_group = true;
+            }
+
+            do
+            {
+                if (foundobj == t_insert_iter->getref())
+                {
+                    if (t_found_in_group)
+                    {
+                        uint2 t_minimum_layer = 0;
+                        count(CT_LAYER, CT_UNDEFINED, t_insert_iter->next()->getref(),
+                              t_minimum_layer, True);
+                        t_before = (t_insert_iter->next() == objptrs ||
+                                    newlayer < t_minimum_layer);
+                    }
+                    break;
+                }
+            }
+            while (t_insert_iter != objptrs);
+        }
+
+        /* Create and insert an object pointer */
+        MCObjptr *newptr = new (nothrow) MCObjptr();
+        if (newptr == nullptr)
+            return ES_ERROR;
+        newptr->setparent(this);
+        newptr->setref(optr);
+        optr->setparent(this);
+        getstack()->appendcontrol(optr);
+
+        if (t_insert_iter == objptrs)
+        {
+            if (t_before)
+                newptr->insertto(objptrs);
+            else
+                newptr->appendto(objptrs);
+        }
+        else
+        {
+            if (t_before)
+                t_insert_iter->prev()->append(newptr);
+            else
+                t_insert_iter->append(newptr);
+        }
 
-		layer_added(optr, newptr -> prev() != objptrs -> prev() ? newptr -> prev() : nil, newptr -> next() != objptrs ? newptr -> next() : nil);
+        layer_added(optr,
+                    (newptr->prev() != objptrs->prev()) ? newptr->prev() : nil,
+                    (newptr->next() != objptrs)         ? newptr->next() : nil);
 	}
 
 	if (oldparent == this)
