[Bug libstdc++/54392] New: [4.6/4.7/4.8 Regression] std::string::append() fails to update length

[Bug libstdc++/54392] New: [4.6/4.7/4.8 Regression] std::string::append() fails to update length

[redi at gcc dot gnu.org]

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

             Bug #: 54392
           Summary: [4.6/4.7/4.8 Regression] std::string::append() fails
                    to update length
    Classification: Unclassified
           Product: gcc
           Version: 4.6.3
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: redi@gcc.gnu.org


#include <string>
#include <cassert>

int main()
{
    std::string s1, s2;
    s1.assign(s2.c_str(), 1);
    assert( s1.size() == 1 );
}

This test passes with G++ 4.4, but in 4.5 and later string::assign(const char*,
size_type) doesn't mutate the string but tries to work in-place on the empty
rep, then tries (and fails) to set the length of the empty rep to 1.

[Bug libstdc++/54392] [4.6/4.7/4.8 Regression] std::string::append() fails to update length

[hjl.tools at gmail dot com]

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

H.J. Lu <hjl.tools at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-08-28
                 CC|                            |paolo.carlini at oracle dot
                   |                            |com
     Ever Confirmed|0                           |1

--- Comment #1 from H.J. Lu <hjl.tools at gmail dot com> 2012-08-28 18:03:34 UTC ---
It is caused by revision 148850:

http://gcc.gnu.org/ml/gcc-cvs/2009-06/msg00833.html

[Bug libstdc++/54392] [4.6/4.7/4.8 Regression] std::string::append() fails to update length

