MSGPACK_DEFINE() with char arrays silently does the wrong thing

[vadz]

Consider the following example program (please ignore the unsafe use of strncpy() etc) using msgpack 1.3.0:

#include <msgpack.hpp>

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

struct S {
    explicit S(const char* n)
    {
        strncpy(name, n, sizeof(name));
    }

    char name[10];

    MSGPACK_DEFINE(name);
};

int main()
{
    S s("foo");

    try {
        msgpack::sbuffer sbuf;
        msgpack::pack(sbuf, s);

        msgpack::unpacked ret;
        msgpack::unpack(ret, sbuf.data(), sbuf.size());

        S s2("");
        ret.get().convert(&s2);

        if (strcmp(s.name, s2.name) == 0)
            return EXIT_SUCCESS;
    } catch (std::exception& e) {
        fprintf(stderr, "Exception: %s\n", e.what());
    }

    return EXIT_FAILURE;
}

Compiling and running it unexpectedly results in an exit failure with "bad cast" exception.

Debugging it I see that what happens is that s is correctly serialized because there is a pack<const char N> specialization. However there is no equivalent specialization for convert(), so convert<char> is used which tries to extract char, as an int, from the object containing a string and fails.

IMO this is a pretty bad bug because everything compiles just fine and even works on the server (serialization) side but then fails during run-time on the client.

[redboltz]

@vadz, thank you for reporting the issue.

I think that the expected behavior is making compile error on the following line:

ret.get().convert(&s2);

I will study how to achieve that.

[redboltz]

I analyzed the problem.

msgpack-c has two convert interfaces. One is reference version:

https://github.com/msgpack/msgpack-c/blob/master/include/msgpack/object.hpp#L492

template <typename T>
inline T& object::convert(T& v) const
{
    msgpack::operator>>(*this, v);
    return v;
}

the other is pointer version:

https://github.com/msgpack/msgpack-c/blob/master/include/msgpack/object.hpp#L499

template <typename T>
inline T* object::convert(T* v) const
{
    convert(*v);
    return v;
}

The pointer version is kept for older version compatibility.

Consider the following code:

http://melpon.org/wandbox/permlink/abcohg5yciTP6pfX

#include <msgpack.hpp>
#include <iostream>

int main() {
    msgpack::zone z;
    msgpack::object obj("ABC", z);
    std::cout << obj << std::endl;

    char c;

    // call reference version
    obj.convert(c); // Shouldn't be a compile error, but be a runtime error

    // call pointer version
    obj.convert(&c); // Shouldn't be a compile error, but be a runtime error

    char* p;
    // call pointer version for `char` unexpectedly
    // If calling reference version of `char*`, I would get a compile error as expected.
    obj.convert(p); // Expected to be a compile error

}

The reported problem is similar as the line obj.convert(p);. I want to get a compile error on the line but don't get. On the contrary, obj.convert(&c); shouldn't make a compile error. There is no way to distinguish the two.

If I remove the pointer version of convert(), I would get a compile error as expected.

https://github.com/msgpack/msgpack-c/blob/master/include/msgpack/object.hpp#L499

#if 0
template <typename T>
inline T* object::convert(T* v) const
{
    convert(*v);
    return v;
}
#endif

I think that it's a time to remove the pointer version convert. I'd like to hear @nobu-k's idea.