gamedig / rust-gamedig

Game Server Query Library.
https://crates.io/crates/gamedig
MIT License
38 stars 11 forks source link

Packet Underflow #26

Closed cainthebest closed 1 year ago

cainthebest commented 1 year ago

When using the lib to call this rust server a error is thrown.

image

CosminPerRam commented 1 year ago

Didn't had the time to look into it, but I'm almost sure it's because of the get_string_utf8, get_string_utf8_unended and/or get_string_utf16 of the Bufferer. When they have no data left, they throw this error (also happens for u8, u16 and so on, but it's something that I also encountered on strings). I'm not sure whether to remove this buffer size check completely or not (for strings).

cainthebest commented 1 year ago

When I did some debugging I was able to know that it is coming from 1 of those functions within bufferer. I think this can be confirmed as a bug. The issue maybe occurring on empty strings as in hexadecimal it would be represented as simply "" (without quotes). In other words, an empty sequence of bytes would have no hexadecimal representation so when it gets to if_empty() on a empty string it throws but that still unknown.

CosminPerRam commented 1 year ago

Not really a bug, calling one of those functions does not expect the buffer to be empty, maybe you want a get a string and you know that it must contain something (even only the NULL character, that's something).

CosminPerRam commented 1 year ago

An empty string (in UTF8 at least) is just 0x00 if I'm right. The problem here is that some games (or protocols?) while having a string as a last part of the packet and also while it is empty, doesnt provide anything (not even the null byte).

Example of BF2142 (GameSpy 3) doing this: image On the last key, where the value is a string and as it is empty, there are no bytes left.

CosminPerRam commented 1 year ago

As I'm working on #25 I encountered the same problem and I ended up creating another function, get_string_utf8_optional which if the buffer is empty, it returns an empty string.

Having a normal get_string_utf8, this get_string_utf8_optional and the additional get_string_utf8_unended when packets end with a string but they dont provide the final NULL byte, is a bit messy and I'll have to look into making this cleaner.

cainthebest commented 1 year ago

All of this makes a lot more sense and possibly get_string can just be a generic fn. Ill get on now and make a pr for a more generic fn and see from there.

cainthebest commented 1 year ago

@CosminPerRam what do you think of this as a possible way to make it a it simpler

CosminPerRam commented 1 year ago

Looks really nice, I just have a small comment, would you make a PR? we'll discuss thing over there.

cainthebest commented 1 year ago

I'm deviating from the example shown for prod use as it comes with shortfalls in protocol impl, Ill open a draft when I got something together.

cainthebest commented 1 year ago

Looks really nice, I just have a small comment, would you make a PR? we'll discuss thing over there.

A draft has been made for this https://github.com/gamedig/rust-gamedig/pull/27

cainthebest commented 1 year ago

Fixed in #62