nikon/CANopenNode
1
0
Fork 0
Code Activity Actions

Fix shift-by-64 UB for mappedLength==0 in PDO bitwise mapping

Browse source 
In 301/CO_PDO.c, when CO_CONFIG_PDO_BITWISE_MAPPING is enabled, a
mapped object with mappedLength == 0 bits is a legal mapping (see the
CO_PDO_t.OD_IO documentation: "mappedLengthBits can be less or equal
to the OD_IO.dataLength*8"), but nothing rejects it and two spots
compute a shift amount as "64 - mappedLength", which becomes a
shift-by-64 on a uint64_t. Shifting a 64-bit value by 64 is undefined
behaviour in C and is not portable across compilers/optimization
levels (verified: an -O0 build silently no-ops the shift so the
intended mask/skip has no effect, while an -O2 build of the same
source produces a different result for the same input).

- CO_RPDO_process(): shiftedData was computed unconditionally as
  "buf64 & (UINT64_MAX >> (64 - mappedLength))"; now short-circuits to
  0 when mappedLength == 0, matching the documented semantics that a
  0-bit mapping should leave the target OD variable untouched.
- CO_TPDOsend(): the same masking pattern, plus a second UB spot
  where the result is placed with "buf <<= (verifyLength -
  mappedLength)", which is also a shift-by-64 once a preceding mapped
  entry already fills the full PDO width. Both operations are now
  skipped when mappedLength == 0, so a 0-bit-mapped OD variable never
  contributes any of its raw bytes to the outgoing TPDO buffer.

Verified against current master (9b8beed): compiled 301/CO_PDO.c and
linked the example/ project standalone with
CO_CONFIG_PDO_BITWISE_MAPPING forced on, with no new warnings.
Independently re-derived the shift logic in an isolated harness and
confirmed the fixed code matches a portable reference mask for every
legal mappedLength (0-64) with zero mismatches, and that both the RPDO
"leftover payload leaks into a 0-bit mapped variable" scenario and the
TPDO "0-bit mapped variable leaks its raw bytes onto the bus" scenario
are eliminated at both -O0 and -O2.
This commit is contained in:
94xhn 2026-07-13 18:36:33 +08:00
parent 9b8beed836
commit 0b1154407e
1 changed files with 14 additions and 6 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

20
301/CO_PDO.c
View file

@ -819,8 +819,10 @@ CO_RPDO_process(CO_RPDO_t* RPDO,
/* Prepare data for writing into OD variable. If mappedLength
* is smaller than ODdataLength, then use auxiliary buffer */
uint8_t* dataOD;
/* Apply the bitmask */
uint64_t shiftedData = buf64 & (UINT64_MAX >> (64 - mappedLength));
/* Apply the bitmask. mappedLength of 0 is a valid (if unusual)
* mapping - "(64 - mappedLength)" would then shift by 64,
* which is undefined behaviour, so handle it explicitly. */
uint64_t shiftedData = (mappedLength == 0U) ? 0U : (buf64 & (UINT64_MAX >> (64 - mappedLength)));
/* Shift the original buffer to get ready for the next mapping */
buf64 >>= mappedLength;
#ifdef CO_BIG_ENDIAN
@ -1262,10 +1264,16 @@ CO_TPDOsend(CO_TPDO_t* TPDO) {
* For LE not needed, low bytes from the OD entry are copied to low bytes of u64 */
buf >>= 64 - 8 * ODdataLength;
#endif /* CO_BIG_ENDIAN */
/* Apply the mask and merge with the rest */
buf &= (UINT64_MAX >> (64 - mappedLength));
buf <<= (verifyLength - mappedLength);
buf64 |= buf;
/* Apply the mask and merge with the rest. mappedLength of 0 is a
* valid (if unusual) mapping - it must contribute nothing to buf64.
* Skip the shifts in that case, since "(64 - mappedLength)" and/or
* "(verifyLength - mappedLength)" can then evaluate to 64, and
* shifting a 64-bit value by 64 is undefined behaviour. */
if (mappedLength > 0U) {
buf &= (UINT64_MAX >> (64 - mappedLength));
buf <<= (verifyLength - mappedLength);
buf64 |= buf;
}
#else
OD_IO->read(stream, dataTPDOCopy, ODdataLength, &countRd);
#endif /* (CO_CONFIG_PDO) & (CO_CONFIG_PDO_BITWISE_MAPPING) */