From fff87ed2d3791d6853529457cc37689e8869b270 Mon Sep 17 00:00:00 2001 From: Jeff Halter <868228+jhalter@users.noreply.github.com> Date: Thu, 9 Jul 2026 18:28:58 -0700 Subject: Publish ClientConn only after login and guard Account with the state mutex NewClientConn added the connection to the ClientManager before Account and Version were assigned, so any goroutine iterating ClientMgr.List() during the login handshake could dereference a nil Account (e.g. HandleSetUser reading c.Account.Login) and panic. Account was also read and written across goroutines with no synchronization: HandleSetUser wrote c.Account.Access on another client's connection while that client's own transaction loop read it in Authorize. The connection is now added to the manager in handleNewConnection only once Account, Version, UserName, and Flags are initialized, so a published client is always fully formed. Failed logins never publish the connection at all; Disconnect's manager delete is a no-op for them. Account joins the mutex-guarded session state with accessors in the established style: SetAccount/GetAccount, SetAccountAccess for the one post-login mutation, AccessBytes for building transaction fields, and Authorize now takes the read lock. All cross-goroutine call sites go through the accessors. HandleSetUser previously recomputed the admin flag from the client's stale access level and only converged on the following edit; the access update now happens before the recompute. --- hotline/client_conn.go | 55 ++++++++++++++++++++++++++++++++---- hotline/server.go | 18 +++++++----- hotline/server_test.go | 6 ++-- internal/mobius/api.go | 4 +-- internal/mobius/handlers_accounts.go | 10 +++---- internal/mobius/handlers_chat.go | 2 +- internal/mobius/handlers_session.go | 8 +++--- 7 files changed, 76 insertions(+), 27 deletions(-) diff --git a/hotline/client_conn.go b/hotline/client_conn.go index 3419e0a..fe4dc72 100644 --- a/hotline/client_conn.go +++ b/hotline/client_conn.go @@ -32,13 +32,13 @@ type ClientConn struct { ID ClientID Version []byte // TODO: make fixed size of 2 - Account *Account - Server *Server // TODO: consider adding methods to interact with server + Server *Server // TODO: consider adding methods to interact with server // The following fields hold mutable session state guarded by mu. They are read and written // by multiple goroutines (the client's own transaction loop, other clients' handlers, and the // server keepalive loop), so production code must use the accessor methods below. Direct // field access is only safe before the connection is shared, e.g. in tests. + Account *Account Flags UserFlags UserName []byte Icon []byte // TODO: make fixed size of 2 @@ -197,6 +197,45 @@ func (cc *ClientConn) GetAutoReply() []byte { return cc.AutoReply } +// SetAccount attaches the authenticated account to the client. +func (cc *ClientConn) SetAccount(account *Account) { + cc.mu.Lock() + defer cc.mu.Unlock() + + cc.Account = account +} + +// GetAccount returns the client's account, or nil before login completes. Every account field +// except Access is immutable after login, so callers may read them from the returned struct; +// Access must be read via Authorize or AccessBytes because SetAccountAccess mutates it. +func (cc *ClientConn) GetAccount() *Account { + cc.mu.RLock() + defer cc.mu.RUnlock() + + return cc.Account +} + +// SetAccountAccess replaces the access bitmap on the client's account. +func (cc *ClientConn) SetAccountAccess(access AccessBitmap) { + cc.mu.Lock() + defer cc.mu.Unlock() + + cc.Account.Access = access +} + +// AccessBytes returns a copy of the account's access bitmap, suitable for use as a transaction +// field. It returns nil before login completes. +func (cc *ClientConn) AccessBytes() []byte { + cc.mu.RLock() + defer cc.mu.RUnlock() + + if cc.Account == nil { + return nil + } + access := cc.Account.Access + return access[:] +} + // incrementIdleTime adds interval seconds to the client's idle time. It returns true if this // crossed the idle threshold and marked the client as away, in which case the caller should // notify other clients of the change. @@ -227,8 +266,8 @@ func (cc *ClientConn) clearIdleAndAway() bool { } func (cc *ClientConn) FileRoot() string { - if cc.Account.FileRoot != "" { - return cc.Account.FileRoot + if account := cc.GetAccount(); account != nil && account.FileRoot != "" { + return account.FileRoot } return cc.Server.Config.FileRoot } @@ -331,6 +370,9 @@ func (cc *ClientConn) Authenticate(login string, password []byte) bool { // Authorize checks if the user account has the specified permission func (cc *ClientConn) Authorize(access int) bool { + cc.mu.RLock() + defer cc.mu.RUnlock() + if cc.Account == nil { return false } @@ -424,11 +466,12 @@ func formatDownloadList(fts []FileTransfer) (s string) { } func (cc *ClientConn) String() string { + account := cc.GetAccount() template := fmt.Sprintf( userInfoTemplate, cc.GetUserName(), - cc.Account.Name, - cc.Account.Login, + account.Name, + account.Login, cc.RemoteAddr, formatDownloadList(cc.ClientFileTransferMgr.Get(FileDownload)), formatDownloadList(cc.ClientFileTransferMgr.Get(FolderDownload)), diff --git a/hotline/server.go b/hotline/server.go index 14cee04..700cf19 100644 --- a/hotline/server.go +++ b/hotline/server.go @@ -512,8 +512,6 @@ func (s *Server) NewClientConn(conn io.ReadWriteCloser, remoteAddr string) *Clie ClientFileTransferMgr: NewClientFileTransferMgr(), } - s.ClientMgr.Add(clientConn) - return clientConn } @@ -627,16 +625,17 @@ func (s *Server) handleNewConnection(ctx context.Context, rwc io.ReadWriteCloser c.SetIcon(clientLogin.GetField(FieldUserIconID).Data) } - c.Account = c.Server.AccountManager.Get(login) - if c.Account == nil { + account := c.Server.AccountManager.Get(login) + if account == nil { return nil } + c.SetAccount(account) if clientLogin.GetField(FieldUserName).Data != nil { if c.Authorize(AccessAnyName) { c.SetUserName(clientLogin.GetField(FieldUserName).Data) } else { - c.SetUserName([]byte(c.Account.Name)) + c.SetUserName([]byte(account.Name)) } } @@ -644,6 +643,11 @@ func (s *Server) handleNewConnection(ctx context.Context, rwc io.ReadWriteCloser c.SetFlag(UserFlagAdmin, 1) } + // Publish the client to the manager only now that its session state (Account, Version, + // UserName, Flags) is fully initialized. Other goroutines iterate ClientMgr.List() and + // dereference Account, so a client must never be visible before login completes. + s.ClientMgr.Add(c) + c.Send(c.NewReply(&clientLogin, NewField(FieldVersion, []byte{0x00, 0xbe}), NewField(FieldCommunityBannerID, []byte{0, 0}), @@ -651,7 +655,7 @@ func (s *Server) handleNewConnection(ctx context.Context, rwc io.ReadWriteCloser )) // Send user access privs so client UI knows how to behave - c.Send(NewTransaction(TranUserAccess, c.ID, NewField(FieldUserAccess, c.Account.Access[:]))) + c.Send(NewTransaction(TranUserAccess, c.ID, NewField(FieldUserAccess, c.AccessBytes()))) // Accounts with AccessNoAgreement do not receive the server agreement on login. The behavior is different between // client versions. For 1.2.3 client, we do not send TranShowAgreement. For other client versions, we send @@ -746,7 +750,7 @@ func (s *Server) handleFileTransfer(ctx context.Context, rwc io.ReadWriter) erro } rLogger := s.Logger.With( "remoteAddr", remoteAddr, - "login", fileTransfer.ClientConn.Account.Login, + "login", fileTransfer.ClientConn.GetAccount().Login, "Name", string(fileTransfer.ClientConn.GetUserName()), ) diff --git a/hotline/server_test.go b/hotline/server_test.go index ebcd073..a9842c8 100644 --- a/hotline/server_test.go +++ b/hotline/server_test.go @@ -15,7 +15,6 @@ import ( "time" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "golang.org/x/text/encoding" "golang.org/x/text/encoding/charmap" @@ -831,8 +830,11 @@ type nopCloserRWC struct { func (n *nopCloserRWC) Close() error { return nil } func TestServer_NewClientConn(t *testing.T) { + // The mock has no Add expectation on purpose: NewClientConn must NOT publish the connection + // to the ClientManager. Publication happens later in handleNewConnection, once the session + // state (Account, Version, UserName, Flags) is fully initialized, so that other goroutines + // never observe a partially initialized client. mockMgr := &MockClientMgr{} - mockMgr.On("Add", mock.AnythingOfType("*hotline.ClientConn")).Return() srv := &Server{ClientMgr: mockMgr} diff --git a/internal/mobius/api.go b/internal/mobius/api.go index 3620cf4..c14c8bf 100644 --- a/internal/mobius/api.go +++ b/internal/mobius/api.go @@ -110,7 +110,7 @@ func (srv *APIServer) OnlineHandler(w http.ResponseWriter, r *http.Request) { } else { for _, c := range srv.hlServer.ClientMgr.List() { users = append(users, map[string]string{ - "login": string(c.Account.Login), + "login": c.GetAccount().Login, "nickname": string(c.GetUserName()), "ip": c.RemoteAddr, }) @@ -164,7 +164,7 @@ func (srv *APIServer) BanHandler(w http.ResponseWriter, r *http.Request) { // Disconnect user if online for _, c := range srv.hlServer.ClientMgr.List() { - if (req.Username != "" && c.Account.Login == req.Username) || + if (req.Username != "" && c.GetAccount().Login == req.Username) || (req.Nickname != "" && string(c.GetUserName()) == req.Nickname) || (req.IP != "" && c.IP() == req.IP) { c.Disconnect() diff --git a/internal/mobius/handlers_accounts.go b/internal/mobius/handlers_accounts.go index 02b7c77..5014d84 100644 --- a/internal/mobius/handlers_accounts.go +++ b/internal/mobius/handlers_accounts.go @@ -55,18 +55,18 @@ func HandleSetUser(cc *hotline.ClientConn, t *hotline.Transaction) (res []hotlin // Notify connected clients logged in as the user of the new access level for _, c := range cc.Server.ClientMgr.List() { - if c.Account.Login == login { + if c.GetAccount().Login == login { newT := hotline.NewTransaction(hotline.TranUserAccess, c.ID, hotline.NewField(hotline.FieldUserAccess, newAccessLvl)) res = append(res, newT) + c.SetAccountAccess(account.Access) + if c.Authorize(hotline.AccessDisconUser) { c.SetFlag(hotline.UserFlagAdmin, 1) } else { c.SetFlag(hotline.UserFlagAdmin, 0) } - c.Account.Access = account.Access - cc.SendAll( hotline.TranNotifyChangeUser, hotline.NewField(hotline.FieldUserID, c.ID[:]), @@ -200,7 +200,7 @@ func HandleUpdateUser(cc *hotline.ClientConn, t *hotline.Transaction) (res []hot } for _, client := range cc.Server.ClientMgr.List() { - if client.Account.Login == login { + if client.GetAccount().Login == login { // "You are logged in with an account which was deleted." res = append(res, @@ -374,7 +374,7 @@ func HandleDeleteUser(cc *hotline.ClientConn, t *hotline.Transaction) (res []hot } for _, client := range cc.Server.ClientMgr.List() { - if client.Account.Login == login { + if client.GetAccount().Login == login { res = append(res, hotline.NewTransaction(hotline.TranServerMsg, client.ID, hotline.NewField(hotline.FieldData, []byte(ErrMsgAccountDeleted)), diff --git a/internal/mobius/handlers_chat.go b/internal/mobius/handlers_chat.go index b81cfa5..ec92a7c 100644 --- a/internal/mobius/handlers_chat.go +++ b/internal/mobius/handlers_chat.go @@ -62,7 +62,7 @@ func HandleChatSend(cc *hotline.ClientConn, t *hotline.Transaction) (res []hotli //cc.Server.mux.Lock() for _, c := range cc.Server.ClientMgr.List() { - if c == nil || cc.Account == nil { + if c == nil || cc.GetAccount() == nil { continue } // Skip clients that do not have the read chat permission. diff --git a/internal/mobius/handlers_session.go b/internal/mobius/handlers_session.go index 443fed0..836c608 100644 --- a/internal/mobius/handlers_session.go +++ b/internal/mobius/handlers_session.go @@ -102,11 +102,11 @@ func HandleTranAgreed(cc *hotline.ClientConn, t *hotline.Transaction) (res []hot if cc.Authorize(hotline.AccessAnyName) { cc.SetUserName(t.GetField(hotline.FieldUserName).Data) } else { - cc.SetUserName([]byte(cc.Account.Name)) + cc.SetUserName([]byte(cc.GetAccount().Name)) } } - login := cc.Account.Login + login := cc.GetAccount().Login ip := cc.IP() if cc.Server.Presence != nil { @@ -191,7 +191,7 @@ func HandleDisconnectUser(cc *hotline.ClientConn, t *hotline.Transaction) (res [ } if clientConn.Authorize(hotline.AccessCannotBeDiscon) { - return cc.NewErrReply(t, clientConn.Account.Login+" is not allowed to be disconnected.") + return cc.NewErrReply(t, clientConn.GetAccount().Login+" is not allowed to be disconnected.") } // If FieldOptions is set, then the client IP is banned in addition to disconnected. @@ -266,7 +266,7 @@ func HandleSetClientUserInfo(cc *hotline.ClientConn, t *hotline.Transaction) (re newNickname := string(t.GetField(hotline.FieldUserName).Data) cc.SetUserName(t.GetField(hotline.FieldUserName).Data) - login := cc.Account.Login + login := cc.GetAccount().Login ip := cc.IP() if cc.Server.Presence != nil { -- cgit