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.

Sign up for GitHub

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

Jump to bottom

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
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 32 additions & 2 deletions generator/C/include_v0.9/protocol.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,13 +168,43 @@ static void mav_array_memcpy(void *dest, const void *src, size_t n)
}
}

/*
* Array direct assignment, for use when fields align and no byte swapping
* is required. Most are directly #defined to mav_array_memcpy, except for
* mav_array_assign_char, which uses strncpy instead.
*/
#if !MAVLINK_NEED_BYTE_SWAP && MAVLINK_ALIGNED_FIELDS
static inline void mav_array_assign_char(char *dest, const char *src, size_t n)
{
if (src == NULL) {
memset(dest, 0, n);
} else {
/* strncpy will zero pad dest array up to n bytes */
strncpy(dest, src, n);
}
}
#define mav_array_assign_uint8_t(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(uint8_t))
#define mav_array_assign_int8_t(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(int8_t))
#define mav_array_assign_uint16_t(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(uint16_t))
#define mav_array_assign_int16_t(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(int16_t))
#define mav_array_assign_uint32_t(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(uint32_t))
#define mav_array_assign_int32_t(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(int32_t))
#define mav_array_assign_uint64_t(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(uint64_t))
#define mav_array_assign_int64_t(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(int64_t))
#define mav_array_assign_float(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(float))
#define mav_array_assign_double(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(double))
#endif

/*
* Place a char array into a buffer
*/
static inline void _mav_put_char_array(char *buf, uint8_t wire_offset, const char *b, uint8_t array_length)
{
mav_array_memcpy(&buf[wire_offset], b, array_length);

if (b == NULL) {
memset(&buf[wire_offset], 0, array_length);
} else {
strncpy(&buf[wire_offset], b, array_length);
}
}

/*
Expand Down
34 changes: 32 additions & 2 deletions generator/C/include_v1.0/protocol.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,13 +186,43 @@ static inline void mav_array_memcpy(void *dest, const void *src, size_t n)
}
}

/*
* Array direct assignment, for use when fields align and no byte swapping
* is required. Most are directly #defined to mav_array_memcpy, except for
* mav_array_assign_char, which uses strncpy instead.
*/
#if !MAVLINK_NEED_BYTE_SWAP && MAVLINK_ALIGNED_FIELDS
static inline void mav_array_assign_char(char *dest, const char *src, size_t n)
{
if (src == NULL) {
memset(dest, 0, n);
} else {
/* strncpy will zero pad dest array up to n bytes */
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.

}
}
#define mav_array_assign_uint8_t(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(uint8_t))
#define mav_array_assign_int8_t(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(int8_t))
#define mav_array_assign_uint16_t(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(uint16_t))
#define mav_array_assign_int16_t(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(int16_t))
#define mav_array_assign_uint32_t(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(uint32_t))
#define mav_array_assign_int32_t(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(int32_t))
#define mav_array_assign_uint64_t(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(uint64_t))
#define mav_array_assign_int64_t(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(int64_t))
#define mav_array_assign_float(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(float))
#define mav_array_assign_double(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(double))
#endif

/*
* Place a char array into a buffer
*/
static inline void _mav_put_char_array(char *buf, uint8_t wire_offset, const char *b, uint8_t array_length)
{
mav_array_memcpy(&buf[wire_offset], b, array_length);

if (b == NULL) {
memset(&buf[wire_offset], 0, array_length);
} else {
strncpy(&buf[wire_offset], b, array_length);
}
}

