msgpack::clone() never creates a new zone

[thpica]

While using msgpack::clone() I had pretty bad issues (std::bad_cast) when cloning any object that would be allocated to the zone.

I think I figured why:

1. When creating a aligned_zone_size_visitor, s will be copied to a private property m_size:

   struct aligned_zone_size_visitor {
       explicit aligned_zone_size_visitor(std::size_t s)
           :m_size(s) {}
       ...
   private:
       std::size_t m_size;
   };   

   https://github.com/msgpack/msgpack-c/blob/master/include/msgpack/v1/object.hpp#L529-L600

2. Because the original reference to s is not retained, s will stay at its initial value and msgpack::aligned_zone_size() always returns 0:

   inline std::size_t aligned_zone_size(msgpack::object const& obj) {
       std::size_t s = 0;
       aligned_zone_size_visitor vis(s);
       msgpack::object_parser(obj).parse(vis);
       return s;
   }

   https://github.com/msgpack/msgpack-c/blob/master/include/msgpack/v1/object.hpp#L602-L607

3. msgpack::clone() only instantiate a new zone if msgpack::aligned_zone_size() > 0 so it will never create a new zone:

   /// clone object
   /**
    * Clone (deep copy) object.
    * The copied object is located on newly allocated zone.
    * @param obj copy source object
    *
    * @return object_handle that holds deep copied object and zone.
    */
   inline object_handle clone(msgpack::object const& obj) {
       std::size_t size = msgpack::aligned_zone_size(obj);
       msgpack::unique_ptr<msgpack::zone> z(size == 0 ? MSGPACK_NULLPTR : new msgpack::zone(size));
       msgpack::object newobj = z.get() ? msgpack::object(obj, *z) : obj;
       return object_handle(newobj, msgpack::move(z));
   }

   https://github.com/msgpack/msgpack-c/blob/master/include/msgpack/v1/object.hpp#L609-L622

Is this an actual bug or a feature I misunderstood?

[redboltz]

@thpica , thank you for reporting the issue. It seems that a bug. I will investigate it.

[redboltz]

There are tests for clone including the types allocated on the zone e.g.) ARRAY, STR

msgpack-c/test/object.cpp

Lines 349 to 408 in 0a65c44

	TEST(object, clone_int)
	{
	int v = 0;
	msgpack::object obj(v);
	std::size_t sz1 = msgpack::aligned_zone_size(obj);
	msgpack::object_handle h = msgpack::clone(obj);
	EXPECT_EQ(h.get(), obj);
	EXPECT_EQ(sz1, msgpack::aligned_zone_size(h.get()));
	h = msgpack::clone(obj);
	EXPECT_EQ(h.get(), obj);
	EXPECT_EQ(sz1, msgpack::aligned_zone_size(h.get()));
	}
	TEST(object, clone_str)
	{
	msgpack::zone z;
	std::string v = "123456789";
	msgpack::object obj(v, z);
	std::size_t sz1 = msgpack::aligned_zone_size(obj);
	msgpack::object_handle h = msgpack::clone(obj);
	EXPECT_EQ(h.get(), obj);
	EXPECT_EQ(sz1, msgpack::aligned_zone_size(h.get()));
	h = msgpack::clone(obj);
	EXPECT_EQ(h.get(), obj);
	EXPECT_EQ(sz1, msgpack::aligned_zone_size(h.get()));
	}
	TEST(object, clone_bin)
	{
	msgpack::zone z;
	std::vector<char> v;
	v.push_back('A');
	v.push_back('B');
	v.push_back('C');
	msgpack::object obj(v, z);
	std::size_t sz1 = msgpack::aligned_zone_size(obj);
	msgpack::object_handle h = msgpack::clone(obj);
	EXPECT_EQ(h.get(), obj);
	EXPECT_EQ(sz1, msgpack::aligned_zone_size(h.get()));
	h = msgpack::clone(obj);
	EXPECT_EQ(h.get(), obj);
	EXPECT_EQ(sz1, msgpack::aligned_zone_size(h.get()));
	}
	TEST(object, clone_array)
	{
	msgpack::zone z;
	std::vector<int> v;
	v.push_back(1);
	v.push_back(2);
	v.push_back(3);
	msgpack::object obj(v, z);
	std::size_t sz1 = msgpack::aligned_zone_size(obj);
	msgpack::object_handle h = msgpack::clone(obj);
	EXPECT_EQ(h.get(), obj);
	EXPECT_EQ(sz1, msgpack::aligned_zone_size(h.get()));
	h = msgpack::clone(obj);
	EXPECT_EQ(h.get(), obj);
	EXPECT_EQ(sz1, msgpack::aligned_zone_size(h.get()));
	}

But checking might not be enough.

[thpica]

In the tests, reassigning the original zone to an empty one after cloning should highlight the issue.

[redboltz]

I don't check the code deeply, so far. But I guess that the copy is shallow, so the pointer of ARRAY, STR, BIN, and EXT point to the address in the old zone. If it does, I guess that writing deep copy visitor for msgpack::object is a solution.

[redboltz]

@thpica , I fixed the issue and tests.

As you mentioned, the problem was at aligned_zone_size_visitor.

struct aligned_zone_size_visitor {
    explicit aligned_zone_size_visitor(std::size_t s)
        :m_size(s) {}
    ...
private:
    std::size_t m_size;
};

size should be passed and stored by reference as follows:

struct aligned_zone_size_visitor {
    explicit aligned_zone_size_visitor(std::size_t& s)
        :m_size(s) {}
    ...
private:
    std::size_t& m_size;
};

I don't check the code deeply, so far. But I guess that the copy is shallow, so the pointer of ARRAY, STR, BIN, and EXT point to the address in the old zone. If it does, I guess that writing deep copy visitor for msgpack::object is a solution.

My guess above is wrong. I already have implemented deep copy version visitor and it is called from clone(). So I don't need to fix about that.

Anyway, I sent the PR #764 to fix the issue. Could you try this?

[redboltz]

Fixed by #764.