Modify

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#2200 closed Bugs (fixed)

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:

Description

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;
}

Attachments (0)

Change History (4)

comment:1 Changed 9 years ago by Marshall Clow

Component: Nonestring_algo
Milestone: Boost 1.36.0To Be Determined
Owner: set to Pavol Droba

comment:2 Changed 9 years ago by Pavol Droba

Resolution: fixed
Status: newclosed

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.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Pavol Droba.
The resolution will be deleted.

Add Comment


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

 
Note: See TracTickets for help on using tickets.