musingstudio

I’m a big fan of C++11 and the new features that have become available, including move semantics.  However, our team found today that a code optimization which worked under VS 2005 was causing memory corruption under VS 2010.

The story goes back to a string holder class a bit like this (if you think BSTRs you’ll be on the right lines, but taking COM out of the picture makes the example clearer):

class CDeepCopy
{
protected:
  char* m_pStr;
  size_t m_length;

  void clone( size_t length, const char* pStr )
  {
    m_length = length;
    m_pStr = new char [m_length+1];
    for ( size_t i = 0; i < length; ++i )
    {
      m_pStr[i] = pStr[i];
    }
    m_pStr[length] = '';
  }

public:
  CDeepCopy() : m_pStr( nullptr ), m_length( 0 )
  {
  }

  CDeepCopy( const std::string& str )
  {
    clone( str.length(), str.c_str() );
  }

  CDeepCopy( const CDeepCopy& rhs )
  {
    clone( rhs.m_length, rhs.m_pStr );
  }

  CDeepCopy& operator=( const CDeepCopy& rhs )
  {
    if (this == &rhs)
      return *this;

    clone( rhs.m_length, rhs.m_pStr );
    return *this;
  }

  bool operator<( const CDeepCopy& rhs ) const
  {
    if (m_length < rhs.m_length)
      return true;
    else if (rhs.m_length < m_length)
      return false;

    return strcmp( m_pStr, rhs.m_pStr ) < 0;
  }

  virtual ~CDeepCopy()
  {
    delete [] m_pStr;
  }
};

CDeepCopy was used as the key into a map. For large data sets with heavily overlapping key values, this was inefficient because each lookup would entail the cost of a copy constructor – this was particularly expensive due to the heap operations. As an optimization, the following technique was used to avoid the copy constructor call in the event of an update operation, yet worked correctly if the operator[]() call led to an insertion.

class CShallowCopy : public CDeepCopy
  {
  public:

  CShallowCopy( const std::string& str ) : CDeepCopy()
  {
    m_pStr = const_cast<char*>(str.c_str());
    m_length = str.length();
  }

  ~CShallowCopy()
  {
    m_pStr = nullptr;
  }
};

So you could do an efficient lookup like this that only caused a deep copy if an insertion into the map was required instead of an update:

int _tmain(int argc, _TCHAR* argv[])
{
  std::map<CDeepCopy, int> entries;
  std::string hello( "Hello" );

  CDeepCopy key( hello );
  entries[key] = 1;

  // Named variable
  CShallowCopy key2( hello );
  entries[key2] = 2;

  // Unnamed temporary variable
  entries[ CShallowCopy( hello ) ] = 3;

  return 0;
}

This code worked as intended in VS2005. However, compiled under VS2010 we were seeing memory corruption in the final case (the one with the unnamed temporary). After some debugging, we realised that under C++11, std::map has an operator[]( KeyT&& key ), that is the key is an rvalue reference – and this form is called for our unnamed temporary case. That avoids calling our CDeepCopy copy constructor, and so the key in the map can be left referencing deallocated memory.

A simple workaround prevents this happening again – declare CDeepCopy( CDeepCopy&& ) as private but omit the definition – this means that the unnamed temporary usage does not compile.