/*
Expand Down
34 changes: 32 additions & 2 deletions generator/C/include_v2.0/protocol.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,13 +179,43 @@ static inline void mav_array_memcpy(void *dest, const void *src, size_t n)
}
}

/*
* Array direct assignment, for use when fields align and no byte swapping
* is required. Most are directly #defined to mav_array_memcpy, except for
* mav_array_assign_char, which uses strncpy instead.
*/
#if !MAVLINK_NEED_BYTE_SWAP && MAVLINK_ALIGNED_FIELDS
static inline void mav_array_assign_char(char *dest, const char *src, size_t n)
{
if (src == NULL) {
memset(dest, 0, n);
} else {
/* strncpy will zero pad dest array up to n bytes */
strncpy(dest, src, n);
}
}
#define mav_array_assign_uint8_t(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(uint8_t))
#define mav_array_assign_int8_t(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(int8_t))
#define mav_array_assign_uint16_t(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(uint16_t))
#define mav_array_assign_int16_t(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(int16_t))
#define mav_array_assign_uint32_t(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(uint32_t))
#define mav_array_assign_int32_t(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(int32_t))
#define mav_array_assign_uint64_t(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(uint64_t))
#define mav_array_assign_int64_t(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(int64_t))
#define mav_array_assign_float(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(float))
#define mav_array_assign_double(DEST,SRC,N) mav_array_memcpy(DEST,SRC,N*sizeof(double))
#endif

/*
* Place a char array into a buffer
*/
static inline void _mav_put_char_array(char *buf, uint8_t wire_offset, const char *b, uint8_t array_length)
{
mav_array_memcpy(&buf[wire_offset], b, array_length);

if (b == NULL) {
memset(&buf[wire_offset], 0, array_length);
} else {
strncpy(&buf[wire_offset], b, array_length);
}
}

/*
Expand Down
8 changes: 4 additions & 4 deletions generator/mavgen_c.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ def generate_message_h(directory, m):
mavlink_${name_lower}_t packet;
${{scalar_fields: packet.${name} = ${putname};
}}
${{array_fields: mav_array_memcpy(packet.${name}, ${name}, sizeof(${type})*${array_length});
${{array_fields: mav_array_assign_${type}(packet.${name}, ${name}, ${array_length});
}}
memcpy(_MAV_PAYLOAD_NON_CONST(msg), &packet, MAVLINK_MSG_ID_${name}_LEN);
#endif
Expand Down Expand Up @@ -304,7 +304,7 @@ def generate_message_h(directory, m):
mavlink_${name_lower}_t packet;
${{scalar_fields: packet.${name} = ${putname};
}}
${{array_fields: mav_array_memcpy(packet.${name}, ${name}, sizeof(${type})*${array_length});
${{array_fields: mav_array_assign_${type}(packet.${name}, ${name}, ${array_length});
}}
memcpy(_MAV_PAYLOAD_NON_CONST(msg), &packet, MAVLINK_MSG_ID_${name}_LEN);
#endif
Expand Down Expand Up @@ -376,7 +376,7 @@ def generate_message_h(directory, m):
mavlink_${name_lower}_t packet;
${{scalar_fields: packet.${name} = ${putname};
}}
${{array_fields: mav_array_memcpy(packet.${name}, ${name}, sizeof(${type})*${array_length});
${{array_fields: mav_array_assign_${type}(packet.${name}, ${name}, ${array_length});
}}
_mav_finalize_message_chan_send(chan, MAVLINK_MSG_ID_${name}, (const char *)&packet, MAVLINK_MSG_ID_${name}_MIN_LEN, MAVLINK_MSG_ID_${name}_LEN, MAVLINK_MSG_ID_${name}_CRC);
#endif
Expand Down Expand Up @@ -417,7 +417,7 @@ def generate_message_h(directory, m):
mavlink_${name_lower}_t *packet = (mavlink_${name_lower}_t *)msgbuf;
${{scalar_fields: packet->${name} = ${putname};
}}
${{array_fields: mav_array_memcpy(packet->${name}, ${name}, sizeof(${type})*${array_length});
${{array_fields: mav_array_assign_${type}(packet->${name}, ${name}, ${array_length});
}}
_mav_finalize_message_chan_send(chan, MAVLINK_MSG_ID_${name}, (const char *)packet, MAVLINK_MSG_ID_${name}_MIN_LEN, MAVLINK_MSG_ID_${name}_LEN, MAVLINK_MSG_ID_${name}_CRC);
#endif
Expand Down
Loading