Opened 10 years ago

Closed 3 months ago

Last modified 3 months ago

#2290 closed defect (fixed)

inserting element of wxArrayString back into array

Reported by: aebailey Owned by: vadz
Priority: low Milestone:
Component: base Version:
Keywords: wxArrayString Cc: aebailey
Blocked By: Blocking:
Patch: yes

Description

This code causes a memory access violation:

wxArrayString array;
array.Add("new wxString");
array.Insert(array[0], 100);

Insert() has a reference to a wxString, which gets moved
during Grow(). The reference is then invalid. This only
happens if you try to insert enough items to cause a
reallocation of the internal array.

I have seen this problem with wxWidgets 2.4.2 and
2.5.3, and it doesn't look like 2.5.4 fixes it.

Change History (15)

comment:1 Changed 10 years ago by aebailey

Oops. That last line of code should have been
array.Insert(array[0], 1, 100);

comment:2 Changed 6 years ago by wojdyr

  • Component set to base
  • Keywords wxArrayString added
  • Status changed from new to confirmed

it's also the case in 2.9

==14409== Invalid read of size 8
==14409== at 0x85954F4: std::string::assign(std::string const&) (in /usr/lib/libstdc++.so.6.0.9)
==14409== by 0x5A10E9A: wxString::operator=(wxString const&) (string.h:1352)
==14409== by 0x5A1562E: wxArrayString::Insert(wxString const&, unsigned long, unsigned long) (arrstr.cpp:314)

comment:3 Changed 3 months ago by oneeyeman

And it is still happens in TRUNK.

comment:4 Changed 3 months ago by oneeyeman

IIUC, in order to fix this use case the string needs to be saved before calling "Grow()" and then this temporary string should be used.

Vadim, do you need a patch or just confirmation that it works?

comment:5 Changed 3 months ago by vadz

It's not really great to always copy the string an extra time unconditionally but if there is no other way to make it work, we probably should still do it.

To be honest, I'm not sure if this is guaranteed to work with the standard containers. If it isn't, I could live with it being unsupported in wx container classes neither.

comment:6 Changed 3 months ago by oneeyeman

  • Patch set

Here is the patch:

diff -bru wxWidgets/src/common/arrstr.cpp /mnt/winxp/wxWidgets/src/common/arrstr.cpp
--- wxWidgets/src/common/arrstr.cpp	2014-06-14 17:47:19.000000000 -0700
+++ /mnt/winxp/wxWidgets/src/common/arrstr.cpp	2014-07-11 03:02:59.781250000 -0700
@@ -307,7 +307,7 @@
   wxCHECK_RET( nIndex <= m_nCount, wxT("bad index in wxArrayString::Insert") );
   wxCHECK_RET( m_nCount <= m_nCount + nInsert,
                wxT("array size overflow in wxArrayString::Insert") );
-
+  wxString temp = str;
   Grow(nInsert);
 
   for (int j = m_nCount - nIndex - 1; j >= 0; j--)
@@ -315,7 +315,7 @@
 
   for (size_t i = 0; i < nInsert; i++)
   {
-      m_pItems[nIndex + i] = str;
+      m_pItems[nIndex + i] = temp;
   }
   m_nCount += nInsert;
 }

I will try to test std::array in the meantime.

comment:7 follow-up: Changed 3 months ago by plorkyeran

The standard doesn't directly address whether or not this should work with std::vector, which is generally interpreted as meaning that it should. libstdc++'s solution is to allocate the new buffer, copy the new item into place, copy or move the old items, and then deallocate the old buffer, which makes it work without ever requiring an extra copy.

comment:8 Changed 3 months ago by vadz

Thank you, this is of course what we should do too, there is really no reason not to if it can be done without any extra cost. All we need to is to refactor Grow() to allow separating the allocation of the new buffer and the deletion of the old one...

comment:9 in reply to: ↑ 7 Changed 3 months ago by oneeyeman

Hi,
Replying to plorkyeran:

The standard doesn't directly address whether or not this should work with std::vector, which is generally interpreted as meaning that it should. libstdc++'s solution is to allocate the new buffer, copy the new item into place, copy or move the old items, and then deallocate the old buffer, which makes it work without ever requiring an extra copy.

And what about MSVC? Especially the old compiler wxWidgets still supports...

comment:10 follow-up: Changed 3 months ago by plorkyeran

2010 has some delightful bugs in its implementation of rvalue references that breaks it for unrelated reasons (it incorrectly selects the && overload of insert and moves the item rather than copying it), but AFAIK it works correctly in all other versions.

comment:11 in reply to: ↑ 10 Changed 3 months ago by oneeyeman

Replying to plorkyeran:

2010 has some delightful bugs in its implementation of rvalue references that breaks it for unrelated reasons (it incorrectly selects the && overload of insert and moves the item rather than copying it), but AFAIK it works correctly in all other versions.

You mean the code above with std::vector<T>? Did you try it?
And like I said what about MSVC 2005? This is the earlist supported....

comment:12 Changed 3 months ago by plorkyeran

I don't have anything older than 2013 installed any more, but the accepted answer to http://stackoverflow.com/questions/11653111/stdvector-inconsistent-crash-between-push-back-and-insertend-x mentions the bug I was talking about. I know it worked correctly with 2008 (as I ran into issues when upgrading from 2008 to 2010), but I don't know about 2005.

comment:13 Changed 3 months ago by vadz

  • Owner set to vadz
  • Status changed from confirmed to accepted

comment:14 Changed 3 months ago by VZ

  • Resolution set to fixed
  • Status changed from accepted to closed

In 76907:

Fix inserting an element of wxArrayString itself back into it.

Do the insertion/addition before deallocating the old memory to allow things
like array.Add(array[0]) to work correctly.

Closes #2290.

comment:15 Changed 3 months ago by VZ

In 76909:

Fix inserting an element of wxArrayString itself back into it.

Do the insertion/addition before deallocating the old memory to allow things
like array.Add(array[0]) to work correctly.

Closes #2290.

Note: See TracTickets for help on using tickets.