container icon indicating copy to clipboard operation
container copied to clipboard

[1.85] flat_map/vector crashes on appends (memory corruption)

Open Lastique opened this issue 10 months ago • 3 comments

Consider this test code:

#include <cstdint>
#include <string>
#include <boost/container/flat_map.hpp>

typedef boost::container::flat_map< std::uint8_t, std::string > my_map;

void add_element(std::string& str, char elem)
{
    str.push_back(elem);
}

int main()
{
    my_map m;
    add_element(m[static_cast< std::uint8_t >(96u)], 'a');
    add_element(m[static_cast< std::uint8_t >(102u)], 'a');
    add_element(m[static_cast< std::uint8_t >(104u)], 'a');
}
g++ -O2 -I. -std=gnu++17 -o test_flat_map test_flat_map.cpp

This crashes with:

free(): invalid pointer
Aborted (core dumped)

valgrind also shows a number of invalid memory accesses, see the attached log:

test_flat_map.log

This code works correctly in Boost 1.84.0. It also doesn't crash if compiled with -O0.

gcc 11.4.0, Kubuntu 22.04.

Lastique avatar Apr 19 '24 20:04 Lastique

Bisect shows that the first bad commit is 1a4a205ea6ef7b4e67a2faab7c7d745711807695. Reverting this commit on 1.85 (with resolved conflicts) fixes the crash.

container_revert_inline_conversion.patch.gz

Lastique avatar Apr 19 '24 20:04 Lastique

Thanks for the report!

I think the issue is produced because flat_map used UB in the implementation, long ago, when C++03 compilers had no movable std::pair type and the class was designed to achieve move emulation in those compilers. The following commit should fix the issue, your example seems to work after the commit, but I didn't want to close the issue without having your feedback:

https://github.com/boostorg/container/commit/20ad12f20e661978e90dc7f36d8ab8ac05e5a5a9

igaztanaga avatar Apr 28 '24 21:04 igaztanaga

Thanks, the commit does fix the problem.

It is probably worth attaching this patch to the 1.85.0 release notes.

Lastique avatar Apr 29 '24 13:04 Lastique

Closing this issue as fixed.

igaztanaga avatar May 23 '24 21:05 igaztanaga

@igaztanaga How come this was not caught by a unit test?

haferburg avatar Jul 09 '24 07:07 haferburg

I think it was optimization-level and context code (inlining decisions) dependent and Boost.Container tests only failed consistently with latest GCC versions (>=11, I think). MSVC and clang showed no failure.

Once latest GCC versions were mainstream, boost.Container tests were failing and I figure out that UB could be the cause.

igaztanaga avatar Jul 10 '24 08:07 igaztanaga

FYI Similar bug (UB => -O2 code elision) occurs in this example, ~~still in process of checking if the mentioned commit fixes the problem~~ the mentioned commit fixes this (near-)minimal reproducible example: https://gcc.godbolt.org/z/qM5befxeY

#include <boost/container/flat_map.hpp>

struct A {
    int i;
    auto operator<=>(const A&) const = default;
};

int main() {
    boost::container::flat_map<A, A> map;
    map.emplace(A{1}, A{});
    for (auto const& [_1, _2] : map)
        throw 123; // this line is never executed, UB!
}

Might be worth getting a patch posted on https://www.boost.org/patches/ and https://www.boost.org/users/history/version_1_85_0.html as a known issue (e.g. how it was done for the 1.83.0 and earlier releases).

E: So, fun. This wasn't caught internally to my org because

13steinj avatar Jul 19 '24 20:07 13steinj

Does boost-1.86.0-beta1 include the fix for this?

justusranvier avatar Jul 26 '24 18:07 justusranvier

Yes. Release notes were not updated for this beta but the fix is there.

igaztanaga avatar Jul 26 '24 22:07 igaztanaga

Trying to build a patch for 1.85 I could not reproduce the bug with Boost.Build. It turns out that Build defines -DNDEBUG and that avoids @Lastique's example's crash...

Anyway, here's the first patch for 1.85 if you want to test it:

0001-container-fix-flat_map.patch

igaztanaga avatar Jul 27 '24 22:07 igaztanaga

The patch is better placed on the website and linked in 1.85 release notes. See an example of a PR doing this here.

Lastique avatar Jul 30 '24 09:07 Lastique

Many thanks for your example Andrey. PR:

https://github.com/boostorg/website/pull/862

igaztanaga avatar Aug 01 '24 21:08 igaztanaga