Ticket #2200 (closed Bugs: fixed)

Opened 9 years ago

Last modified 9 years ago

Bad memory management in algorithm::is_any_of

Reported by: jgottman@… Owned by: pavol_droba
Milestone: To Be Determined Component: string_algo
Version: Boost 1.35.0 Severity: Problem
Keywords: string algorithms Cc:


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


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;


Change History

comment:1 Changed 9 years ago by marshall

  • Owner set to pavol_droba
  • Component changed from None to string_algo
  • Milestone changed from Boost 1.36.0 to To Be Determined

comment:2 Changed 9 years ago by pavol_droba

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

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

comment:3 Changed 9 years ago by 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.

comment:4 Changed 9 years ago by 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.


Add a comment

Modify Ticket

Change Properties
<Author field>
as closed
The resolution will be deleted. Next status will be 'reopened'

E-mail address and user name can be saved in the Preferences.

Note: See TracTickets for help on using tickets.