bugfix: append trailing zero to memory buffer, to allow safe use as char*

[rhaschke]

Often the content of a MemoryBuffer is interpreted as a char*, for example in rviz.
However, the MemoryBuffer was missing a trailing zero, leading to a buffer-overflow:

==16592==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x62f000515864 at pc 0x7f48bbeb366e bp 0x7fff1263cf30 sp 0x7fff1263c6d8
READ of size 54373 at 0x62f000515864 thread T0
    #0 0x7f48bbeb366d  (/usr/lib/x86_64-linux-gnu/libasan.so.4+0x5166d)
    #1 0x7f48bba921b7 in rviz::getMeshUnitRescale(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /homes/rhaschke/src/ros/src/rviz/src/rviz/mesh_loader.cpp:601
 ...
0x62f000515864 is located 0 bytes to the right of 54372-byte region [0x62f000508400,0x62f000515864)
allocated by thread T0 here:
    #0 0x7f48bbf42618 in operator new[](unsigned long) (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xe0618)
    #1 0x7f48b8784f50 in resource_retriever::Retriever::get(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (/opt/ros/melodic/lib/libresource_retriever.so+0x2f50)

Indeed, reported buffer sizes from MemoryBuffer and from strlen() are different usually:

package://pa10_7a_description/meshes/link1_mesh.dae: buffer size=54372 string size=54375
package://pa10_7a_description/meshes/link2_mesh.dae: buffer size=110579 string size=110587

This PR solves that potential issue by adding a trailing zero to the MemoryBuffer, while still reporting the original size. Alternatively, of course one could fix this in all usage locations, e.g. in rviz. However to append the trailing zero there, the memory buffer needs to be reallocated (again) to make space for the new char.

[clalancette]

This PR solves that potential issue by adding a trailing zero to the MemoryBuffer, while still reporting the original size. Alternatively, of course one could fix this in all usage locations, e.g. in rviz. However to append the trailing zero there, the memory buffer needs to be reallocated (again) to make space for the new char.

I agree that this should be fixed here, rather than in the callers. It looks like you've taken care of most of the cases, which is awesome, thanks. There is one corner case that would still be problematic; fetching of a 0-sized resource. In that case, curl_easy_perform would return 0, but buf.v.empty() returns true, so we never append a trailing \0. I think we probably want to handle that case in the same way now, so that downstream consumers can still call strlen and get a zero-size. What do you think?

[rhaschke]

Well spotted. However, in that case the returned MemoryResource comprises a nullptr, which is fine and probably handled by callers anyway. No need to allocate a 1-byte buffer.

[clalancette]

Well spotted. However, in that case the returned MemoryResource comprises a nullptr, which is fine and probably handled by callers anyway. No need to allocate a 1-byte buffer.

True. OK, I'll buy that logic, thanks :).

[clalancette]

Looks good to me. @sloretz , thoughts?

[sloretz]

I'm OK with this since res.size still shows what bytes were actually received, though I think the rviz example should be using Parse(res.data.get(), res.size).