Bug in ArrayObj.mqh

New comment
 

To Metaquotes developers:

This is a deep bug that has been propagated in all versions of both MT4 and MT5 include file packages and would cause problems for all users of any of the object classes that use array objects.

@@ -372,17 +372,13 @@ bool CArrayObj::Delete(const int index)
 bool CArrayObj::Delete(const int index)
   {
 //--- check
-   if(index>=m_data_total)
+   if(index < 0 || index>=m_data_total)
       return(false);
 //--- delete
-   if(index<m_data_total-1)
-     {
-      if(index>=0)
-         MemMove(index,index+1,m_data_total-index-1);
-     }
-   else
    if(m_free_mode && CheckPointer(m_data[index])==POINTER_DYNAMIC)
       delete m_data[index];
+   if(index<m_data_total-1)
+      MemMove(index,index+1,m_data_total-index-1);
    m_data_total--;
 //--- successful
    return(true);


In the buggy code, the object pointer is first overwritten by the MemMove and then whatever is present in the array after the move is what is presented to the delete operator, which could cause catastrophic results. Based on the indenting of the original code after the else, this looks like code that was half finished.

Please either integrate my changes or those of your own to fix this.

Thanks.

 
whroeder1:
This is a user's forum not Metaquotes.
sure.. thanks for the links. I'll report it there as well. But this is something that all users who code with the object classes need to know about, no?
 
fnordglunk39679:

To Metaquotes developers:

This is a deep bug that has been propagated in all versions of both MT4 and MT5 include file packages and would cause problems for all users of any of the object classes that use array objects.


In the buggy code, the object pointer is first overwritten by the MemMove and then whatever is present in the array after the move is what is presented to the delete operator, which could cause catastrophic results. Based on the indenting of the original code after the else, this looks like code that was half finished.

Please either integrate my changes or those of your own to fix this.

Thanks.

I have this code, build 1714. What you posted is confusing.

//+------------------------------------------------------------------+
//| Deleting element from the specified position                     |
//+------------------------------------------------------------------+
bool CArrayObj::Delete(const int index)
  {
//--- check
   if(index>=m_data_total)
      return(false);
//--- delete
   if(index<m_data_total-1)
     {
      if(index>=0 && MemMove(index,index+1,m_data_total-index-1)<0)
         return(false);
     }
   else
   if(m_free_mode && CheckPointer(m_data[index])==POINTER_DYNAMIC)
      delete m_data[index];
   m_data_total--;
//--- successful
   return(true);
  }

Anyway there is no bug on this code.

If MemMove() is successfully executed, the next statement is "m_data_total--";

 
Alain Verleyen:

I have this code, build 1714. What you posted is confusing.

Anyway there is no bug on this code.

If MemMove() is successfully executed, the next statement is "m_data_total--";

I'm sorry you find diff output confusing. You can apply this patch to the code and it will show the correct. Here is the new without the diff output "confusing" you.

//+------------------------------------------------------------------+
//| Deleting element from the specified position                     |
//+------------------------------------------------------------------+
bool CArrayObj::Delete(const int index)
  {
//--- check
   if(index < 0 || index>=m_data_total)
      return(false);
//--- delete
   if(m_free_mode && CheckPointer(m_data[index])==POINTER_DYNAMIC)
      delete m_data[index];
   if(index<m_data_total-1)
      MemMove(index,index+1,m_data_total-index-1);
   m_data_total--;
//--- successful
   return(true);
  }



In the old code, if MemMove is successfully executed and these are dynamic pointers, then the pointer passed to delete is not the correct one because it has been overwritten. So the pointer meant to be deleted is not, and something else is passed to delete. This is a memory leak and potential for use after free for the other pointer. The delete should be executed first, not after the pointer is overwritten. Most definitely a bug.

 
Alain Verleyen: Anyway there is no bug on this code.
No check for negative index. Have to delete index, before the move looses it.
 
fnordglunk39679:

I'm sorry you find diff output confusing. You can apply this patch to the code and it will show the correct. Here is the new without the diff output "confusing" you.



In the old code, if MemMove is successfully executed, then the pointer passed to delete is not the correct one because it has been overwritten. So the pointer meant to be deleted is not, and something else is passed to delete. This is a memory leak and potential for use after free for the other pointer. The delete should be executed first, not after the pointer is overwritten. Most definitely a bug.

I always found the diff output confusing. Doesn't matter.

I posted the code from build 1714 (last beta). There is not bug there.

Could you posted the original code please, and say which build you are using ?

 
whroeder1:
No check for negative index. Have to delete index, before the move looses it.

Not sure what code you are talking about. In the code I posted which is the current official version, there is a check for negative number.

The data is either delete OR moved, not both.

 
Alain Verleyen:

Not sure what code you are talking about. In the code I posted which is the current official version, there is a check for negative number.

The data is either delete OR moved, not both.

if the data is only deleted, then if the move is not executed then the array has a bogus pointer that is pointing to freed memory. It must always be moved, and if the pointer is a dynamic pointer it must be deleted before the rest of the array is moved up and overwrites the pointer.  The array holds pointers to each data element. All pointers after the one to be deleted must be moved up (and only after this pointer is deleted if it is dynamic). This shrinks the array by one to remove the deleted element. The move up operation must always be done to have the array be correct.

The original is as you posted.

here it is in pseudo graphical:

I have an array of 3 object pointers, each object pointer pointing to some area of memory. The memory pointed to can be either static (meaning already allocated by the actual code before runtime) or dynamic (meaning allocated at the time of need at run time and must be freed to avoid memory leaks).


          | dp0 | dp1 | dp2 |
          ------------------------

index:   0        1         2

with dp0 -> pointing to d0 |....|, dp1 -> pointing to d1 |....|, and dp2 -> pointing to d2 |.....|

.. say I want to delete index 1 object so the array should look like this:

          | dp0 |  dp2 |
          -----------------

index:   0        1

In order to do this, _first_ the pointer dp1 must be passed to delete so d1 can be deleted. then the array contents of all the pointers after must be moved up to get rid of the hole in the array.  If dp1 points to static memory, no delete is needed and only the move up to cover the hole must be done.


In any case, the number of elements must be decremented whether or not the MemMove succeeds.. and if it fails, in my fix, the entire array is wrong because the pointed to memory was already deleted. Failure of the MemMove could leave a pointer to freed memory causing a "use after free" which is almost as bad as a memory leak but unsure what to do about this.

 
fnordglunk39679:

if the data is only deleted, then if the move is not executed then the array has a bogus pointer that is pointing to freed memory. It must always be moved, and if the pointer is a dynamic pointer it must be deleted before the rest of the array is moved up and overwrites the pointer.  The array holds pointers to each data element. All pointers after the one to be deleted must be moved up (and only after this pointer is deleted if it is dynamic). This shrinks the array by one to remove the deleted element. The move up operation must always be done to have the array be correct.

The original is as you posted.

here it is in pseudo graphical:

I have an array of 3 object pointers, each object pointer pointing to some area of memory. The memory pointed to can be either static (meaning already allocated by the actual code before runtime) or dynamic (meaning allocated at the time of need at run time and must be freed to avoid memory leaks).


          | dp0 | dp1 | dp2 |
          ------------------------

index:   0        1         2

with dp0 -> pointing to d0 |....|, dp1 -> pointing to d1 |....|, and dp2 -> pointing to d2 |.....|

.. say I want to delete index 1 object so the array should look like this:

          | dp0 |  dp2 |
          -----------------

index:   0        1

In order to do this, _first_ the pointer dp1 must be passed to delete so d1 can be deleted. then the array contents of all the pointers after must be moved up to get rid of the hole in the array.  If dp1 points to static memory, no delete is needed and only the move up to cover the hole must be done.


In any case, the number of elements must be decremented whether or not the MemMove succeeds.. and if it fails, in my fix, the entire array is wrong because the pointed to memory was already deleted. Failure of the MemMove could leave a pointer to freed memory causing a "use after free" which is almost as bad as a memory leak but unsure what to do about this.

There is no bug. The delete is done in MemMove. Look better and test the code.
 
Alain Verleyen:
There is no bug. The delete is done in MemMove. Look better and test the code.

Ah.


Now that is confusing.

The function name is MemMove not MemMoveAndDeleteIfDynamic.

Yes, no bug. thanks for showing this to me. You can delete this whole chain if you wish.



12
New comment