check null pointer before using memcpy()

[ygj6]

Avoid passing null pointer to memcpy().
Fixed the following error mentioned by @jamessan in issue #881:

runtime error: null pointer passed as argument 2, which is declared to never be null

[codecov-commenter]

Codecov Report

Merging #890 into c_master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           c_master     #890   +/-   ##
=========================================
  Coverage     52.82%   52.82%           
=========================================
  Files             8        8           
  Lines          1043     1043           
=========================================
  Hits            551      551           
  Misses          492      492           

[jamessan]

Should this be inside the if?

[redboltz]

I think that the fix code has no problem.
Source data are buf and len. And the destination is sbuf->data.

If buf is nullptr then doesn't copy. This archives nullptr dereference.
Then simply add size.

Passing buf as nullptr is unnatural case. In this case, len should be 0.

[jamessan]

I prefer to be paranoid. If buf is null but len is non-zero (a bug in the calling code), why should sbuf->size change?

[redboltz]

I think that it is msgpack-c API spec definition problem.
The error hannpens at

msgpack-c/test/msgpack_c.cpp

Line 875 in 82ddfc1

msgpack_pack_str_body(&pk, str, str_size);

str is NULL.

    msgpack_pack_str_body(&pk, str, str_size);

msgpack_pack_str_body() accepts or doesn't accept NULL 2nd parameter is not documented.

If msgpack_pack_str_body() accepts it, how str_size is treated. It is also not documented.

Possible choice:

1. Ignore str_size.
2. Advance the pointer by str_size without copy.

(3. If msgpack_pack_str_body() doesn't accept NULL str, then UB.)

I think that 3 is not good.
The current PR is 2.
If msgpack_pack_str_body() respects str, then 1 is better.
If msgpack_pack_str_body() respects str_size, then 2 is better.

I think that choosing 1 or 2 is just a preference.

I think that assert(str || str_size == 0); should be added on the top of the function.
The assert means if str is NULL then str_size should be 0. If str is not NULL, any str_size is OK including 0.

[redboltz]

@ygj6, could you add assert ?

And, it is out of spec but could you update the code as folows ?

    if(buf) { 
        memcpy(sbuf->data + sbuf->size, buf, len); 
        sbuf->size += len;
    }
}

I choose option 1 above.

BTW, the real problem is poor documentation. I know about that.
Unfortunately I don't have much time to write Doxygen style document (comment) like

msgpack-c/include/msgpack/v1/unpack.hpp

Lines 933 to 947 in 2f5b137

	/// Unpack one msgpack::object.
	/**
	*
	* @param result The object that contains unpacked data.
	* @param referenced If the unpacked object contains reference of the buffer,
	* then set as true, otherwise false.
	*
	* @return If one msgpack::object is unpacked, then return true, if msgpack::object is incomplete
	* and additional data is required, then return false. If data format is invalid, throw
	* msgpack::parse_error.
	*
	* See:
	* https://github.com/msgpack/msgpack-c/wiki/v1_1_cpp_unpacker#msgpack-controls-a-buffer
	*/
	bool next(msgpack::object_handle& result, bool& referenced);

to whole code.

So I wrote minimal document on wiki.
https://github.com/msgpack/msgpack-c/wiki/v2_0_c_overview

We will add document to wiki or code in the future.

[ygj6]

Okay, I will add assert(str || str_size == 0) on line 875 in msgpack-c/test/msgpack_c.cpp.

[redboltz]

@ygj6 ,sorry to confuse you.

I wrote assert(str || str_size == 0). It's wrong.
It should be assert(buf || len == 0)

I mean adding assert(buf || len == 0) to

msgpack-c/include/msgpack/sbuffer.h

Line 61 in ba15089

msgpack_sbuffer* sbuf = (msgpack_sbuffer*)data;

.

The combination of NULL str and non zero len is apparently misuse. So I think that assert should be added in the library.
In the release build, assert is simple removed. For this case,

    if(buf) { 
        memcpy(sbuf->data + sbuf->size, buf, len); 
        sbuf->size += len;
    }

avoid copy and increment size.

[ygj6]

For some older compilers, the statement must follow the variable declaration. Therefore, I will add assert(buf || len == 0) after the line msgpack_sbuffer* sbuf = (msgpack_sbuffer*)data;. What do you think?

[redboltz]

For some older compilers, the statement must follow the variable declaration. Therefore, I will add assert(buf || len == 0) after the line msgpack_sbuffer* sbuf = (msgpack_sbuffer*)data;. What do you think?

I agree. I forgot about oder C (ANSI-C).