Mailing-List: contact stdcxx-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: stdcxx-dev@incubator.apache.org
Received-SPF: pass (athena.apache.org: local policy)
Message-ID: <46F2D039.5070508@roguewave.com>
Date: Thu, 20 Sep 2007 13:55:37 -0600
From: Martin Sebor <sebor@roguewave.com>
Organization: Rogue Wave Software, Inc.
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US;
 rv:1.8.1.6) Gecko/20070802 SeaMonkey/1.1.4
MIME-Version: 1.0
To: stdcxx-dev@incubator.apache.org
Subject: Re: [PATCH] Add overflow checking to basic_string append and
 push_back
References: 
 <C8506D6098A6F545BA3745D0CE359E830194EC2E@qxvcexch01.ad.quovadx.com>
In-Reply-To: 
 <C8506D6098A6F545BA3745D0CE359E830194EC2E@qxvcexch01.ad.quovadx.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Travis Vitek wrote:
> It appears that recent changes to string have accidentally removed some
> overflow checking that used to be in the basic_string::append() and
> push_back() methods. The following patch adds the checks back in.

Does this fix a test failure? Or some regression? (If the former,
which test? If the latter, we need a test case :)

In any event, adding an if to these two performance sensitive
functions looks risky from an efficiency POV (we'd definitely
need to see before and after timings to consider the patch).

The change also seems unnecessary -- when size() equals capacity()
we check that it doesn't exceed max_size() before allocating more
memory in append(). Otherwise, when size() is less than capacity()
(or rather capacity() - 1), there should be no reason to check
against max_size() because we know that capacity() must have
been below max_size() the last time we reallocated.

Martin

> 
> Travis
> 
> 
> 2007-09-20  Travis Vitek  <vitek@roguewave.com>
> 
> 	* string (append): add integer overflow check
> 	(push_back): Same
> 
> ===================================================================
> --- string	(revision 576541)
> +++ string	(working copy)
> @@ -1088,6 +1088,11 @@
>  inline void basic_string<_CharT, _Traits, _Allocator>::
>  push_back (value_type __c)
>  {
> +    _RWSTD_REQUIRES (size () <= max_size () - 1,
> +                     (_RWSTD_ERROR_LENGTH_ERROR,
> +                      _RWSTD_FUNC ("basic_string::append(value_type)"),
> +                      size (), max_size () - 1));
> +
>      const size_type __size = size () + 1;
>  
>      if (   capacity () < __size
> @@ -1095,7 +1100,6 @@
>          append (1, __c);
>      else {
>          traits_type::assign (_C_data [size ()], __c);
> -        // append the terminating NUL character
>          traits_type::assign (_C_data [__size], value_type ());
>          _C_pref ()->_C_size._C_size = __size;
>      }
> @@ -1196,6 +1200,12 @@
>  basic_string<_CharT, _Traits, _Allocator>::
>  append (const_pointer __s, size_type __n)
>  {
> +    _RWSTD_REQUIRES (size () <= max_size () - __n,
> +                     (_RWSTD_ERROR_LENGTH_ERROR,
> +                      _RWSTD_FUNC
> ("basic_string::append(const_pointer,"
> +                                   " size_type)"),
> +                      size (), max_size () - __n));
> +
>      const size_type __newsize = size () + __n;
>  
>      if (   capacity () <= __newsize