ArduPilot / ArduPilot/pymavlink
Undefined Behavior in mavlink_helpers.h: Left Shift of Negative Value
- Dominant language
- Python
- Stars
- 728
- Forks
- 739
- Avg merge
- 4d 10h
- Merged PRs (30d)
- 7
Description
The clang [UndefinedBehaviorSanitizer](https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html) flagged a line in the mavlink C library v1 in [mavlink_helpers.h](https://github.com/ArduPilot/pymavlink/blob/f55377066e5f5dba2c63b7a5d116454b1b0ce303/generator/C/include_v1.0/mavlink_helpers.h):
```C
if (status->msg_received == MAVLINK_FRAMING_BAD_CRC) {
/*
the CRC came out wrong. We now need to overwrite the
msg CRC with the one on the wire so that if the
caller decides to forward the message anyway that
mavlink_msg_to_send_buffer() won't overwrite the
checksum
*/
r_message->checksum = _MAV_PAYLOAD(rxmsg)[status->packet_idx] |(_MAV_PAYLOAD(rxmsg)[status->packet_idx+1]<<8);
}
```
Here, the intention is to overwrite `r_message->checksum` with a 2-btye checksum from `_MAV_PAYLOAD`. The second char is left-shifted by 8 bits. [Left-shifting negative values results in undefined behavior in C](https://stackoverflow.com/questions/3784996/why-does-left-shift-operation-invoke-undefined-behaviour-when-the-left-side-oper).
The byte types could be changed to `uint8_t`, or it may be sufficient to reinterpret the values as `uint16_t`:
```
r_message->checksum = *reinterpret_cast(_MAV_PAYLOAD(rxmsg)[status->packet_idx]);
```
I did not check other versions of mavlink C, but the problem may exist there as well.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start in generator/C/include_v1.0/mavlink_helpers.h at the checksum assignment reported by Clang UBSan. Check the other MAVLink C versions mentioned in the issue, then verify that the checksum is assembled without undefined left-shift behavior and that UBSan no longer reports the case.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c
- Domain
- networking
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100