Boost C++ Libraries: Ticket #1849: Deserialization of std::string overwrites non-copied contents.

Robert Ramey — Sun, 08 Jun 2008 04:41:04 GMT

add

#include <boost/serialization/string.hpp>

and run your test again.

Robert Ramey

status changed

Robert Ramey — Sun, 08 Jun 2008 04:41:17 GMT

status new → assigned

dev@… — Mon, 09 Jun 2008 10:11:51 GMT

Added the string.hpp and got the same result.

Using g++ version 4.3.1 also got the same result. (don't know if the 4.3.1 stdlib from gcc-snapshot is automatically used when switching the compiler in kdevelop)

Adding a character to "BarST" -> "BarSTx" leads to _no_ overwriting. Removing a character from "BarST" -> "BarS" leads to _no_ overwriting.

So this problem only exists when the _lengths of the strings match_ like they did in my program when the bug first occured to me.

Robert Ramey — Mon, 09 Jun 2008 19:20:40 GMT

I presume that "to _no_ overwriting" refers to an exception.

This looks very much like a bug in the standard library implementation being used. I'll have look at the serialization string implemention. I wouldn't want to have to create a new string on all platforms just to work around this. Keep me posted if you find a good (portable?) fix for this.

Robert Ramey

anonymous — Mon, 09 Jun 2008 21:35:34 GMT

No, there was no exception thrown or anything.

It must have to do something with the way the deserialization writes into that string.

If the lengths match, it doesn't allocate new memory and also doesn't check if the memory referred to belongs to another object and should be copied on write.

Also because the original string was a const, the constness must have been cast away somewhere.

I don't understand this with the actual implementation because of that in string.hpp:

...
BOOST_CLASS_IMPLEMENTATION(std::string, boost::serialization::primitive_type)
#ifndef BOOST_NO_STD_WSTRING
BOOST_CLASS_IMPLEMENTATION(std::wstring, boost::serialization::primitive_type)
#endif
// left over from a previous incarnation - strings are now always primitive types
#if 0
... old implementation

as far as i came in the code, the old implementation does the reasonable thing (call .clear(), .reserve() and .push_back()).

But i don't understand where the loading with the new implementation is actually happening.

What does that "primitive_type" mean when loading an object? Maybe a string sometimes ain't as primitive as expected.

The obvious fix would be to use the old implementation again, but i don't know if that raises other problems.

Maybe the problem is in the gnu-stl, maybe someone else can reconfirm it with other compilers/platforms because i only have access to the gnu compilers and the error occurs on linux and mingw. But let alone that it happens on multiple versions of gnu there should be great interest of this bug to go away.

Robert Ramey — Tue, 10 Jun 2008 01:50:49 GMT

as far as serialization is concerned std::string is primitive - that is its implemented as code intrinsic to the library - look at the documentation regarding serialization traits. The implementation has been move to text_iarchive_impl.ipp

template<class Archive>
BOOST_ARCHIVE_DECL(void)
text_iarchive_impl<Archive>::load(std::string &s)
{
    std::size_t size;
    * this->This() >> size;
    // skip separating space
    is.get();
    // borland de-allocator fixup
    #if BOOST_WORKAROUND(_RWSTD_VER, BOOST_TESTED_AT(20101))
    if(NULL != s.data())
    #endif
        s.resize(size);
    if(0 < size)
        is.read(&(*s.begin()), size);
}

We ARE breaking the rules here. I think we had a discussion regarding this point some time ago. The concensus was that this hack was worth the risk to improve performance. Performance is a BIG issue - but of course the program has to work - I'll look into what to do about this. My first thought is another hack that it clear out the string if the sizes are the same.

Robert Ramey

anonymous — Tue, 10 Jun 2008 04:46:52 GMT

You're pointing to the right location.

Obviously your code is correct with:

is.read(&(*s.begin()), size);

The call to .begin() causes the string to copy and .data() changes locations.

But in 1.35.0 release this line is executed.

is.read(const_cast<char *>(s.data()), size);

.data() obviously doesn't copy the string and there's also the bad cast.

So the issue is fixed in the next release.

status, milestone changed; resolution set

Robert Ramey — Tue, 10 Jun 2008 05:23:23 GMT

status assigned → closed
resolution → fixed
milestone Boost 1.35.1 → Boost 1.36.0