stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Martin Sebor <se...@roguewave.com>
Subject Re: [PATCH] Add overflow checking to basic_string append and push_back
Date Fri, 21 Sep 2007 00:27:08 GMT
Travis Vitek wrote:
> Stupid outlook.
>  
>> [21.3.5.2 p4] Effects: Determines the effective length rlen of the
>> string to append as the smaller of n and str.size() - pos. The function
>> then throws length_error if size() >= npos - rlen.
>>
>> The append that I'm invoking is described to behave as if it had called
>> that function.

Not quite. It's described to *return the same value* as the other
one. There's a subtle but important difference -- see issue 625:
http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-active.html#625

But I agree, both calls to append() in your test case should throw
length_error due to 21.3, 4a) and irrespective of the issue.

That being said, rather than bloating the inline function by adding
a _RWSTD_REQUIRES() statement I wonder if we could achieve the same
effect by using size_t instead of size_type. The patch below fixed
the problem in the recently changed overload of append().

However, I suspect this (the absence of overflow handling) is
a pervasive problem throughout all of string and probably also
throughout the rest of our containers. So I think we should try
address it consistently throughout the whole library instead of
on a case by cases basis. If you agree, I suggest you create an
issue in Jira and schedule it for some release after 4.2.

Martin

Index: /nfs/homes/sebor/stdcxx/include/string
===================================================================
--- /nfs/homes/sebor/stdcxx/include/string      (revision 577852)
+++ /nfs/homes/sebor/stdcxx/include/string      (working copy)
@@ -1196,7 +1196,7 @@
  basic_string<_CharT, _Traits, _Allocator>::
  append (const_pointer __s, size_type __n)
  {
-    const size_type __newsize = size () + __n;
+    const _RWSTD_SIZE_T __newsize = size () + __n;

      if (   capacity () <= __newsize
          || size_type (1) < size_type (_C_pref ()->_C_get_ref ()))



>> The attached test case shows that we are not following
>> through on that requirement.
>>
> 
> #include <string>
> #include <stdexcept>
> 
> template <class T>
> struct Xallocator: std::allocator<T>
> {
>     //typedef typename std::allocator<T>::size_type size_type;
>     typedef unsigned char size_type;
> 
>     Xallocator (): std::allocator<T>() { } 
>     Xallocator (const Xallocator &rhs): std::allocator<T>(rhs) { }
>     template <class U>
>     Xallocator (const Xallocator<U> &rhs): std::allocator<T>(rhs) { }
> 
>     template <class U> 
>     struct rebind { typedef Xallocator<U> other; };
> 
>     size_type max_size () const { return 255; }
> };
> 
> typedef std::basic_string<char, std::char_traits<char>, Xallocator<char>
> String;
> 
> 
> #include <stdio.h>
> #include <assert.h>
> 
> int main()
> {
>     bool throw_required = false;
>     bool throw_happened = false;
> 
>     try {
>         String a (240, 'a');
>         String b ( 20, 'b');
> 
>         const String::size_type rlen = b.size();
> 
>         if (a.size () >= String::npos - rlen)
>             throw_required = true;
> 
>         // this throws correctly
>         //a.append (b, 0, String::npos);
> 
>         // this does not
>         a.append (b.c_str (), b.size ());
> 
>         // yet they are supposed to be equivalent
>     }
>     catch (const std::length_error&) {
>         throw_happened = true;
>     }
>     catch (...) {
>     }
> 
>     if (throw_required) {
>         assert (throw_happened);
>     }
> 
>     return 0;
> }


Mime
View raw message