Inefficient, possibly dangerous integer overflow checking #775
Description
In template_callback_array (unpack.c), we have:
size = n*sizeof(msgpack_object);
if (size / sizeof(msgpack_object) != n) {
// integer overflow
return MSGPACK_UNPACK_NOMEM_ERROR;
}
This is very inefficient - division is one of the more costly instructions. I haven't analyzed it carefully, but it may not always work. The problem is that on 64-bit, sizeof is also 64-bit. On most platforms, unsigned int is 32-bit. We'll do the multiplication, operator upcast to unsigned 64-bit int, then truncate back down on assignment. Also, note that an input of n = 0 will pass this check.
A better way to do this is to simply do the math in 64-bit, assign the result to a 64-bit integer type, and then check to see if the high bits are set. It will be provably correct, and run a lot faster.
Then finally, there is this:
o->via.array.ptr = (msgpack_object*)msgpack_zone_malloc(*u->z, size);
If size is 0, then a standards compliant allocator will give you a non-null pointer that you're not supposed to read or write. I'm not sure what sort of trouble this would cause downstream, but I suspect that n=0 ought to be considered an invalid input. If so, check for that and reject it.