Hey thanks for the PR! It would seem CI hasn't been run in awhile on this crate, so there are probably things unrelated to your change that are failing as new lints and such get added to clippy.

Let me know if you need any help with that.

Edit - Actually it's probably better if I create a separate PR fixing those issues.

@Nereuxofficial okay, you can rebase your branch on the latest master branch to pick up the CI fixes.

As for the overflow fix, I'm still working on reproducing the original issue with the example bytes I gave in that comment 😅

Also, here is the reference pseudocode for decoding variable byte integers, from the spec

multiplier = 1
value = 0

do
   encodedByte = 'next byte from stream'
   value += (encodedByte AND 127) * multiplier

   if (multiplier > 128*128*128)
      throw Error(Malformed Variable Byte Integer)

   multiplier *= 128
while ((encodedByte AND 128) != 0)

@Nereuxofficial okay, you can rebase your branch on the latest master branch to pick up the CI fixes.

As for the overflow fix, I'm still working on reproducing the original issue with the example bytes I gave in that comment 😅

Also, here is the reference pseudocode for decoding variable byte integers, from the spec

multiplier = 1
value = 0

do
   encodedByte = 'next byte from stream'
   value += (encodedByte AND 127) * multiplier

   if (multiplier > 128*128*128)
      throw Error(Malformed Variable Byte Integer)

   multiplier *= 128
while ((encodedByte AND 128) != 0)

Interesting. I still have the fuzzing file available but the example was exactly what was there before the fix. I'll have a look at this again later today

So these are the bytes crashing the program previously which i just decoded with decode_mqtt
[255, 255, 255, 255, 255, 255, 255, 255, 255, 254, 255, 53, 127, 255]
Here is the test i wrote:

		let data: &[u8] = &[
            255, 255, 255, 255, 255, 255, 255, 255, 255, 254, 255, 53, 127, 255,
        ];
        decode_mqtt(&mut BytesMut::from(data), ProtocolVersion::V500).unwrap();

I think we just need to limit the loop iterations to 4, as a variable byte integer cannot have more than 4 bytes.

And then maybe we don't need the > 128 * 128 * 128 check at all. Honestly I'm not sure why the pseudocode from the spec is so misleading. Maybe it's assuming the packet has already been validated or something.

fn decode_variable_int(bytes: &mut Cursor<&mut BytesMut>) -> Result<Option<u32>, DecodeError> {
    let mut multiplier: u32 = 1;
    let mut value: u32 = 0;

    for _ in 0..4 {
        let encoded_byte = read_u8!(bytes);

        value += ((encoded_byte & 0b0111_1111) as u32) * multiplier;

        multiplier *= 128;

        if encoded_byte & 0b1000_0000 == 0b0000_0000 {
            return Ok(Some(value));
        }
    }

    Err(DecodeError::InvalidRemainingLength)
}

We should also add a test for this failing scenario to make sure we don't repeat any mistakes.

I can add these changes if you prefer.

I think we just need to limit the loop iterations to 4, as a variable byte integer cannot have more than 4 bytes.

And then maybe we don't need the > 128 * 128 * 128 check at all. Honestly I'm not sure why the pseudocode from the spec is so misleading. Maybe it's assuming the packet has already been validated or something.

fn decode_variable_int(bytes: &mut Cursor<&mut BytesMut>) -> Result<Option<u32>, DecodeError> {
    let mut multiplier: u32 = 1;
    let mut value: u32 = 0;

    for _ in 0..4 {
        let encoded_byte = read_u8!(bytes);

        value += ((encoded_byte & 0b0111_1111) as u32) * multiplier;

        multiplier *= 128;

        if encoded_byte & 0b1000_0000 == 0b0000_0000 {
            return Ok(Some(value));
        }
    }

    Err(DecodeError::InvalidRemainingLength)
}

We should also add a test for this failing scenario to make sure we don't repeat any mistakes.

I can add these changes if you prefer.

Yeah me neither, i was trying to make a pure C implementation which would match the spec exactly and try whether it would crash as well and then send that example to OASIS so they may or may not fix that in the spec, because if someone implements it with their reference in C they risk overflowing their int which may be pretty bad.

Nvm it was another overflow in the program...
As for the mqtt_v5 implementation i'm going to implement your fix right now, thanks so much for your help!