Bad memory management in algorithm::is_any_of

[jgottman@…]

The is_any_of predicate has been modified to use either a fixed buffer or dynamic memory allocation, depending on its size. There are two major problems when it uses dynamic memory allocation. See the code in boost_1_36_0\boost\algorithm\string\detail\classification.hpp.

1) It allocates memory with the command

        m_Storage.m_dynSet=new set_value_type[m_Size];

but it deallocates with the command

        delete m_Storage.m_dynSet;

This should be

        delete[] m_Storage.m_dynSet;

or else we will have undefined behavior.

2) The first line of its assignment operator is

      m_Storage.m_dynSet=0;

This means that if m_dynSet has had memory allocated that memory will leak. Also, this means that it is unsafe under self-assignment.

Here is some untested code that should help with the assignment problem:

is_any_ofF& operator=(const is_any_ofF& Other)
{
   const set_value_type* SrcStorage=0;
   set_value_type* DestStorage=0;                
   if (Other.m_Size <= FIXED_STORAGE_SIZE) {
       SrcStorage = &Other.m_Storage.m_fixSet[0];
       if (m_Size > FIXED_STORAGE_SIZE) {
           m_Storage.m_dynSet  =  new set_value_typ[Other.m_Size]; 
           DestStorage = m_Storage.m_dynSet;
       } else {
           DestStorage = &m_Storage.m_fixSet[0];
       }
  } else { 
       SrcStorage = Other.m_Storage.m_dynSet;
       if (m_Size<= FIXED_STORAGE_SIZE) {
          m_Storage.m_dynSet  =  new set_value_type[Other.m_Size]; 
       } else if (m_Size <= Other.m_Size) { 
          //We already have enough space allocated, so no new allocation required
       } else {
          //Use temporary var for strong exception safety
          set_value_type *temp = new set_value_type[Other.m_Size]; 
          delete[] m_Storage.m_dynSet;
          m_Storage.m_dynSet = temp;
       }
       DestStorage = m_Storage.m_dynSet;
  }
  m_Size = Other.m_Size;
  ::memcpy(DestStorage, SrcStorage, sizeof(set_value_type)*m_Size);
  return *this;
}

[Pavol Droba]

Thanks for spotting that out. I have just commited a fix into the trunk.

[Joe Gottman <jgottman@…>]

This doesn't fix the self-assignment problem. If we have code like

   is_any_of<char> foo("abcdefghijklmnopqrstuvwxyz");
   foo = foo;

then the operator= as currently written will delete this->m_Storage.m_dynSet (which is the same thing as Other.m_Storage.m_dynSet), and replace it with garbage. There are two possible ways to fix this. The first is to put the following code at the beginning of operator=

    if (this == &Other) return *this;

The second is to do the following: If *this and Other both allocate memory, check whether Other->m_Size is less than or equal to this->m_Size. If it is, then we don't have to reallocate this->m_Storage.m_dynSet. It will already have enough space allocated to hold the string from Other. It might have more space allocated than necessary, but that is no big deal. This saves us an unnecessary reallocation in some cases, including the self-assignment case.

[Pavol Droba]

Ok, I have used the combination of both. Self assignment case is tested at the beginning and the new space is not allocate if newsize <= size < 2*newsize.