C++ Custom std::map<> key class causing memory violation

Question

For the first time I've written a class that is supposed to be usable as a key type for std::map<>. I've overloaded copy constructor, assignment, and operator < as suggested in other questions on SO. But for some reason it crashes when I'm trying to insert using operator []. This class is meant to hold a buffer of binary data whose length is indicated by the member m_nLen.

Here is the code :

class SomeKeyClass
{
public:

   unsigned char m_buffer[ SOME_LENGTH_CONSTANT ];

   size_t m_nLen;

public:

   inline SomeKeyClass( const unsigned char * data, size_t nLen )
   {
      m_nLen = min( SOME_LENGTH_CONSTANT, nLen );
      memcpy( m_buffer, data, m_nLen );
   }

   inline SomeKeyClass( const SomeKeyClass& oKey )
   {
      *this = oKey;
   }

   inline bool operator < ( const SomeKeyClass& oKey ) const
   {
      return memcmp( m_buffer, oKey.m_buffer, min( m_nLen, oKey.m_nLen ) ) < 0;
   }

   inline SomeKeyClass & operator = ( const SomeKeyClass& oKey )
   {
      memcpy( m_buffer, oKey.m_buffer, oKey.m_nLen );
      return *this;
   }
};

Is there anything wrong with this class? Could I use std::string<unsigned char> for using binary data as keys instead?

Answer 1

The issue is that you were not setting the m_nLen member in the copy constructor or the assignment operator. Thus whenever you use the object that has the uninitialized or wrong m_nLen value, things may go wrong leading to possible crashes (in general, undefined behavior).

When implementing a user-defined copy constructor and assignment operator, you should strive to make sure that what comes out at the end is an actual copy of the object in question (reference counted objects are a special case, but it still implies that a copy is being done). Otherwise, programs that produce incomplete or wrong copies of the object are very fragile, and an awful burden to debug.

Answer 2

See Paul McKenzie's answer for the reason it crashes.

Is there anything wrong with this class ?

Yes, your operator< is broken.

Consider the case where you have one key "abc" and another key "abcd", your less-than operator will say they are equivalent, because you only test the first 3 characters.

A correct implementation needs to compare the lengths when memcmp says they are equal, because the memcmp call doesn't necessarily compare the full strings:

bool operator<(const SomeKeyClass& oKey) const
{
  const std::size_t len = std::min(m_nLen, oKey.m_nLen);
  if (len > 0)
  {
    const int cmp = memcmp(m_buffer, oKey.m_buffer, len);
    if (cmp != 0)
      return cmp < 0;
  }
  return m_nLen < oKey.m_nLen;
}