mavlink_msg_xxx_decode by mavgen_c does not handle extensions of message properly

[jnippula]

In mavlink_msg_${name_lower}_decode() function, there is a msg length check and memset(0) in "#else" section to take care of possibly trimmed messages and fill the last removed fields with zeros: https://github.com/ArduPilot/pymavlink/blob/ba8ca05b2b905c6af2ae20aafa12d7bd09ce9195/generator/mavgen_c.py#L403-L404

However, in the "#if MAVLINK_NEED_BYTE_SWAP || !MAVLINK_ALIGNED_FIELDS" section this check is missing: https://github.com/ArduPilot/pymavlink/blob/ba8ca05b2b905c6af2ae20aafa12d7bd09ce9195/generator/mavgen_c.py#L400

In case the message has extension fields which have been trimmed out by sender (message length are reduced accordingly), the receiver may just copy those fields from memory outside of the message data area and filling with garbage.

This has caused issues at least with HIL_GPS message, where last two fields (id and yaw) were set zero by sender and the receiver got some random values in id field causing it to detect multiple GPS sensors.

    hil_gps->id = mavlink_msg_hil_gps_get_id(msg);
    hil_gps->yaw = mavlink_msg_hil_gps_get_yaw(msg);

[jnippula]

I created PR for fix proposal: https://github.com/ArduPilot/pymavlink/pull/700

[hamishwillee]

@peterbarker Does this make sense?

[jlaitine]

Any update on this? We have to do ugly workarounds around the issue, exactly for HIL_GPS message handling. It is also theoretically possible to get a crash due to this in MMU enabled environments, if the message data area happens to be in the very end of the mapped memory. After all,it is reading bytes outside of the buffer. Unlikely, perhaps, but possible. It is not nice to receive garbage for the extension bytes when the sender has sent 0s.

[hamishwillee]

Not from me. @peterbarker ?

[jnippula]

Actually, this may affect also to COMMAND_ACK message handling. https://mavlink.io/en/messages/common.html#COMMAND_ACK

In case the target_component is zero, it can be something garbage in receiver end. E.g. MAVSDK rejects the ack message if target_component does not match with it's own component_id.

[hamishwillee]

@JonasVautherin FYI in case this does affect MAVSDK

[JonasVautherin]

The PR makes sense to me :+1:. Bonus given to the point that the fix has apparently been used by @jnippula for a while, so apparently it's working :blush:

[jnippula]

The implementation had to be changed because the original proposal did not handle properly other than fields with length of 1 byte. Also, just realized that the trimming feature affects to all kind of message fields, not only extension fields (and this is also clearly described in the documentation). This makes the issue much bigger.

One clear issue is that pymavlink test scripts does not take big endian case into account at all.