[redi at gcc dot gnu.org]

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

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-08-28 18:10:13 UTC ---
(In reply to comment #1)
> It is caused by revision 148850:
> 
> http://gcc.gnu.org/ml/gcc-cvs/2009-06/msg00833.html

That patch fixed a race condition, and it *is* correct to prevent writes to the
empty shared rep. If we reverted that patch we'd reintroduce the race, and
still erroneously write to the empty rep.

The underlying bug is in string::append() where it tries to write to it in the
first place. I think as well as checking _M_disjunct(__s) and _M_is_shared() we
should check whether we have the empty rep.

[Bug libstdc++/54392] [4.6/4.7/4.8 Regression] std::string::assign() fails to update length

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[4.6/4.7/4.8 Regression]    |[4.6/4.7/4.8 Regression]
                   |std::string::append() fails |std::string::assign() fails
                   |to update length            |to update length

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-08-29 07:35:36 UTC ---
For some reason my fingers keep typing append when I mean assign.

s/append/assign/g

[Bug libstdc++/54392] [4.6/4.7/4.8 Regression] std::string::assign() fails to update length

[michael.haubenwallner at salomon dot at]

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

--- Comment #4 from Michael Haubenwallner <michael.haubenwallner at salomon dot at> 2012-08-29 10:50:22 UTC ---
(In reply to comment #0)

Extending the testcase shows even more bad behavior in 4.4.3 and earlier:

#include <string>
#include <cassert>

int main()
{
    std::string s1, s2;
    s1.assign(s2.c_str(), 1);
    assert( s1.size() == 1 );

    std::string s3;
    assert( s3.size() == 0 ); // up to gcc-4.4.3 fail here
}

[Bug libstdc++/54392] [4.6/4.7/4.8 Regression] std::string::assign() fails to update length

[paolo.carlini at oracle dot com]

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

--- Comment #5 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-08-29 14:15:58 UTC ---
Jon, didn't we somehow discuss this issue already, in a slightly different form
(the issue of course was less evident pre that fix)? Thus, if I remember
correctly what I was thinking at that time, a good general and safe fix would
be tweaking a bit _M_disjunct, making it more careful so to speak, thus
considering the address of the source as overlapping also if it corresponds to
the '\0' at the end of destination. The philosophy in other terms is
"preserving" the whole internal buffer, thus '\0' included. Matter of adding a
+ 1 somewhere, in other terms ;) If my analysis is correct, the fix would work
as a special case for the test here involving empty reps, and I think it's the
safe fix we want for the current std::string implementation at this stage.

[Bug libstdc++/54392] [4.6/4.7/4.8 Regression] std::string::assign() fails to update length

[paolo.carlini at oracle dot com]

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

--- Comment #6 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-08-29 14:47:28 UTC ---
Nope, I checked and _M_disjunct seems already Ok. The issue is actually with
the logic in assign when _M_disjunct is false: it assumes we are simply
overwriting things and no memory allocation needed.

[Bug libstdc++/54392] [4.6/4.7/4.8 Regression] std::string::assign() fails to update length

[paolo.carlini at oracle dot com]

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

--- Comment #7 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-08-29 15:13:57 UTC ---
Thus, I guess the right thing to do is sort-of the other way around of what I
was thinking: change _M_disjunct in such a way that when the destination is the
empty rep it always returns true, thus we first allocate memory for the
destination and then copy the '\0' to it and everything is fine (nothing bad
can happen to the source in this special case because the empty rep source
isn't in dynamically allocated memory, thus isn't deallocated, it stays in
memory forever).

[Bug libstdc++/54392] [4.6/4.7/4.8 Regression] std::string::assign() fails to update length

[michael.haubenwallner at salomon dot at]

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

--- Comment #8 from Michael Haubenwallner <michael.haubenwallner at salomon dot at> 2012-08-29 15:20:50 UTC ---
Actually, valgrind does show an "Invalid write of size 1" for this testcase,
unrelated to the default string at all:

{
  std::string s1 = "a";
  s1.assign(s1.c_str(), 2);
}

So I'm proposing this fix here:

--- basic_string.tcc.orig       2012-08-29 17:12:53.349220855 +0200
+++ basic_string.tcc    2012-08-29 17:13:37.021326394 +0200
@@ -262,7 +262,7 @@
     {
       __glibcxx_requires_string_len(__s, __n);
       _M_check_length(this->size(), __n, "basic_string::assign");
-      if (_M_disjunct(__s) || _M_rep()->_M_is_shared())
+      if (_M_disjunct(__s) || _M_rep()->_M_is_shared() || this->size() < __n)
        return _M_replace_safe(size_type(0), this->size(), __s, __n);
       else
        {

[Bug libstdc++/54392] [4.6/4.7/4.8 Regression] std::string::assign() fails to update length

[paolo.carlini at oracle dot com]

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

--- Comment #9 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-08-29 15:28:45 UTC ---
Ok, I see. The problem is that when the source is inside the destination, we
may be copying one more char, the final '\0', than the current size and
overflow the allocated memory. We need reallocation in this case too. But we
can't just follow the path of _M_replace_safe, because it deallocates the
source! This case is harder to fix than the empty rep special case.

[Bug libstdc++/54392] [4.6/4.7/4.8 Regression] std::string::assign() fails to update length

[paolo.carlini at oracle dot com]

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

--- Comment #10 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-08-29 15:32:29 UTC ---
Note that the last snippet definitely caused problems even before the recent
changes. Can't be a regression. Given the current status of the implementation
I'm not sure if is worth fixing right now.

[Bug libstdc++/54392] [4.6/4.7/4.8 Regression] std::string::assign() fails to update length

[paolo.carlini at oracle dot com]

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

--- Comment #11 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-08-29 16:20:43 UTC ---
I'm sorry, you are right - it's been a while since the last time I looked into
this code - going that way of the conditional is Ok, because the memory
corresponding to the source (which coincide with that of the destination
string) is deallocated only after all the chars are copied. Good. Thus it means
that in general, we want, again to change _M_disjoint not to check that the
destination is the empty rep, but precisely for this more general case, that
is, the source begin is the same as the this->begin() beginning, and the size
is this->size() + 1 and return true. We have to add an overload, because the
current one doesn't take the size of the source. I think this is it.

[Bug libstdc++/54392] [4.6/4.7/4.8 Regression] std::string::assign() fails to update length

[paolo.carlini at oracle dot com]

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

--- Comment #12 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-08-29 16:53:02 UTC ---
Nope, unfortunately I remembered correctly: if we call _M_replace_safe for
these cases the new chars are copied at the very end. At that point, the
destination string is already reallocated but in these cases it's also the
source string and __s doesn't point anymore to it. _M_replace_safe is exactly
for when __s remains a valid address for the source chars from beginning to end
of the whole operation.

[Bug libstdc++/54392] [4.6/4.7/4.8 Regression] std::string::assign() fails to update length

[paolo.carlini at oracle dot com]

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

--- Comment #13 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-08-29 23:21:25 UTC ---
In fact, considering this issue a regression seems a bit of an over stretch to
me: the empty rep case used to work essentially be chance, it's just a very
special case (... of a very special case, which nobody bothered to report about
in some 10 years). If you ask me, I would not handle this for std::string and
fix it instead in vstring (where, by the way, a fix is straightforward and
localized to _M_replace, I'll take care of that ASAP)

[Bug libstdc++/54392] [4.6/4.7/4.8 Regression] std::string::assign() fails to update length

[michael.haubenwallner at salomon dot at]

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

--- Comment #14 from Michael Haubenwallner <michael.haubenwallner at salomon dot at> 2012-08-30 07:33:16 UTC ---
Indeed, the old buffer is freed before being copied.

Yep, this isn't a regression. In fact, with 4.4.3 it was the /empty string/
having the size of 1 in the comment#0 testcase, so bug#40518 actually improved
things here.

However, doesn't any operation with (const _CharT*, size_type) as input
parameter potentially have a problem with ( own.c_str(), own.size()+1 ) ?

[Bug libstdc++/54392] [4.6/4.7/4.8 Regression] std::string::assign() fails to update length

[paolo.carlini at oracle dot com]

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

--- Comment #15 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-08-31 16:52:16 UTC ---
Yes it does. However considering that nobody noticed that for 10 years and that
the std::string implementation is near its end of life anyway (as I said, in
the new one the fix is straightforward and I'll take care of it in a few days)
I don't consider this issue very serious, frankly.

[Bug libstdc++/54392] [4.6/4.7/4.8 Regression] std::string::assign() fails to update length

[rguenth at gcc dot gnu.org]

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

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.6.4

[Bug libstdc++/54392] [4.6/4.7/4.8 Regression] std::string::assign() fails to update length

[rguenth at gcc dot gnu.org]

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

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
           Priority|P3                          |P2

[Bug libstdc++/54392] [4.6/4.7/4.8 Regression] std::string::assign() fails to update length

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #16 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-11 15:57:40 UTC ---
So, what are we going to do with this?  Defer for 4.9, or fix for 4.8?

[Bug libstdc++/54392] [4.6/4.7/4.8 Regression] std::string::assign() fails to update length

[paolo.carlini at oracle dot com]

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

--- Comment #17 from Paolo Carlini <paolo.carlini at oracle dot com> 2013-01-11 16:01:28 UTC ---
In my opinion, this isn't really fixable in a generic and decently neat way in
the current implementation, that is in std::string. I still mean to fix it in
__versa_string for 4.8 too if you like, when I'm back from my 1-week vacations.
There a fix is pretty simple.

[Bug libstdc++/54392] [4.6/4.7/4.8 Regression] std::string::assign() fails to update length

[paolo.carlini at oracle dot com]

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

--- Comment #18 from Paolo Carlini <paolo.carlini at oracle dot com> 2013-01-11 16:03:23 UTC ---
PS: for 4.9, I think we essentially agree that we want to roll in anyway
__versa_string, break the ABI, etc. We also exchanged some ideas about that
with Jason, off lists.

[Bug libstdc++/54392] [4.6/4.7/4.8 Regression] std::string::assign() fails to update length

[redi at gcc dot gnu.org]

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

--- Comment #19 from Jonathan Wakely <redi at gcc dot gnu.org> 2013-01-11 18:58:53 UTC ---
Agreed. I don't think we'll ever fix this in the current std::string code TBH

[Bug libstdc++/54392] [4.6/4.7/4.8 Regression] std::string::assign() fails to update length

[paolo.carlini at oracle dot com]

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

--- Comment #20 from Paolo Carlini <paolo.carlini at oracle dot com> 2013-01-31 10:05:35 UTC ---
In fact, ext/vstring seems *already* Ok. The reason being that before checking
whether we can work in place we check whether a reallocation is needed, thus in
all the cases at issue here _M_replace immediately calls _M_mutate, which does
the right thing. I'll double check anyway. Also, I'll see again-again whether I
can figure out something decently neat for current basic_string.

[Bug libstdc++/54392] [4.7/4.8/4.9 Regression] std::string::assign() fails to update length

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.6.4                       |4.7.4

--- Comment #21 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-04-12 15:16:49 UTC ---
GCC 4.6.4 has been released and the branch has been closed.

[Bug libstdc++/54392] [4.7/4.8/4.9 Regression] std::string::assign() fails to update length

[jakub at gcc dot gnu.org]

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

--- Comment #22 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
So 5.0 material, given that we are not going to change std::string in 4.9?

[Bug libstdc++/54392] [4.7/4.8/4.9 Regression] std::string::assign() fails to update length

[redi at gcc dot gnu.org]

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

--- Comment #23 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Yes, definitely. It should get fixed automatically by switching to the new
std::string impl.

[Bug libstdc++/54392] [4.7/4.8/4.9/4.10 Regression] std::string::assign() fails to update length

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54392

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.7.4                       |4.8.4

--- Comment #24 from Richard Biener <rguenth at gcc dot gnu.org> ---
The 4.7 branch is being closed, moving target milestone to 4.8.4.

[Bug libstdc++/54392] [4.8/4.9/5 Regression] std::string::assign() fails to update length

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54392

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |24882
   Target Milestone|4.8.4                       |5.0

[Bug libstdc++/54392] [4.8/4.9/5 Regression] std::string::assign() fails to update length

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54392

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #25 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Fixed for GCC 5 (when using the new string ABI)