Skip to content

Send all the player options in auto lobby when host receives them #6995

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

Open
Garanas wants to merge 2 commits into
base: develop
Choose a base branch
from
+9 −0
Open

Send all the player options in auto lobby when host receives them #6995

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2eac3f
Send all the player options upon receiving them
Garanas Jan 6, 2026
7605e8b
Fix overshadowing local
Garanas Jan 6, 2026
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
9 changes: 9 additions & 0 deletions lua/ui/lobby/autolobby/AutolobbyController.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,6 +624,15 @@ AutolobbyCommunications = Class(MohoLobbyMethods, AutolobbyServerCommunicationsC
-- put the player where it belongs
self.PlayerOptions[playerOptions.StartSpot] = playerOptions

-- send all player options to the server, this is necessary to do immediately (for the host) so that the server knows who is (not) connected.
for slot, playerOptions in self.PlayerOptions do
local ownerId = playerOptions.OwnerID
self:SendPlayerOptionToServer(ownerId, 'Team', playerOptions.Team)
self:SendPlayerOptionToServer(ownerId, 'Army', playerOptions.StartSpot)
self:SendPlayerOptionToServer(ownerId, 'StartSpot', playerOptions.StartSpot)
self:SendPlayerOptionToServer(ownerId, 'Faction', playerOptions.Faction)
end
Comment on lines 627 to 634
Copy link

@coderabbitai coderabbitai bot Jan 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, let's locate and examine the AutolobbyController.lua file
fd -name "AutolobbyController.lua" -type f

Repository: FAForever/fa

Length of output: 286

🏁 Script executed:

#!/bin/bash
# Locate the file with correct syntax
fd --name "AutolobbyController.lua" --type f

Repository: FAForever/fa

Length of output: 298

🏁 Script executed:

#!/bin/bash
# Find the file with correct fd syntax
fd "AutolobbyController.lua"

Repository: FAForever/fa

Length of output: 102

🏁 Script executed:

#!/bin/bash
# Read the section around lines 627-634 with more context
echo "=== Lines 600-650 context ==="
sed -n '600,650p' lua/ui/lobby/autolobby/AutolobbyController.lua | cat -n

echo ""
echo "=== Lines 560-580 for LaunchThread comparison ==="
sed -n '560,580p' lua/ui/lobby/autolobby/AutolobbyController.lua | cat -n

Repository: FAForever/fa

Length of output: 4321

🏁 Script executed:

#!/bin/bash
echo "=== Searching for SendPlayerOptionToServer calls with 'Army' ==="
rg "SendPlayerOptionToServer.*Army" -n -B2 -A2 lua/

echo ""
echo "=== Searching for server-side player option handling ==="
rg "set_player_option|_configured_player|Army" -n lua/ui/lobby/autolobby/ | head -40

echo ""
echo "=== Check if there are other places that send all player options ==="
rg "for.*PlayerOptions.*do" -n lua/ui/lobby/autolobby/AutolobbyController.lua

Repository: FAForever/fa

Length of output: 2538

🏁 Script executed:

#!/bin/bash
echo "=== Reading AutolobbyServerCommunicationsComponent.lua around SendPlayerOptionToServer ==="
cat -n lua/ui/lobby/autolobby/components/AutolobbyServerCommunicationsComponent.lua | head -150

Repository: FAForever/fa

Length of output: 8540

🏁 Script executed:

#!/bin/bash
echo "=== Search for Army-related comments or documentation ==="
rg -i "army" lua/ui/lobby/autolobby/ -B2 -A2 | head -80

echo ""
echo "=== Check git history or comments around the ProcessAddPlayerMessage change ==="
rg -B5 -A5 "ProcessAddPlayerMessage" lua/ui/lobby/autolobby/AutolobbyController.lua | head -60

Repository: FAForever/fa

Length of output: 5447

Fix Army value inconsistency: use calculated army index, not StartSpot.

This implementation has critical issues:

  1. Army value bug: Line 631 sends playerOptions.StartSpot as the Army value, but LaunchThread (line 562) explicitly documents that "Army numbers need to be calculated: they are numbered incrementally in slot order" and correctly uses a calculated armyIndex at line 574. Sending StartSpot as Army will cause the server to receive inconsistent and incorrect Army values.

  2. Inefficient synchronization: The loop sends ALL player options every time ANY single player is added, resulting in O(N²) messages for N sequential players (1+2+3+...+N = 36 messages for an 8-player game instead of 8).

  3. Variable shadowing: The loop variable playerOptions (line 628) shadows the function parameter playerOptions (line 618), reducing clarity.

If the intent is to ensure the server knows about all connected players early, that's reasonable. However, the Army value must be calculated consistently. Consider sending only the newly added player's options, or if all-player sync is necessary, compute Army values the same way as LaunchThread does.

💡 Suggested fix if sending only the new player 
-        -- send all player options to the server, this is necessary to do immediately (for the host) so that the server knows who is (not) connected. 
-        for slot, playerOptions in self.PlayerOptions do
-            local ownerId = playerOptions.OwnerID
-            self:SendPlayerOptionToServer(ownerId, 'Team', playerOptions.Team)
-            self:SendPlayerOptionToServer(ownerId, 'Army', playerOptions.StartSpot)
-            self:SendPlayerOptionToServer(ownerId, 'StartSpot', playerOptions.StartSpot)
-            self:SendPlayerOptionToServer(ownerId, 'Faction', playerOptions.Faction)
-        end
+        -- Inform the server about the newly added player's options immediately
+        -- so the server can mark them as configured and avoid incorrect violation bans
+        local ownerId = playerOptions.OwnerID
+        self:SendPlayerOptionToServer(ownerId, 'Team', playerOptions.Team)
+        self:SendPlayerOptionToServer(ownerId, 'Army', playerOptions.StartSpot)
+        self:SendPlayerOptionToServer(ownerId, 'StartSpot', playerOptions.StartSpot)
+        self:SendPlayerOptionToServer(ownerId, 'Faction', playerOptions.Faction)

Note: If sending only the new player, you still need to compute the correct Army index. This requires calculating the position of this player in the sorted slot order, similar to LaunchThread lines 563-576.

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Member Author

@Garanas Garanas Jan 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

This is actually a good point 😃 ! How do we know what the actual start spot is for a player? We can't exactly trust on the user input here.

Copy link
Member Author

@Garanas Garanas Jan 6, 2026

Choose a reason for hiding this comment

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

The suggested fix is wrong, since then the host would never sent their own player options with the current changes.

Copy link

@coderabbitai coderabbitai bot Jan 6, 2026

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for StartSpot validation and slot management
echo "=== Looking for StartSpot validation patterns ==="
rg -n "StartSpot" lua/ui/lobby/autolobby/ -B2 -A2 | head -80

echo ""
echo "=== Looking for slot occupancy checks ==="
rg -n "occupied|slot.*check|validate.*slot" lua/ui/lobby/autolobby/ -i

echo ""
echo "=== Looking for PlayerOptions bounds or validation ==="
rg -n "PlayerOptions\[" lua/ui/lobby/autolobby/AutolobbyController.lua -B1 -A1

echo ""
echo "=== Checking PlayerCount usage for bounds ==="
rg -n "PlayerCount" lua/ui/lobby/autolobby/AutolobbyController.lua | head -20

Length of output: 7969

@Garanas, excellent security question! You're absolutely right to be concerned about trusting client-provided StartSpot values. Currently, no validation exists — the code directly uses the client's StartSpot as an array index at line 625, which is unsafe.

Missing Validations

The TODO comment at line 623 flags this, but here's what needs to be validated before line 625:

-- Validate StartSpot before using it
local startSpot = playerOptions.StartSpot

-- 1. Type and existence check
if type(startSpot) ~= 'number' then
    self:DebugWarn("Invalid StartSpot type from", data.SenderID)
    -- Reject or assign a free slot
    return
end

-- 2. Bounds check (must be within valid player slots)
if startSpot < 1 or startSpot > self.PlayerCount then
    self:DebugWarn("StartSpot out of bounds from", data.SenderID, startSpot, self.PlayerCount)
    -- Reject or assign a free slot
    return
end

-- 3. Occupancy check (slot must not be taken)
if self.PlayerOptions[startSpot] ~= nil then
    self:DebugWarn("StartSpot already occupied:", startSpot, "by", self.PlayerOptions[startSpot].PlayerName)
    -- Reject or assign a free slot
    return
end

Host Authority Model

Since this is ProcessAddPlayerMessage (which only the host receives), the host should have full authority to:

  • Validate client requests
  • Assign slots deterministically if the client's request is invalid
  • Potentially find the first available slot and assign it

The autolobby appears to be designed for matchmaker-controlled games where slot assignments come from the server via command-line args, but validation is still needed for defensive programming.

The Original Army Value Bug

Separately, the critical issue from my original comment remains: line 631 sends playerOptions.StartSpot as the Army value, but LaunchThread (lines 563-576) correctly calculates Army as an incrementally numbered index in sorted slot order. This inconsistency will cause the server to receive wrong Army values, regardless of whether StartSpot validation is added.

Both issues need fixing:

  1. Add StartSpot validation (security/correctness)
  2. Fix Army value to use calculated index, not StartSpot (correctness/consistency)

Would you like me to provide a more complete code suggestion that addresses both concerns?

🧬 Code Graph Analysis Results

engine/User.lua

  • Snippet: HasCommandLineArg(option)
    • Description: Function signature for checking if a command-line option is present. Relevant to how AutolobbyController.lua reads command-line arguments (e.g., /startspot) to set local player StartSpot.
    • Full snippet:
    function HasCommandLineArg(option)
end
    • Context: Used by code paths that parse startup arguments to configure local player options.

lua/simInit.lua

  • Snippet: WaitSeconds(n)
    • Description: Basic timing helper that yields for a number of seconds. Relevant to understanding timing-based behavior in autolobby threads (e.g., rejoin checks, status broadcasting).
    • Full snippet:
    function WaitSeconds(n)
    if n <= 0.1 then
        WaitTicks(1)
        return
    end
    WaitTicks(n * 10 + 1)
end
    • Notes: Used to pause coroutines/threads; informs how long certain launch/retry sequences wait.

lua/userInit.lua

  • Snippet: WaitSeconds(n)
    • Description: Enhanced timing helper with frame/time measurement to drive waits, including dynamic calculation of elapsed time and FPS. Relevant for understanding timing behavior in autolobby threads and rejoin logic.
    • Full snippet:
    function WaitSeconds(n)
    local start = CurrentTime()
    local elapsed_frames = 0
    local elapsed_time = 0
    local wait_frames

    repeat
        wait_frames = math.ceil(math.max(1, AvgFPS * 0.1, n * AvgFPS))
        WaitFrames(wait_frames)
        elapsed_frames = elapsed_frames + wait_frames
        elapsed_time = CurrentTime() - start
    until elapsed_time >= n

    if elapsed_time >= 3 then
        AvgFPS = math.max(10, math.min(200, math.ceil(elapsed_frames / elapsed_time)))
    end
end
  • Snippet: WaitSeconds(n) (alternative minimal definition)
    • Description: Another definition of WaitSeconds used in this file; helps contrast simple vs. FPS-aware waiting.
    • Full snippet:
    function WaitSeconds(n)
    local start = CurrentTime()
    local elapsed_time = 0
    while elapsed_time < n do
        local dt = WaitTicks(1)
        elapsed_time = CurrentTime() - start
    end
end
    • Notes: Illustrates timing logic that could affect how startup/startspot decisions propagate in autolobby.

Copy link

@coderabbitai coderabbitai bot Jan 6, 2026

Choose a reason for hiding this comment

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

@Garanas You're absolutely right! I apologize for the incorrect suggestion. The host creates their own player options in the Hosting function but never goes through ProcessAddPlayerMessage, so they would indeed never send their options if we only sent the newly added player.

The loop sending all player options makes sense — it ensures the server has complete information about all connected players (including the host) whenever anyone joins.

However, the Army value bug remains critical: Line 631 still sends playerOptions.StartSpot as the Army value, but Army indices must be calculated incrementally in sorted slot order (as LaunchThread correctly does on lines 563-576).

Here's a corrected fix that preserves the all-player synchronization while fixing the Army calculation:

-- send all player options to the server, this is necessary to do immediately (for the host) so that the server knows who is (not) connected. 
-- Army numbers need to be calculated: they are numbered incrementally in slot order.
local slots = {}
for slotIndex, _ in pairs(self.PlayerOptions) do
    table.insert(slots, slotIndex)
end
table.sort(slots)

for armyIndex, slotIndex in ipairs(slots) do
    local playerOpts = self.PlayerOptions[slotIndex]
    local ownerId = playerOpts.OwnerID
    self:SendPlayerOptionToServer(ownerId, 'Team', playerOpts.Team)
    self:SendPlayerOptionToServer(ownerId, 'Army', armyIndex)  -- use calculated armyIndex, not StartSpot
    self:SendPlayerOptionToServer(ownerId, 'StartSpot', playerOpts.StartSpot)
    self:SendPlayerOptionToServer(ownerId, 'Faction', playerOpts.Faction)
end

This also addresses the variable shadowing issue by using playerOpts in the loop instead of reusing playerOptions.

Regarding your earlier question about trusting start spots: that's a good security concern. The current code accepts whatever StartSpot the joining player claims (line 624). Should there be validation to ensure the StartSpot is not already occupied and within valid range [1, PlayerCount]?


-- sync game options with the connected peer
self:SendData(data.SenderID, { Type = "UpdateGameOptions", GameOptions = self.GameOptions })

Expand Down
Loading