Char[] / String handling is a bit broken?

[azerupi]

On the project I'm working on we are parsing MAVLink messages from PX4 and QGroundControl, intercepting some and forwarding the rest. While doing that I've noticed that some forwarded messages containing char[] fields are not byte equivalent before and after decode and re-encoding.

The MAVLink spec doesn't specify anything about NULL termination and each message seems to have its own specific requirements around it. Some examples:

• AUTH_KEY (7): key: char[32] nothing specified. No null-termination as the key would always be 32 chars long.
• PARAM_VALUE (22): param_id: char[16] terminated by NULL if the length is less than 16 human-readable chars and WITHOUT null termination (NULL) byte if the length is exactly 16 chars
• ADSB_VEHICLE (246): callsign: char[9] 8+null
• STATUSTEXT (253): text: char[50] Status text message, without null termination character

It is not clear to me if ALL strings that are smaller than the char[] field length should be null-terminated or not?

In any case rust-mavlink does not handle this correctly. If we look at the PARAM_VALUE_DATA struct that is generated:

pub struct PARAM_VALUE_DATA {
    // omitted some fields...
    pub param_id: arrayvec::ArrayString<16>,
    // omitted some fields...
}

In the deserialization code we can see that if a null byte is encountered it will be truncated from the string.

for _ in 0..16usize {
    let next_char = buf.get_u8();
    if next_char == 0 {
        break;
    }
    __struct.param_id.push(next_char as char);
}

In the serialization however we are only adding values for as many bytes are in the slice. First this is bad because it means if the array/string does not contain exactly the amount of bytes that the field is expecting we are shifting everything left. Second when re-encoding the message we received from the deserialization step we've already lost the null-byte if it was there and thus we are always triggering the issue.

let slice = self.param_id.as_bytes();
for val in slice {
    __tmp.put_u8(*val);
}

Here is an example of the PARAM_VALUE message before deserialization and after deserialization + re-serialization in wireshark. You can see the null byte is missing and the length is one shorter.

I can take a stab at fixing this, but I would first like to discuss what the right solution would be. Would the simple solution of zero filling the remaining bytes be the correct solution for all cases?

[azerupi]

It is also wrong on the decoding side because if there are multiple null values filling the end of the char[] array we stop reading at the first and then after that the next null byte is read as the next field. We should flush the null bytes.

[joaoantoniocardoso]

Thanks for raising the issue. This is a very important thing to fix ASAP. From my tests, it has been broken since before 0.10.

I first noticed this problem when I was writing tests using random messages (arbitrary feature), but I had to pause the investigations to focus on another project fully.

It is not clear to me if ALL strings that are smaller than the char[] field length should be null-terminated or not?

This is confusing indeed. My current understanding is that we should at least null-terminate when we have enough space for it.

Also, as we want to be fully compliant with the specs, if there are exceptions, we'd need to bake them in.

First this is bad because it means if the array/string does not contain exactly the amount of bytes that the field is expecting we are shifting everything left.

Feel free to explain your reasoning, I'm failing to understand the problem here -- the deserialized object is not meant to replicate the binary representation of the "raw" MAVLink bytes.

Second when re-encoding the message we received from the deserialization step we've already lost the null-byte if it was there and thus we are always triggering the issue.

It is also wrong on the decoding side because if there are multiple null values filling the end of the char[] array we stop reading at the first and then after that the next null byte is read as the next field. We should flush the null bytes.

Good points. Perhaps we should code the ideal serialization/deserialization in a contained example, and then see how we can fix it in the lib?

---

Also, related:

I'm currently reworking (PR) the char[]/string part to be presented as a human-readable string, and not as an array of bytes (like in 0.10) or an array of chars (like in 0.13.x and maybe 0.14.x).

[azerupi]

First this is bad because it means if the array/string does not contain exactly the amount of bytes that the field is expecting we are shifting everything left.

Feel free to explain your reasoning, I'm failing to understand the problem here -- the deserialized object is not meant to replicate the binary representation of the "raw" MAVLink bytes.

What I mean is that if the array len is not the same as capacity then we write only part of the bytes to the buffer and don't pad the rest so the next field would be written at the wrong location. It would be equivalent to truncating the char[] field even if it is not the last one.

Good points. Perhaps we should code the ideal serialization/deserialization in a contained example, and then see how we can fix it in the lib?

I'm currently adding tests based on real messages captured from communication between PX4 and QGroundControl that seem to fail. I'll try to provide a fix for at least those but we should verify that it works in the general case.

[joaoantoniocardoso]

@azerupi can you check if the same problem still happens with PR #376?

[onur-ozkan]

Let's keep this open until the fix is confirmed by the reporter.

[azerupi]

@joaoantoniocardoso it does work, but at he expense of comfort.
Where previously you could access the string with as_str() like this:

match msg {
    mavlink::common::MavMessage::PARAM_VALUE(param_value) => {
        if param_value.param_id.as_str() == "_HASH_CHECK" {}
}

Now the user has to convert the byte slice that may contain null bytes himself. Can we maybe provide a wrapper type around the slice that has a method to retrieve either the full byte slice or the str truncating from the first null byte?

[joaoantoniocardoso]

Yes, that sounds great

[azerupi]

Great, I'll try to submit something if I have time later today or next week unless someone beats me to it 🙂