msgpack / msgpack-c

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

msgpack 3.0.1 encodes 16 byte EXT as array and not fixext #712

Closed drrlvn closed 5 years ago

drrlvn commented 6 years ago

I have an adaptor for boost::uuids::uuid that encodes them as EXT that worked well with version 2.1.5.

When I try to upgrade to msgpack-c 3.0.1 the UUID is encoded differently: instead of being encoded as a fixext 16 (0xd8, type and 16 bytes) it's encoded as a byte array (0xc7, length which is 17 now and then type and data).

This is the relevant part of my code in case there's a bug in there somewhere:

template<>
struct object_with_zone<boost::uuids::uuid> {
    void operator()(msgpack::object::with_zone & o, const boost::uuids::uuid & v) const
    {
        o.type = msgpack::type::EXT;
        o.via.ext.size = sizeof(v);
        auto p = static_cast<char *>(o.zone.allocate_align(sizeof(v) + 1));
        p[0] = _MSGPACK_TYPE_UUID;
        std::memcpy(&p[1], v.data, sizeof(v));
        o.via.ext.ptr = p;
    }
};
redboltz commented 6 years ago

It seems that

        p[0] = _MSGPACK_TYPE_UUID;
        auto p = static_cast<char *>(o.zone.allocate_align(sizeof(v) + 1));

should be

        auto p = static_cast<char *>(o.zone.allocate_align(sizeof(v) + 1));
        p[0] = _MSGPACK_TYPE_UUID;

I guess that it is just a kind of typo and not the reason of behavior you experienced.

Could you tell me the actual value of _MSGPACK_TYPE_UUID ? And sizeof(v) ?

drrlvn commented 6 years ago

I seem to have pasted the code wrong, sorry for that. Should be fixed now.

static constexpr auto _MSGPACK_TYPE_UUID = 0;

Size of v is 16: https://godbolt.org/g/aFNgz7

redboltz commented 6 years ago

Your code seems to be creating msgpack::object. But you said about encoding. Maybe I need to see pack template.

drrlvn commented 6 years ago

Here is the pack template:

template<>
struct pack<boost::uuids::uuid> {
    template<typename Stream>
    msgpack::packer<Stream> & operator()(msgpack::packer<Stream> & o, const boost::uuids::uuid & v) const
    {
        o.pack_ext(sizeof(v), _MSGPACK_TYPE_UUID);
        o.pack_ext_body(reinterpret_cast<const char *>(v.data), sizeof(v));
        return o;
    }
};
redboltz commented 6 years ago

I've tested using the following code:

https://wandbox.org/permlink/y8xKyiNP8bOwJz2t

I think that msgpack-c works fine.

I took a look your pack template. I think that the code looks good. In order to check the behavior, adding debug print or using debugger.

drrlvn commented 6 years ago

I'm not sure why but adding std::abort() in the pack template didn't cause a crash but adding it in the object_with_zone template did, so I think that's what is being called and where the problem is.

I receive the buffer in python where I see it's encoded with 0xc7.

grmcdorman commented 5 years ago

May be related to just-submitted issue 754; msgpack::pack(s, object) where object is of type msgpack::object containing ext data incorrectly packs the object.

drrlvn commented 5 years ago

@grmcdorman Thank you for finding that!

@redboltz I see that #754 is fixed but no release have been made since. Could you please make a new release so that I can verify it fixes my problem?

redboltz commented 5 years ago

@drrlvn , I will start the new release process soon.

drrlvn commented 5 years ago

Fixed in 3.2.0, thanks @grmcdorman and @redboltz!