How do I check that, say, stream.ReadFixedString64() is a valid thing to call right now?

Unity Engine
, , , , ,
1

So, I know I’m expecting the next thing in my stream (DataStreamReader) to be readable using stream.ReadFixedString64()

Which it is! Works fine, no probs at all. But, how do I know that’s actually what’s in the stream in the first place? Like, what if someone modded the client and is making the server send me an invalid packet? How do I check that the next thing can actually be read using ReadFixedString64? With an integer it’s easy, just check if the remaining stream length at least 4 bytes. But I don’t know the rules for a string, but I do know from testing that the length is variable despite the word “Fixed”.

Or am I meant to check the result of stream.HasFailedReads after I’ve attempted to read the string, and decide how to react from that info? But surely that can’t be the intended approach because that means the client can actually cause error messages in the server by sending invalid packets?

This is something I really want to avoid, because it suggests my server has an error despite that I specifically wrote code to handle invalid packets. Is there some sort of equivalent “bool TryReadFixedString64(out string result)” I could use, so I can decide how to handle invalid packets?

2

You can’t. You will only know after the fact. Which is why I would expect this method to throw an exception. If that is the case, you can assume something went wrong, be it malicious or otherwise you cannot know so the best thing to do here is to disconnect that client.

No! You only know that you have 4 more bytes to read. Those could be 4 literal bytes or a float or a short string or even just random noise (garbage).

If it’s not throwing exceptions (which may make sense since those come at a performance cost) then it sounds like this “failed” property is your way to check this.

You react by disconnecting the client. I wouldn’t even attempt to mitigate the issue. Client sent something incorrect - drop it.

By discarding such packets and dropping the client you guarantee server stability and that’s of the utmost importance!

It’s of course up to you to program this, as is the case with any and all I/O bound operations - be it file system or network messages or sql queries - all of them can fail so it is absolutely mandatory to design the code to handle failures appropriately.

I see a lot of devs ignoring this when they a) call an Unity Services without try/catch or b) do not implement the Netcode TransportFailure event - in that case the network shuts down for that client but the app would still remain in the connected state which is a terrible user experience (no info, can’t quit, can’t reconnect) and that gets your game negative reviews.

Again, there is no second guessing. If you receive something invalid from the client, invalidate that client! Kick its butt! It’s not supposed to happen and there must not be any second guessing as to why that occured.

3

You can’t. You will only know after the fact.

That’s why I’m asking for a “bool TryReadFixedString64(out string result)” that can return False if it wasn’t valid! This isn’t a try/catch, this is like any TryParse where it’s a given you’re not supposed to use the output if False was returned

No! You only know that you have 4 more bytes to read. Those could be 4 literal bytes or a float or a short string or even just random noise (garbage).

Yes! Any combination of four bytes is readable as a 32-bit integer, doesn’t matter what’s in them or how we got there! So for all intents and purposes, I’m reading an integer regardless of what those bits were meant to be. Then I work out from there if it’s a legal integer in the range I’m expecting or not with my own code, because Transport cannot do that for me. Same as working with any binary stream. Right? Any combination of four bytes will pass as an integer, so HasFailedReads will actually return False in this situation.

And yes, I’m dropping the connection if the number in the integer isn’t in valid range of what it’s used for.

Again, there is no second guessing. If you receive something invalid from the client, invalidate that client! Kick its butt! It’s not supposed to happen and there must not be any second guessing as to why that occured.

That’s why I want to verify all data I get. If something like "bool TryReadFixedString64(out string result)” was implemented, I’d drop the connection if any returns false. If it’s not false, I run additional checks to verify that the given data is consistent with what I’m expecting! If that doesn’t pass, I drop the connection too!

The problem is, the current functions logs an error on invalid reads even if I check for HasFailedReads after the fact. When I see an error, my reaction is “my code is wrong”, every single time, and it’ll be added to Player.log too, even if I’m handling the failed read correctly and dropping the connection as I should.

This is all this is about. Avoiding an error showing up in my log because something the client sends when nothing in the code is wrong and I handle all invalid packets appropriately.

4

If you really need this behavior, you can add the functionality yourself by modifying the source of DataStreamReader in whichever package you’re using (Unity Collections, or Unity Transport for older packages). The gist would be adding a method bool TryReadBytesInternal(byte*,int) that does the same work as void ReadBytesInternal(byte*,int) but doesn’t include error logging and returns true/false based on whether there’s enough data remaining, and adding related methods on top of that. Then the onus would be on you to maintain your modified version of the package…

1 Like
5

Yeah, that may totally work! I think that’s what I’m gonna have to do! Thank you!

Or, maybe I’ll exclusively use the methods that read/write bytes and write my own set of utilities to handle & verify what’s in them in a way I prefer.

6

I had a moment to check the code. If ReadFixedString has any issues reading the string data it will return null (it literally returns a ushort with the value 0 which is equivalent to null after casting it). This happens when the string length (first two bytes of the stream, a ushort) is not within a valid range.

So this boils down to performing a simple null check. :wink:

var str = stream.ReadFixedString64();
if (str == null)
    HandleStreamError();
7

But the problem is the error that’s written into the log when it’s invalid

I could check stream.HasFailedReads, that’s super easy! Or I could use your solution. But, I need to get rid of that error, or I’ll react like “there’s an error in my code” every time I see it no matter how well I handle it (also it stops Play Mode if Error Pause is enabled)

I’ll probs end up working with the bytes directly and ignore this part of Transport, that way I can implement my own patterns.

8

Integer 0 isn’t equal to null in C#. null can’t cast to an integer type, and boxed integers are non-null.

ReadFixedString64 returns FixedString64Bytes. Comparison to null is not appropriate, these types can only compare to non-null strings (exception thrown for null).

Empty (length 0) fixed string is indistinguishable from a DataStreamReader read failure. You need a HasFailedReads check when empty strings are allowed.

It seems like a choice between adding a bunch of methods to DataStreamReader and creating an independent type that does this. Upon further reflection, I think the latter is indeed a better way of going about things, but only when you’re in full control of the context that handles the data reading.

1 Like
9

It seems like a choice between adding a bunch of methods to DataStreamReader and creating an independent type that does this. Upon further reflection, I think the latter is indeed a better way of going about things, but only when you’re in full control of the context that handles the data reading.

Yeah! Good news is, I am :slight_smile: Dealing with the data will be 100% in my control, and I have a lot of experience with that part from writing my own binary serializer. I think I’ll stick to reading/writing my own bytes

1 Like