-
Notifications
You must be signed in to change notification settings - Fork 598
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
base: master
Are you sure you want to change the base?
Mavgen C: Use strncpy instead of memcpy to pack char[n] fields #922
Conversation
@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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
366ebe5
to
f88ec4f
Compare
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.
|
So there is no reason this can't go in @peterbarker? Can you merge? |
I've marked for discussion at DevCallEU |
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....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:
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 macrosmavlink_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 usestrncpy
(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.