String serialization

[andrewvoznytsa]

Seems like mavlink is handling string serialization in non common way.

Let's look, for example, at uint16_t mavlink_msg_camera_information_pack_chan(... , const char *cam_definition_uri);

As one would expect cam_definition_uri is variable-length zero-terminated string because it is natural on the 'host' side (C/C++ app). But if you look inside mavlink_msg_camera_information_pack_chan() you'll see that it is handled with _mav_put_char_array(buf, 95, cam_definition_uri, 140); which expands into mav_array_memcpy(&buf[wire_offset], b, array_length); which expands into memcpy(dest, src, n); which basically is not either safe nor secure because it can go beyond buffer boundaries and which was generated by pymavlink and which is the reason why am I here.

So the workaround to handle that safely is to pre-allocate big enough buffer, copy string there, then pass that buffer to one of those serialization calls. Which sounds odd - if you get your url as a std::string which you use to save time on your dev cycle, because most of the routine is handled by compilier and/or run time library, you have to return back to the roots of development.

Since mavlink serialization code is a bridge between host (C/C++) on one side and mavlink bus on the other side I'd expect that it follows hosts assumptions and translates them correctly into mavlink messages. What is confusing there on the host side is definition of const char* cam_definition_uri - URI and const char* assumes zero terminated string. But what current code expects is char uri[sizeof(mavlink_camera_information_t::cam_definition_uri)] and therefore mavlink_msg_camera_information_pack_chan should look like as uint16_t mavlink_msg_camera_information_pack_chan(... , const char uri[sizeof(mavlink_camera_information_t::cam_definition_uri)]); which technically is the same pointer as before but at least gives idea to developers what kinds of stuff it expect as input which is already much better. What would make it even better is something like uint16_t mavlink_msg_camera_information_pack_chan(... , const char* uri, size_t uri_length); And my favorite is commond sense uint16_t mavlink_msg_camera_information_pack_chan(... , const char *cam_definition_uri); using srtncpy() to copy the URI.

So my question is - is there any chance to get the fix merged soon or it is something that can be only evaluated for one of the next major updates? (on my side it looks like - I'll either review all my code and add workarounds where necessary or fix mavlink code generator to work in a way expected by humans).

[shancock884]

Ah yes, I recall noticing that I must use a full sized array when building a PLAY_TUNE message, but did not dig as you have.

I would vote for using strncpy, as it seems to make the most sense for C programmers use. This has an added advantage that characters beyond the null up to the specified length get set to zero, so any random gumf in memory beyond the null wont accidentally end up in the message, and potentially end up stopping the trailing-zero-truncation being as efficient as it should be. Note that the function would need to check for NULL input parameter and do a memset to 0, to maintain current functionality of mav_array_memcpy.

_mav_put_char_array is in generator/C/include_v2.0/protocol.h, so seems to be a relatively isolated change, but actually it looks like in little endian + stuct aligned cases, it isn't actually called, and the generated code goes directly to mav_array_memcpy for all arrays.

[shancock884]

I've had a crack at this in the following branch, if you want to try it out: https://github.com/shancock884/pymavlink/tree/iss725-string-serializarion

[andrewvoznytsa]

Thanks for your interest and suggestions. I had to add workarounds in my code and forget about this. I was ready to do a bit more work and fix it properly on mavlink side but due to lack of interested from others I had to choose the way which is fully under my control with known time frames - I can't afford to start working on the issue in a year after problem discovering.

[shancock884]

Ok. No worries. I came to start using this project only recently, and when I saw 'September' and didnt realise it was a year ago... I will use my updates anyway, and see if they can be accepted.