fixing some unchecked additions

[Usioumeo]

Pull Request

Summary

I was fuzz-testing a library that depends on this repo, and I got some errors.
Simply in SerMsg::unpack_corbs(...) there were two additions that in some particular circumstances could overflow, but rust adds some controls for this types of error and sometimes it panicked with "attempting to add with overflow".
Changing this to additions with "wrapping_add" we get a "controlled overflow" like in C (and i think this was the intent).

Description

Changed two additions (line 273 and 277) with wrapping add.

Motivation and Context

Fixes some hard to catch panics in an embedded context.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

• My code has been formatted with "cargo fmt".
• My code has been checked with "cargo clippy".
• My code is passing all previous and new tests.
• I have added new tests for my changes (if applicable).
• My change requires a change/update to the documentation.
• I have updated the documentation accordingly.

Other info

[LukaOber]

Thank you very much for the PR and interest in the library.

Can you provide me with an example where your overflow happens? The addition of the 'self.cobs_byte' and 'delta' should never be able to exceed the byte limit of 255.

I suspect there might be a bug/problem somewhere else.

[Usioumeo]

Sure, this is one of test that fails on the first sum (line 273):

#[test]
fn test_overflow(){
    let v = [ 126, 1, 7, 10, 62, 144, 168, 18, 47, 0, 35, 253, 239, 188, 13, 129];
    let mut m = SerMsg::new();
    m.parse_read_bytes(&v);
}

Pay attention that you have to compile it in debug mode, because in release it removes the overflow check

[LukaOber]

This is not a message this library produced but is instead a self made one for testing, correct?

In a real message this should never happen. 'delta' + 'self.cobs_byte' can not exceed the maximum packet size of 254 Bytes in a non-damaged message. If the message is damaged it should have been detected by the CRC-Check earlier.

To be extra safe I will however add a check for an overflow (probably tomorrow). Your provided solution using wrapping_add fixes the overflow panic but I think it is damaging parts of the message. Deserializing data with your solution will lead to data corruption, I think.

[Usioumeo]

Like you said is not a correct message, but it was malformed from a simulated serial (very faulty, it skips/add bytes and sometimes it change them). Nonetheless that doesn't mean it isn't a real message, because if some unlucky errors happened it should be caught from crc-8, if not from all the other checks, if it's invalid but still pass there isn't much you could do (other than adding redundancy/other checksum... but it's out of scope here).

I thought that cobs was doing some mod 256 arithmetic, but then I checked online and found that it was simpler than what I tought.
I changed the commit referred to this request with a new modification, where it checks if adding the delta goes out of the payload.

Also I'm not sure why you were checking it two times and I wasn't so sure why (you already checked it two lines above).

[LukaOber]

Also I'm not sure why you were checking it two times and I wasn't so sure why (you already checked it two lines above).

I just noticed this as well, checking twice is a mistake I overlooked.

Using saturating_add feels correct here. I might add another test tomorrow and will then merge this PR.