[PATCH] Fix basic_string::reserve bug PR4548

[Paolo Carlini <pcarlini at unitus dot it>]

Hi all,

The PR is about a bug of str.reserve(x) when x < str.size() and str is shared (the
latter condition is important).

Also thanks to some hints from the submitter, the analysis was simple, the resulting
patch is very short (3 lines) and non invasive and probably a good candidate for
gcc-3_0 too. Nevertheless, the bug is nasty and leads to memory corruption.

In a nutshell (see below for details), currently, when a given string is shared
basic_string::reserve does not adjust the requested value for its capacity()
to a final value in any case >= the current size(). This is very bad :)

The following small testcase clearly illustrates the problem:

   1 #include <string>
   2 #include <iostream>
   3
   4 int main()
   5 {
   6   std::string str_one = "WWW GNU WWW GNU";
   7
   8   std::string str_two;
   9
  10   str_two = str_one;
  11
  12   str_one.reserve(1);
  13
  14   std::cout << str_one.capacity();
  15 }

The output is 1 instead of 15 !!!!

The following patch has been tested and regression checked on i686-pc-linux-gnu.
I'm also including a new test for the testsuite.

Cheers,

Paolo Carlini  <pcarlini@unitus.it>

        // libstdc++/4548
        * include/bits/basic_string.tcc (basic_string::reserve): Never shrink
        below the current size.
        * testsuite/21_strings/capacity.cc: Add test.


--- basic_string.tcc.orig       Sat Nov 17 17:01:19 2001
+++ basic_string.tcc    Sat Nov 17 19:46:45 2001
@@ -315,6 +315,9 @@ namespace std
         {
          if (__res > this->max_size())
            __throw_length_error("basic_string::reserve");
+         // Make sure we don't shrink below the current size
+         if (__res < this->size())
+           __res = this->size();
          allocator_type __a = get_allocator();
          _CharT* __tmp = _M_rep()->_M_clone(__a, __res - this->size());
          _M_rep()->_M_dispose(__a);


--- capacity.cc.orig    Sat Nov 17 21:31:48 2001
+++ capacity.cc Sat Nov 17 22:01:25 2001
@@ -169,9 +169,28 @@ bool test01()
   return test;
 }

+// libstdc++/4548
+bool test02()
+{
+  bool test = true;
+
+  std::string str01 = "twelve chars";
+  // str01 becomes shared
+  std::string str02 = str01;
+  str01.reserve(1);
+  VERIFY( str01.capacity() == 12 );
+
+#ifdef DEBUG_ASSERT
+  assert(test);
+#endif
+
+  return test;
+}
+
 int main()
 {
   test01();
+  test02();

   return 0;
 }


Detailed analysis of the testcase above:
----------------------------------------

In line #12 of the testcase above str_one.reserve is called:

  template<typename _CharT, typename _Traits, typename _Alloc>
    void
    basic_string<_CharT, _Traits, _Alloc>::reserve(size_type __res)
    {
      if (__res > this->capacity() || _M_rep()->_M_is_shared())
        {
         if (__res > this->max_size())
           __throw_length_error("basic_string::reserve");
         allocator_type __a = get_allocator();
         _CharT* __tmp = _M_rep()->_M_clone(__a, __res - this->size());
         _M_rep()->_M_dispose(__a);
         _M_data(__tmp);
        }
    }

Even if __res > this->capacity() is false (__res == 1 and this->capacity() == 15),
nevertheless the if body is executed since the string str_one is shared.
Therefore _M_clone is called with the second argument == -14 < 0. Suspect!

  template<typename _CharT, typename _Traits, typename _Alloc>
    _CharT*
    basic_string<_CharT, _Traits, _Alloc>::_Rep::
    _M_clone(const _Alloc& __alloc, size_type __res)
    {
      _Rep* __r = _Rep::_S_create(_M_length + __res, __alloc);
      if (_M_length)
 {
   try
     { traits_type::copy(__r->_M_refdata(), _M_refdata(), _M_length); }
   catch(...)
     {
       __r->_M_destroy(__alloc);
       __throw_exception_again;
     }
 }
      __r->_M_length = _M_length;
      return __r->_M_refdata();
    }

In _M_clone _Rep::_S_create is called with the first argument == 1, that is the
clone is created with an initial capacity of 1. Then, since _M_length == 15, the if
body is executed and traits::copy is called to fill the new clone with a third
parameter of 15, that is 15 characters are going to be copied in the clone which has
a capacity of 1!!! No check at all is carried out by the fast low level copy function
(in char_traits.h) involved:

 static char_type*
 copy(char_type* __s1, const char_type* __s2, size_t __n)
 {  return static_cast<char_type*>(memcpy(__s1, __s2, __n)); }

Bummer!!!