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 #725 which has come up a few times as a problem, in particular for new users after they have discovered this the hard way.

[tridge]

the problem is that this will leave the bytes after the string uninitialised, which leaks information and will change the CRC. I think we need to have a mav_strcpy() which zero fills

[shancock884]

Hi. My understanding of strncpy is that it already will zero-fill the destination buffer up to n, in the case where the src buffer is shorter.
e.g. this page states "If the array pointed to by s2 is a string that is shorter than n bytes, NUL characters shall be appended to the copy in the array pointed to by s1, until n bytes in all are written."

[julianoes]

I didn't know that strncpy fills with zeros. I also always assumed it just wrote only one zero. In that case, I think this should work.

@tridge do you agree?

[julianoes]

I'm bumping this as it would be nice to have!

[tridge]

hmm, it does seem to be guaranteed by the strncpy definition

       If the length of src is less than n, strncpy() writes additional null bytes to dest to ensure that a total of n bytes are written.

[shancock884]

I've added a code comment to each strncpy call to say this, as it is indeed not a very known feature:
/* strncpy will zero pad dest array up to n bytes */
Is this then ok to resolve the review comment?
Also rebased to latest master.

[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?

[peterbarker]

I've marked for discussion at DevCallEU