Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shancock884
Copy link
Contributor

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);
  1. 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
Copy link
Contributor

@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.

if (src == NULL) {
memset(dest, 0, n);
} else {
strncpy(dest, src, n);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Uses new macros mavlink_array_assign_[type] in place of direct calls to mav_array_memcpy.
The 'char' version then uses strncpy, while others call mav_array_memcpy as before
Update _mav_put_char_array function to use strncpy
@peterbarker
Copy link
Contributor

peterbarker commented Sep 18, 2024 via email

@hamishwillee
Copy link
Contributor

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

@peterbarker
Copy link
Contributor

I've marked for discussion at DevCallEU

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants