Re: [PATCH] Add overflow checking to basic_string append and push_back

[Martin Sebor]

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