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.

Sign up for GitHub

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

Support empty team names #381

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Support empty team names #381

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion squad-server/rcon.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ export default class SquadRcon extends Rcon {
const match = line.match(
/ID: (?<squadID>\d+) \| Name: (?<squadName>.+) \| Size: (?<size>\d+) \| Locked: (?<locked>True|False) \| Creator Name: (?<creatorName>.+) \| Creator Online IDs:([^|]+)/
);
const matchSide = line.match(/Team ID: (\d) \((.+)\)/);
const matchSide = line.match(/Team ID: (\d) \((.*)\)/);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Do we have some log examples for this case? Would like to see this in action.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have specific log examples available at hand, but it did cause issues with Supermod where there were 3 teams: (Civilians), (), (Armed Forces). This specific entry is from the RCON, not the logs. Let me see if I can reproduce it, but we've since taken down that server.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, this does not happen with vanilla Squad, but is possible with modded servers.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is edge cases where it may break running vanilla servers if that is a possibility.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have the exact message, but this is what I wrote down in my notes:

Team ID: 0 (Civilians)
Team ID: 1 ()
Team ID: 2 (Armed Forces)

It was incorrectly assigning players to the wrong team. Team 1 being skipped (not matching), they were incorrectly assigned to team 0.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh gotcha. Would still need it to get the regex test out of the way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually remember such cases in vanilla too about a year ago. It was causing issues with BMs parsing. They maght have fixed it but I'm not sure. My intuition is this happens if ListPlayers is requested while someone has just joined and is still in Spectate state. so a way to repro would be to ask someone join the server while spamming ListPlayers requests.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is that it won't break normal use. I don't think so but would rather be safe.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly don't see how it would break it. It's a separate match. In case of current regex we would get an undefined if the field is actually empty. In case of this fix it would work in all cases. But it's worth testing if you are concerned.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, thinking about it a bit more... Since it's a separate regex that could fail matching - we'd be better off simply handling the non-matching case. Changing to a new regex from this PR would require us to check for an empty group anyways so there is literally no point in applying this change.

if (matchSide) {
teamID = +matchSide[1];
teamName = matchSide[2];
Expand Down