Mavgen C: Use strncpy instead of memcpy to pack char[n] fields

[shancock884]

The aim of this PR is to fix #725 and fix mavlink/mavlink#1946

Current situation:

At present when a user calls the mavlink_msg_<message>_pack function on a message containing char[n] fields, these fields will be copied into the destination buffer using memcpy. This can go wrong in a couple of ways, if end users are not careful, for example....

1. If specifying an argument as a string literal, the read will go beyond the input variable's scope, which could have some bad consequences:

mavlink_msg_param_request_read_pack(mysys, mycomp, &msg, tgtsys, tgtcomp, "PARAM", -1);

2. Even if using a large enough buffer, it is possible that the bits of memory beyond the null may contain leftover or random values, which would end up in the message. In this example, bytes after null could be anything when requesting PARAM, and will include the last bit of LONGER_PARAM when reading SHORT:

const char* list_of_params = {"PARAM", "LONGER_PARAM", "SHORT"};
char param[17]; // enough for null
for (int i=0; i<3;i++) {
    strcpy(param,list_of_params_i_want[i]);
    mavlink_msg_param_request_read_pack(mysys, mycomp, &msg, tgtsys, tgtcomp, param, -1);
}

These extra bytes will stop the Mavlink 2 message-truncation from being as efficient as it can be, and may confuse some receivers.

Proposal:

This PR proposes to use the strncpy function when packing char arrays, as I believe that this aligns with the way in which Mavlink char array fields are specified and used:

• destination array will be filled up to a maximum of n chars, and therefore may not be null-terminated.
• if null is found within the n characters, reading stops, and the remainder of the destination up to n is filled with zeros.

This should both protect against reading bad memory, and also ensure bytes beyond the length of short strings are zero, allowing the Mavlink 2 message truncation to work properly.

For little-endian and struct-aligned cases: This is done by replacing the calls to mav_array_memcpy in mavgen_c.py with type-specific array assignment macros mavlink_array_assign_[type]. For char arrays, this uses strncpy (after check that input is not null pointer). For other arrays, this is a thin-macro which calls mav_array_memcpy, as per current behaviour.

Big big-endian or non-struct-aligned cases: This is done by an update to the existing _mav_put_char_array function to use strncpy (after check that input is not null pointer).

Testing

I've tested this update (in particular using the PARAM_REQUEST_READ and PLAY_TUNE messages) on my system. I also checked compilation with MAVLINK_ALIGNED_FIELDS=0.

[hamishwillee]

@peterbarker Can you please look at this. Fixes https://github.com/ArduPilot/pymavlink/issues/725 which has come up a few times as a problem, in particular for new users after they have discovered this the hard way.

[peterbarker]

On Tue, 17 Sep 2024, Simon Hancock wrote:

/* strncpy will zero pad dest array up to n bytes */

Not a blocker at all, but I will note that ArduPilot's "strncpy_noterm" does not obey this rule.

[hamishwillee]

So there is no reason this can't go in @peterbarker? Can you merge?