C4K3 / ozelot

Rust library for handling all MCMODERN networking
Creative Commons Zero v1.0 Universal
34 stars 9 forks source link

Fixed off-by-one error when reading varints #5

Closed ostylk closed 6 years ago

ostylk commented 6 years ago

I had a problem connecting on a server resulting in this error: thread '<unnamed>' panicked at 'attempt to multiply with overflow', /home/ostylk/.cargo/registry/src/github.com-1ecc6299db9ec823/ozelot-0.6.0/src/read.rs:118:19

So it turned out that when "parsing" a varint 4294967296 has been mistakenly used as the max value for u32(it should be 4294967295 2^32 - 1). I replaced those magic numbers with the provided rust constants and that fixed my issue connecting on the server.

C4K3 commented 6 years ago

Thanks for the PR.

But now the test read_and_write!(-1, &[255, 255, 255, 255, 15], read_varint, write_varint); fails.

Is the test wrong?

C4K3 commented 6 years ago

I used the example varint code from wiki.vg, and it gets the same representation, so I think the test is correct.

https://ideone.com/ZpsDAd

ostylk commented 6 years ago

The test looks correct. That's really weird. I created a small program to reproduce the error I've described: https://pastebin.com/J5FkrGAY . kadcon.de is a german minecraft server. thread 'main' panicked at 'attempt to multiply with overflow', /home/ostylk/.cargo/registry/src/github.com-1ecc6299db9ec823/ozelot-0.6.0/src/read.rs:118:19

I don't think the server is sending faulty packets as the normal minecraft client can connect without any problem.

C4K3 commented 6 years ago

I can reproduce that. The varint that gets sent in hex is 0x80 80 80 80 08

C4K3 commented 6 years ago

Ok I think I know what's happening. The value in question is -2147483648, which fits in i32, but +2147483648 does not. The calculation is (4294967296 - res) as i32 * -1, so it's converting to +2147483648 and then flipping the sign, which causes an overflow error.

ostylk commented 6 years ago

So I've reworked the read and write methods of varint and added some tests to verify them. Now it seems to be working fine.

C4K3 commented 6 years ago

Awesome, thanks a lot.

Could you squash your commits? Then I'll merge.