kinnay / SMB35

An open source server for Super Mario Bros. 35
GNU Affero General Public License v3.0
180 stars 24 forks source link

Disconnected players stay in the game, become "immortal", and defeat all players #6

Closed just-another-random-person closed 3 years ago

just-another-random-person commented 3 years ago

We noticed in several games, some people would be frozen in place, never time out, and beat us all after we reach fastest red timer and time out.

At first we thought it was a hacker, until one of us got a connection error and also had the same result.

Now we can reproduce it reliably: Steps to reproduce:

  1. Have 2 or more players join a lobby, then wait until the game starts
  2. One player presses the home button, closes the game by pressing X.
  3. That player is now "immortal" in the game, will never run out of time, and cannot be beaten by the remaining players.

This is the error that appears once they close the game: Screen Shot 2021-05-30 at 6 15 46 PM

And of course, this initiates the ClosedResourceError stacktrace spam until the end of the game Screen Shot 2021-05-30 at 6 17 52 PM

kinnay commented 3 years ago

Yeah, removing disconnected nodes will probably fix this. We will also have to send a node notice packet to all other nodes to inform them that a node was removed.

I think we should also send a node notice packet when a new node joins the session, but that's probably unrelated to this issue.

just-another-random-person commented 3 years ago

So I've been trying my hand at this...
After all, all I need is to make all other clients realize one disconnected, somehow, and they should mark it dead themselves... right?

So I did 3 types of attempts, first was to broadcast a Disconnect RPC packet, with the source node_id set to the disconnected client's.
Maybe they'll interpret it as "the node with source node_id is disconnecting, we should mark the player dead".

I thought I'll just do this for everyone who gets marked as disconnected, maybe they'll automatically get marked as dead.

Screen Shot 2021-06-02 at 3 17 44 AM

Just modified it a bit to allow empty payloads (as the disconnect packet has)

Screen Shot 2021-06-02 at 3 17 57 AM

Catch the usual stacktrace culprit when someone disconnects

Screen Shot 2021-06-02 at 3 18 10 AM

And just copy relay_rpc logic to broadcast the packet

Screen Shot 2021-06-02 at 3 19 54 AM

.... and it ended up disconnecting everyone when someone exited out of the game. I guess the disconnect packet just makes the client disconnect, irregardless of the node id in the source field.

just-another-random-person commented 3 years ago

So I tried to instead just remove the node, and broadcast a node notice packet that it was removed, as originally recommended.

Screen Shot 2021-06-02 at 3 22 08 AM

Screen Shot 2021-06-02 at 3 22 14 AM

Added a send_node_notice method to EagleClient

Screen Shot 2021-06-02 at 3 22 35 AM

Catching the usual stacktrace culprit again when someone disconnects

Screen Shot 2021-06-02 at 3 25 56 AM

And copying the relay_rpc logic again to broadcast the node notice

Screen Shot 2021-06-02 at 3 26 37 AM

Now I'm sure the methods are being called, I filled the entire thing with logger.info calls everywhere.

But nothing happens. No idea what I'm doing wrong (probably a lot), but I'm trying to cut corners just to make it work, and I've never coded in Python before.

But yeah, everything gets called but nothing happens in game.

just-another-random-person commented 3 years ago

Final attempt was to just try to directly kill the player that disconnected by broadcasting a "Dead" Mario 35 RPC packet.

Added a few new properties to hold some necessary data:

Screen Shot 2021-06-02 at 3 27 05 AM

Screen Shot 2021-06-02 at 3 27 16 AM

Screen Shot 2021-06-02 at 3 27 29 AM

Screen Shot 2021-06-02 at 3 27 36 AM

Grab the data from the "AddPlayer" Mario 35 RPC Packet (as it seems to be the only packet that will have one each for all players)

Screen Shot 2021-06-02 at 3 27 43 AM

Screen Shot 2021-06-02 at 3 28 13 AM

Screen Shot 2021-06-02 at 3 27 52 AM

Now I know the RPC packets are relayed, with the correct node IDs, by littering the code with logger.info's.
I was even able to grab the player's names (the ascii ones anyway, I've tried a few encodings with Japanese symbols and others, and I can't seem to get it).

But still nothing happens when the "Dead" packet is broadcasted.

At first I was using zeroes for all the "unknown" values, but eventually, I modified some to look like the values the game actually sends when someone dies. Still nothing, either there's a checksum I'm failing or I'm misunderstanding something fundamental on how this all works.

(sorry it's all a mess, not as concise or elegant as your original code, but yeah, my first time with Python and rushing to get this to work)

kinnay commented 3 years ago

Added a send_node_notice method to EagleClient

The source node should be set to 0 in the node notice packet. Instead, the node id of the disconnected client is part of the packet payload. It's 16 bits before the server time field. The game is probably ignoring your node notice packet because you forgot this field.

just-another-random-person commented 3 years ago

Totally overlooked that, thanks.

So I changed it to this:

Screen Shot 2021-06-02 at 5 11 33 AM

And now I get error code 2623-0001 when the other player closes their game.

Pretty stumped, I'll try to tinker with the available options some more, but I think I've gone as far as I can go without any reverse engineering.

just-another-random-person commented 3 years ago

So I tried to do it properly:

change 1 Screen Shot 2021-06-04 at 10 03 46 PM change 2 Screen Shot 2021-06-04 at 10 04 29 PM change 3 Screen Shot 2021-06-04 at 10 04 49 PM

Basically these are the new changes (aside from the other ones above already).

Still the same problem, error code 2623-0001 when the other player closes their game. Not sure but is it possible the original server explicitly kills off the disconnected players by sending a "Dead" packet? The last 5 bits seem to be some kind of checksum, otherwise the "Dead" packet gets ignored.

Also, the original server's behavior is different for disconnected clients:
players that die normally [not KOd, but from normal enemies or pits] have half their coins go to the "prize pot"
players that disconnect have ALL their coins go to the "prize pot"

This makes me realize, it may be possible the last few bits of the "Dead" packet refers to the player ID of who killed said player. I'll try to experiment with that angle instead a bit more.

kinnay commented 3 years ago

I pushed a fix. When I tested it the player that disconnected died immediately. Can you let me know if it works for you?

just-another-random-person commented 3 years ago

It works!

I can't believe the only missing thing was that the node notice (for all nodes [type=4], or node added [type=0])
had to be sent AFTER client ready.

I even tried it in my code above, simply moved the 2 lines that relay node notice type 0 and 4 from EagleClient.process() to process_client_ready() and it also worked!

Of course, your code is much cleaner and more concise. Thanks for the fix!