Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect handling of long team names #767

Open
Irup opened this issue Sep 2, 2018 · 1 comment
Open

Incorrect handling of long team names #767

Irup opened this issue Sep 2, 2018 · 1 comment
Labels
bug some feature is broken

Comments

@Irup
Copy link

Irup commented Sep 2, 2018

Entering a server with team names longer than 9 symbols causes incorrect behavior or a crash.

I investigated the problem after reading issue #737 which incorrectly identified the cause of the crash.

The issue is easy to fix, but perhaps the fix should not emulate the original client's handling, but accommodate these longer names if the server protocol should ever be altered further down the line; the 9 symbol name restriction is an arbitrary choice after all...

PySpades and PySnip load the full team name strings from their config files, and send them untruncated to clients.
The team names' packet normally looks like this:

0F1EC0C0 C0960000 00009642 6C756500 ###########Blue.
00000000 00526564 00000000 00000000 .....Red.......#
00050A00 00008043 00008043 0000803F ################
00008043 00008043 0000803F 0000EC42 ################
00008043 00005442 0000C543 00008043 ################
00005442                            ####

The buffers will always be 10 bytes long each if their contents fit.
But when they don't fit, they expand the packet:

0F1EC0C0 C0960000 00009654 65616D20 ###########Team 
31207465 7374696E 67206E61 6D650054 1 testing name.T
65616D20 32207465 7374696E 67206E61 eam 2 testing na
6D650000 00050A00 00008043 00008043 me.#############
0000803F 00008043 00008043 0000803F ################
0000EC42 00008043 00005442 0000C543 ################
00008043 00005442                   ########

No information is lost, however; it's just pushed further out to make room for the names.

Map objects are glitched if either team name buffers exceed 10 chars, but I can't say if this is a client-side or server-side problem.

Effects in clients:

This is safe:
"Team 1 na\0"
"Team 2 na\0"

This isn't:
"Team 1 nam\0" -- Team 2's name buffer terminates at char 0, meaning it has a blank name.
"Team 1 name\0" -- Team 1's name buffer bleeds into team 2's, making team 2's name "e". Note that this is "correct" behavior, as it's not dangerous and happens in the original client.
"Team 2 nam\0" -- Bleeds over null byte, but isn't problematic. the "m" is visible in OpenSpades, but not in the original client.
"Team 2 name\0" -- Expected final "e" is strangely not visible in OpenSpades.
"Team 2 namex\0" -- Immediate crash in OpenSpades.

Memory excerpt of the original client's packet buffer:

D94A56CC C9DD0000 D8959C03 80EFB003 ################
00FF0054 65616D20 31207465 7374696E ###Team 1 testin
67206E61 6D650054 65616D20 32207465 g name.Team 2 te
7374696E 67206E61 6D650000 00000000 sting name.#####
00003842 00809443 00004C42 0080FA43 ################

Memory excerpt of the permanent team name buffers after modifying the packet buffer's contents:

1C000000 1D000000 1E000000 1F000000 ################
5465616D 20312074 65007469 FF0000FF Team 1 te.ti####
74696E67 206E616D 65000000 00FF00FF ting name...####
53706563 7461746F 72000000 000000FF Spectator...####
00000000 00000000 00000000 00000000 ################

Indeed, the in-game result is "Team 1 te" for team 1, and "ting name" for team 2.
12 bytes, "Team 1 testi", was copied into team 1's buffer, then a null byte was placed into the tenth byte to make sure it's terminated, then the part that overlapped into team 2's buffer is given the same treatment.

Edited to remove stupid scary words like "code execution" and "undefined behavior" because I've since actually read the source code and seen how unfounded my initial assessment was, and I've fixed and added some information.

@NotAFile
Copy link
Contributor

NotAFile commented Sep 2, 2018

note that the length has been restricted in piqueserver. This has not been released yet though.

@feikname feikname added the bug some feature is broken label Oct 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug some feature is broken
Projects
None yet
Development

No branches or pull requests

3 participants