[Bug libstdc++/48257] New: std::string::assign() corrupts std::string static data when called on emptyString1 using emptyString2.data()

[Bug libstdc++/48257] New: std::string::assign() corrupts std::string static data when called on emptyString1 using emptyString2.data()

[mohsinrzaidi at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48257

           Summary: std::string::assign() corrupts std::string static data
                    when called on emptyString1 using emptyString2.data()
           Product: gcc
           Version: 4.1.2
            Status: UNCONFIRMED
          Severity: minor
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: mohsinrzaidi@gmail.com


The output of the following program is:
4
4

#include <string>
#include <iostream>

using namespace std;
int main()
{
    try
    {
        string emptyStr1;
        string emptyStr2;
        string emptyStr3;

        emptyStr3.assign(emptyStr2.data(), 4);

        cout << emptyStr1.size() << endl;
        cout << emptyStr3.size() << endl;
    }
    catch(...)
    {
        cout << "Reached here" << endl;
    }
}

The size of emptyStr1/3 should not have been modified when emptyStr3 was
assigned the contents of emptyStr2 up to 4 bytes although all string are empty.

>From what I've understood of the basic_string code, the following code flow
will result in this case:

<snip>

  template<typename _CharT, typename _Traits, typename _Alloc>
    inline basic_string<_CharT, _Traits, _Alloc>::
    basic_string()
#ifndef _GLIBCXX_FULLY_DYNAMIC_STRING
    : _M_dataplus(_S_empty_rep()._M_refdata(), _Alloc()) { }
#else
    : _M_dataplus(_S_construct(size_type(), _CharT(), _Alloc()), _Alloc()) { }
#endif

</snip>

First, the default constructor. Since I do not have
"_GLIBCXX_FULLY_DYNAMIC_STRING" defined in my implementation, _M_dataplus._M_p
will get initialized to &(_S_empty_rep_storage + sizeof (_Rep)) for all three
strings.

<snip>

      const _CharT*
      data() const
      { return _M_data(); }

</snip>

<snip>

      _CharT*
      _M_data() const
      { return  _M_dataplus._M_p; }

</snip>

<snip>

  template<typename _CharT, typename _Traits, typename _Alloc>
    basic_string<_CharT, _Traits, _Alloc>&
    basic_string<_CharT, _Traits, _Alloc>::
    assign(const _CharT* __s, size_type __n)
    {
      __glibcxx_requires_string_len(__s, __n);
      _M_check_length(this->size(), __n, "basic_string::assign");
      if (_M_disjunct(__s) || _M_rep()->_M_is_shared())
    return _M_replace_safe(size_type(0), this->size(), __s, __n);
      else
    {
      // Work in-place.
      const size_type __pos = __s - _M_data();
      if (__pos >= __n)
        _M_copy(_M_data(), __s, __n);
      else if (__pos)
        _M_move(_M_data(), __s, __n);
      _M_rep()->_M_set_length_and_sharable(__n);
      return *this;
    }
     }

</snip>

When assign is called using emptyStr2.data(), which returns the same value as
that set above for _M_dataplus._M_p, _M_disjunct() will return 0, as will
_M_is_shared() and we end up going into the else block.

Here, since __s = _M_dataplus._M_p, which is what _M_data() returns, __pos = 0
and so the only statements we end up executing are the last two in the else
block thereby setting the length = 4 in the static storage returned by
_M_rep().

This results in the corruption of static storage used by all std::string
objects.

Does that make sense?

==========================
Using built-in specs.
Target: x86_64-redhat-linux
Configured with: ../configure --prefix=/usr --mandir=/usr/share/man
--infodir=/usr/share/info --enable-shared --enable-threads=posix
--enable-checking=release --with-system-zlib --enable-__cxa_atexit
--disable-libunwind-exceptions --enable-libgcj-multifile
--enable-languages=c,c++,objc,obj-c++,java,fortran,ada --enable-java-awt=gtk
--disable-dssi --enable-plugin
--with-java-home=/usr/lib/jvm/java-1.4.2-gcj-1.4.2.0/jre --with-cpu=generic
--host=x86_64-redhat-linux
Thread model: posix
gcc version 4.1.2 20080704 (Red Hat 4.1.2-48)

[Bug libstdc++/48257] std::string::assign() corrupts std::string static data when called on emptyString1 using emptyString2.data()

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48257

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |INVALID

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-03-23 16:59:31 UTC ---
this is undefined behaviour, the standard requires that emptyStr2.data() points
to an array of at least 4 chars

[Bug libstdc++/48257] std::string::assign() corrupts std::string static data when called on emptyString1 using emptyString2.data()

[mohsinrzaidi at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48257

Mohsin <mohsinrzaidi at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |UNCONFIRMED
         Resolution|INVALID                     |

--- Comment #2 from Mohsin <mohsinrzaidi at gmail dot com> 2011-03-23 19:09:31 UTC ---
(In reply to comment #1)
> this is undefined behaviour, the standard requires that emptyStr2.data() points
> to an array of at least 4 chars

Kindly point me to where this is stated in the standards. I have searched but
have not been able to find any information in this regard.

[Bug libstdc++/48257] std::string::assign() corrupts std::string static data when called on emptyString1 using emptyString2.data()

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48257

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |INVALID

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-03-23 19:39:28 UTC ---
In C++03 see 21.3.5.3 [lib.string.assign] paragraph 6 where 
string::assign(const char* s, size_t n) is defined to return
assign( string(s, n) ) and that constructor:

Constructs an object of class basic_string and determines its initial string
value from the array of charT of length n whose first element is designated by
s, 

In C++0x the requirement is explicit, [string::assign] paragraph 8 says

Requires: s points to an array of at least n elements of charT.

[Bug libstdc++/48257] std::string::assign() corrupts std::string static data when called on emptyString1 using emptyString2.data()

[mohsinrzaidi at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48257

Mohsin <mohsinrzaidi at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |UNCONFIRMED
         Resolution|INVALID                     |

--- Comment #4 from Mohsin <mohsinrzaidi at gmail dot com> 2011-03-24 06:19:52 UTC ---
(In reply to comment #3)
> In C++03 see 21.3.5.3 [lib.string.assign] paragraph 6 where 
> string::assign(const char* s, size_t n) is defined to return
> assign( string(s, n) ) and that constructor:
> 
> Constructs an object of class basic_string and determines its initial string
> value from the array of charT of length n whose first element is designated by
> s, 
> 
> In C++0x the requirement is explicit, [string::assign] paragraph 8 says
> 
> Requires: s points to an array of at least n elements of charT.

I believe this is what you were referring to (from N3242=11-0012):

basic_string& assign(const charT* s, size_type n);
8 Requires: s points to an array of at least n elements of charT.
9 Throws: length_error if n > max_size().
10 Effects: Replaces the string controlled by *this with a string of length n
whose elements are a copy
of those pointed to by s.
11 Returns: *this.

Thanks for pointing this out. However, does libstdc++ throw and error if "n >
max_size()"? I don't think it does.

>From the latest libstdc++ documentation:

<snip>

template<typename _CharT, typename _Traits , typename _Alloc >
basic_string< _CharT, _Traits, _Alloc > & std::basic_string< _CharT, _Traits,
_Alloc >::assign     (     const _CharT *      __s,
        size_type      __n     
    )             

Set value to a C substring.

Parameters:
        s     The C string to use.
        n     Number of characters to use.

Returns:
    Reference to this string.

This function sets the value of this string to the first n characters of s. If
n is is larger than the number of available characters in s, the remainder of s
is used.

Definition at line 261 of file basic_string.tcc.

References std::basic_string< _CharT, _Traits, _Alloc >::size().

</snip>

I don't see this as saying that the string has to have at least n elements OR
that an error will be thrown if it isn't.

>From the latest basic_string code:

<snip>

  template<typename _CharT, typename _Traits, typename _Alloc>
00259     basic_string<_CharT, _Traits, _Alloc>&
00260     basic_string<_CharT, _Traits, _Alloc>::
00261     assign(const _CharT* __s, size_type __n)
00262     {
00263       __glibcxx_requires_string_len(__s, __n);
00264       _M_check_length(this->size(), __n, "basic_string::assign");
00265       if (_M_disjunct(__s) || _M_rep()->_M_is_shared())
00266     return _M_replace_safe(size_type(0), this->size(), __s, __n);
00267       else
00268     {
00269       // Work in-place.
00270       const size_type __pos = __s - _M_data();
00271       if (__pos >= __n)
00272         _M_copy(_M_data(), __s, __n);
00273       else if (__pos)
00274         _M_move(_M_data(), __s, __n);
00275       _M_rep()->_M_set_length_and_sharable(__n);
00276       return *this;
00277     }
00278      }

</snip>

I don't see any errors being thrown. I am not trying to nit-pick but I've
raised this issue because I was affected by it and had to spend several days
isolating it. It would have made my life a whole lot easier if, as you claim,
libstdc++ had thrown an error.

Just a request, but please let the discussion complete before you go ahead and
mark the bug as Resolved-Invalid. I don't mind it being marked invalid, but I
consider it disrespectful for you to not even wait for my response and rushing
to mark it as invalid. Once we're through discussing, I will mark it invalid
myself if necessary.

[Bug libstdc++/48257] std::string::assign() corrupts std::string static data when called on emptyString1 using emptyString2.data()

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48257

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |INVALID

--- Comment #5 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-03-24 08:02:51 UTC ---
Jon is absolutely right. Please, always refer to the Standard for the complete
specifications. And yes exceeding max_size for the total length leads to an
exception being thrown, this is what _M_check_length is for.

[Bug libstdc++/48257] std::string::assign() corrupts std::string static data when called on emptyString1 using emptyString2.data()

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48257

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-03-24 09:29:28 UTC ---
Also, GCC 4.1.2 is ancient and not supported here, you should either report
bugs to the vendor of your version (Red Hat) or refer to a supported version.

As for being disrespectful, the bug was invalid and I know that for a fact,
which is why I didn't wait for you to agree.  You don't need to reopen the bug
in order to post a new comment if you'd like pointers to the relevant pieces of
the standard or the code.

[Bug libstdc++/48257] std::string::assign() corrupts std::string static data when called on emptyString1 using emptyString2.data()

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48257

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-03-24 09:37:03 UTC ---
(In reply to comment #4)
> 
> I don't see any errors being thrown. I am not trying to nit-pick but I've
> raised this issue because I was affected by it and had to spend several days
> isolating it. It would have made my life a whole lot easier if, as you claim,
> libstdc++ had thrown an error.

I never claimed there'd be an exception throw in your original example, I said
it's undefined behaviour.  An exception is thrown if n > max_size()

> Just a request, but please let the discussion complete before you go ahead and
> mark the bug as Resolved-Invalid. I don't mind it being marked invalid, but I
> consider it disrespectful for you to not even wait for my response and rushing
> to mark it as invalid. Once we're through discussing, I will mark it invalid
> myself if necessary.

That's not how it works, most users do not close their own bugs, maintainers
don't want to keep revisiting old bugs to see if the user has decided to accept
the resolution yet.

[Bug libstdc++/48257] std::string::assign() corrupts std::string static data when called on emptyString1 using emptyString2.data()

[mohsinrzaidi at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48257

--- Comment #8 from Mohsin <mohsinrzaidi at gmail dot com> 2011-03-24 09:50:11 UTC ---
(In reply to comment #6)
> Also, GCC 4.1.2 is ancient and not supported here, you should either report
> bugs to the vendor of your version (Red Hat) or refer to a supported version.
> 

In my last comment, I did not post snippets to old code - I posted them off the
latest code.

> As for being disrespectful, the bug was invalid and I know that for a fact,
> which is why I didn't wait for you to agree.  You don't need to reopen the bug
> in order to post a new comment if you'd like pointers to the relevant pieces of
> the standard or the code.

Agreed.

[Bug libstdc++/48257] std::string::assign() corrupts std::string static data when called on emptyString1 using emptyString2.data()

[mohsinrzaidi at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48257

--- Comment #9 from Mohsin <mohsinrzaidi at gmail dot com> 2011-03-24 09:52:56 UTC ---
(In reply to comment #7)
> (In reply to comment #4)
> > 
> > I don't see any errors being thrown. I am not trying to nit-pick but I've
> > raised this issue because I was affected by it and had to spend several days
> > isolating it. It would have made my life a whole lot easier if, as you claim,
> > libstdc++ had thrown an error.
> 
> I never claimed there'd be an exception throw in your original example, I said
> it's undefined behaviour.  An exception is thrown if n > max_size()
> 

Agreed.

> > Just a request, but please let the discussion complete before you go ahead and
> > mark the bug as Resolved-Invalid. I don't mind it being marked invalid, but I
> > consider it disrespectful for you to not even wait for my response and rushing
> > to mark it as invalid. Once we're through discussing, I will mark it invalid
> > myself if necessary.
> 
> That's not how it works, most users do not close their own bugs, maintainers
> don't want to keep revisiting old bugs to see if the user has decided to accept
> the resolution yet.

Agreed.(In reply to comment #7)
> (In reply to comment #4)
> > 
> > I don't see any errors being thrown. I am not trying to nit-pick but I've
> > raised this issue because I was affected by it and had to spend several days
> > isolating it. It would have made my life a whole lot easier if, as you claim,
> > libstdc++ had thrown an error.
> 
> I never claimed there'd be an exception throw in your original example, I said
> it's undefined behaviour.  An exception is thrown if n > max_size()
> 
> > Just a request, but please let the discussion complete before you go ahead and
> > mark the bug as Resolved-Invalid. I don't mind it being marked invalid, but I
> > consider it disrespectful for you to not even wait for my response and rushing
> > to mark it as invalid. Once we're through discussing, I will mark it invalid
> > myself if necessary.
> 
> That's not how it works, most users do not close their own bugs, maintainers
> don't want to keep revisiting old bugs to see if the user has decided to accept
> the resolution yet.

Agreed, thanks.

[Bug libstdc++/48257] std::string::assign() corrupts std::string static data when called on emptyString1 using emptyString2.data()

[mohsinrzaidi at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48257

--- Comment #10 from Mohsin <mohsinrzaidi at gmail dot com> 2011-03-24 10:21:56 UTC ---
Two questions here:

1. Is the behaviour undefined for __n < number of elements in __s?

2. For cases undefined in the specs, do we take steps to ensure robustness? I
still cannot digest that a programmer error could corrupt std::string static
memory.

[Bug libstdc++/48257] std::string::assign() corrupts std::string static data when called on emptyString1 using emptyString2.data()

[mohsinrzaidi at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48257

--- Comment #11 from Mohsin <mohsinrzaidi at gmail dot com> 2011-03-24 10:24:15 UTC ---
(In reply to comment #10)
> Two questions here:
> 
> 1. Is the behaviour undefined for __n < number of elements in __s?
>

Oops! I meant for __n >  number of elements in __s. 

>
> 2. For cases undefined in the specs, do we take steps to ensure robustness? I
> still cannot digest that a programmer error could corrupt std::string static
> memory.

[Bug libstdc++/48257] std::string::assign() corrupts std::string static data when called on emptyString1 using emptyString2.data()

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48257

--- Comment #12 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-03-24 11:27:44 UTC ---
(In reply to comment #11)
> (In reply to comment #10)
> > Two questions here:
> > 
> > 1. Is the behaviour undefined for __n < number of elements in __s?
> >
> 
> Oops! I meant for __n >  number of elements in __s. 

Yes!  The standard defines the behaviour of string::assign when s contains at
least n elements. When that precondition isn't met the definition doesn't apply
i.e. it's undefined.


> > 2. For cases undefined in the specs, do we take steps to ensure robustness? 

Where possible, yes, that's what -D_GLIBCXX_DEBUG tries to do.  But in general
it's not possible to verify that the supplied string meets the required length.
Given a const char*, how do you tell if it points to an array of at least n
chars?  You can't.

It would be possible to check that the supplied const char* is not the shared
static "empty string" representation, but that would add overhead and still
wouldn't prevent similar errors like:

  string s("oops", 999);

> > I
> > still cannot digest that a programmer error could corrupt std::string static
> > memory.

Really?  A sufficiently malicious/careless programmer can corrupt pretty much
anything!

In any case, if you try your original example with current releases it doesn't
print '4'

[Bug libstdc++/48257] std::string::assign() corrupts std::string static data when called on emptyString1 using emptyString2.data()

[mohsinrzaidi at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48257

--- Comment #13 from Mohsin <mohsinrzaidi at gmail dot com> 2011-03-25 05:48:21 UTC ---
(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > Two questions here:
> > > 
> > > 1. Is the behaviour undefined for __n < number of elements in __s?
> > >
> > 
> > Oops! I meant for __n >  number of elements in __s. 
> Yes!  The standard defines the behaviour of string::assign when s contains at
> least n elements. When that precondition isn't met the definition doesn't apply
> i.e. it's undefined.

Sorry for the confusion here Jon. I meant to ask if the specs define what the
behaviour should be if __s *does not* contain __n elements.

> > > 2. For cases undefined in the specs, do we take steps to ensure robustness? 
> Where possible, yes, that's what -D_GLIBCXX_DEBUG tries to do.  But in general
> it's not possible to verify that the supplied string meets the required length.
> Given a const char*, how do you tell if it points to an array of at least n
> chars?  You can't.

We could always look for the null-termination :) But like you say below, this
would add overhead and still not handle all cases.

> It would be possible to check that the supplied const char* is not the shared
> static "empty string" representation, but that would add overhead and still
> wouldn't prevent similar errors like:
>   string s("oops", 999);

Understood.

> > > I
> > > still cannot digest that a programmer error could corrupt std::string static
> > > memory.
> Really?  A sufficiently malicious/careless programmer can corrupt pretty much
> anything!

Hehe, understood.

> In any case, if you try your original example with current releases it doesn't
> print '4'

Great! I'll try that.

Thanks for bearing with my impetuous behaviour. I know it mustn't be easy
maintaining such a widely used code base.

[Bug libstdc++/48257] std::string::assign() corrupts std::string static data when called on emptyString1 using emptyString2.data()

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48257

--- Comment #14 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-03-25 09:54:25 UTC ---
(In reply to comment #13)
> 
> Sorry for the confusion here Jon. I meant to ask if the specs define what the
> behaviour should be if __s *does not* contain __n elements.

No, it's undefined.

If the standard said what should happen, it would be defined.

There's no way to tell if it doesn't contain the necessary number of elements.

> > > > 2. For cases undefined in the specs, do we take steps to ensure robustness? 
> > Where possible, yes, that's what -D_GLIBCXX_DEBUG tries to do.  But in general
> > it's not possible to verify that the supplied string meets the required length.
> > Given a const char*, how do you tell if it points to an array of at least n
> > chars?  You can't.
> 
> We could always look for the null-termination :) But like you say below, this
> would add overhead and still not handle all cases.

No, you most definitely cannot. Not all arrays of characters are terminated:

char s[] = { 'a', 'b', 'c' };
std::string str(s, 3);

Attempting to check for a null terminator here will walk off the end of the
array into random pages of memory, possibly causing a segfault.

You can check in some specific cases, but not for all cases. As I said, in
general you can't detect the error, so it's the programmer's responsibility to
meet the preconditions of the functions they call.

It's really no different to calling memcpy() with invalid arguments and
overwriting any region of memory.