msgpack / msgpack-c

MessagePack implementation for C and C++ / msgpack.org[C/C++]
Other
3.01k stars 875 forks source link

msgpack incorrectly deserializes a std::map to std::multimap #1105

Closed DUOLabs333 closed 7 months ago

DUOLabs333 commented 7 months ago

Describe the bug A clear and concise description of what the bug is. msgpack-c or msgpack-cxx version --- HEAD on Git

To Reproduce Initialize a msgpack::type::variant, and set it to std::map<variant, variant> Serialize it, then unserialize it. Try to access the original map with .as_map(), and see the crash.

Expected behavior I expected .as_map to return the map

Fix The issue is in include/msgpack/v1/adaptor/boost/msgpack_variant.hpp, specifically, here:

case type::MAP:
            return o.as<std::multimap<type::basic_variant<STR, BIN, EXT>, type::basic_variant<STR, BIN, EXT> > >();

There needs to be some way of differentiating multimaps from maps.

redboltz commented 7 months ago

msgpack::type::variant only supports std::multimap<variant, variant> for converting from msgpack::object. Here is document (sorry about missing link) https://github.com/msgpack/msgpack-c/wiki/v2_0_cpp_variant

However, you actually assigned std::map<variant, variant> to variant, and it packed to MessagePack MAP. I guess that it is the confusing part. I don't remember in detail history but std::map<variant, variant> packing only support is added by user's request. So, it is supprted silently. It should be documented.

Why convert to std::map<variant, variant> is not supported ?

The answer is MessagePack doesn't have a way to preserve programming language's type information. In addition, MessagePack format allows duplicate keys.

If you want to extract asstd::map<variant, variant>, you need to convert to std::multimap<variant, variant> once, and then, create your std::map<variant, variant> from std::multimap<variant, variant>. If key conflict found, overwriting or ignoring new one or throwing expection is applications decision.

Terminology note

DUOLabs333 commented 7 months ago

I made a fork that just changed the conversion from type::MAP to std::map, and it seemed to work out well.