diff options
| author | Jeff Halter <868228+jhalter@users.noreply.github.com> | 2026-06-12 08:47:54 -0700 |
|---|---|---|
| committer | Jeff Halter <868228+jhalter@users.noreply.github.com> | 2026-06-12 08:47:54 -0700 |
| commit | c15f8510fbd5ccf9a122f88d6e975d1ef69e00b3 (patch) | |
| tree | 1ba7e392610637ed792d0003568cf9edb6adc526 /hotline/client_conn_test.go | |
| parent | b2c462a3a1353f0653a5964b3a6924538ce83523 (diff) | |
Fix data races on ClientConn state, banner reload, and rate limiter growth
ClientConn's mutable session state (Flags, UserName, Icon, IdleTime,
AutoReply) was guarded inconsistently: two mutexes (FlagsMU and mu)
covered some paths while others mutated or read the fields with no
locking at all, including HandleSetClientUserInfo, HandleUpdateUser
(which writes other clients' admin flag), the login flow, the HTTP API
handlers, and the keepalive loop. Consolidate on a single mutex with
accessor methods (SetFlag/IsFlagSet/FlagBytes, SetUserName/GetUserName,
and so on) used by all production code; direct field access remains for
test construction. The idle/away logic moves into incrementIdleTime
and clearIdleAndAway helpers that report whether a notification is
needed, so SendAll is no longer called while holding the lock.
HandleRejectChatInvite also no longer appends to the username slice,
which could write past its length into the backing buffer.
The server banner is now behind Banner/SetBanner with an RWMutex: the
SIGHUP reload previously reassigned the field while banner download
goroutines read it, and nilled it when the file read failed. Reload
now keeps the previous banner on failure.
Per-IP rate limiter entries now record a last-seen time, and the
keepalive ticker evicts entries idle for over seven days, so the map
no longer grows unboundedly with each unique client IP.
Diffstat (limited to 'hotline/client_conn_test.go')
| -rw-r--r-- | hotline/client_conn_test.go | 57 |
1 files changed, 57 insertions, 0 deletions
diff --git a/hotline/client_conn_test.go b/hotline/client_conn_test.go index 5c5463d..3312954 100644 --- a/hotline/client_conn_test.go +++ b/hotline/client_conn_test.go @@ -660,3 +660,60 @@ func TestClientConn_SendDisconnectRace(t *testing.T) { wg.Wait() } + +func TestClientConn_incrementIdleTime(t *testing.T) { + cc := &ClientConn{} + + // Increment until just below the idle threshold: not yet away. + for i := 0; i < userIdleSeconds/idleCheckInterval; i++ { + assert.False(t, cc.incrementIdleTime(idleCheckInterval)) + } + assert.False(t, cc.IsFlagSet(UserFlagAway)) + + // The increment that crosses the threshold marks the client away exactly once. + assert.True(t, cc.incrementIdleTime(idleCheckInterval)) + assert.True(t, cc.IsFlagSet(UserFlagAway)) + assert.False(t, cc.incrementIdleTime(idleCheckInterval), "already-away client should not be marked away again") +} + +func TestClientConn_clearIdleAndAway(t *testing.T) { + cc := &ClientConn{IdleTime: 500} + + // Not away: idle timer resets, no notification needed. + assert.False(t, cc.clearIdleAndAway()) + assert.Equal(t, 0, cc.IdleTime) + + // Away: flag clears and the caller is told to notify. + cc.SetFlag(UserFlagAway, 1) + assert.True(t, cc.clearIdleAndAway()) + assert.False(t, cc.IsFlagSet(UserFlagAway)) + assert.False(t, cc.clearIdleAndAway(), "second clear should report no change") +} + +// TestClientConn_sessionStateRace exercises concurrent access to the mutable session state through +// the accessor methods. Run with -race. +func TestClientConn_sessionStateRace(t *testing.T) { + cc := &ClientConn{} + + var wg sync.WaitGroup + for range 4 { + wg.Add(1) + go func() { + defer wg.Done() + for i := range 100 { + cc.SetUserName(fmt.Appendf(nil, "user-%d", i)) + _ = cc.GetUserName() + cc.SetIcon([]byte{0, byte(i)}) + _ = cc.GetIcon() + cc.SetAutoReply([]byte("brb")) + _ = cc.GetAutoReply() + cc.SetFlag(UserFlagRefusePM, uint(i%2)) + _ = cc.IsFlagSet(UserFlagRefusePM) + _ = cc.FlagBytes() + _ = cc.incrementIdleTime(idleCheckInterval) + _ = cc.clearIdleAndAway() + } + }() + } + wg.Wait() +} |