diff --git a/CHANGELOG.md b/CHANGELOG.md index 30928e179..7dd7352ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Race condition in stage broadcast causing nil pointer panics during player logout - Client crash when loading decoration presets (decomyset) with more than 40 entries +- Config file handling and validation +- Fixes 3 critical race conditions in handlers_stage.go +- Fix an issue causing a crash on clans with 0 members +- Fixed deadlock in zone change causing 60-second timeout when players change zones +- Fixed crash when sending empty packets in QueueSend/QueueSendNonBlocking +- Fixed missing stage transfer packet for empty zones + +### Security + +- Bumped golang.org/x/net from 0.33.0 to 0.38.0 +- Bumped golang.org/x/crypto from 0.31.0 to 0.35.0 ## [9.2.0] - 2023-04-01 diff --git a/schemas/patch-schema/27-fix-character-defaults.sql b/schemas/patch-schema/27-fix-character-defaults.sql new file mode 100644 index 000000000..55f9fb4d0 --- /dev/null +++ b/schemas/patch-schema/27-fix-character-defaults.sql @@ -0,0 +1,15 @@ +BEGIN; + +-- Initialize otomoairou (mercenary data) with default empty data for characters that have NULL or empty values +-- This prevents error logs when loading mercenary data during zone transitions +UPDATE characters +SET otomoairou = decode(repeat('00', 10), 'hex') +WHERE otomoairou IS NULL OR length(otomoairou) = 0; + +-- Initialize platemyset (plate configuration) with default empty data for characters that have NULL or empty values +-- This prevents error logs when loading plate data during zone transitions +UPDATE characters +SET platemyset = decode(repeat('00', 1920), 'hex') +WHERE platemyset IS NULL OR length(platemyset) = 0; + +COMMIT; diff --git a/server/channelserver/handlers_stage.go b/server/channelserver/handlers_stage.go index 93cc10ce3..4082f2e30 100644 --- a/server/channelserver/handlers_stage.go +++ b/server/channelserver/handlers_stage.go @@ -97,7 +97,8 @@ func doStageTransfer(s *Session, ackHandle uint32, stageID string) { s.stage = s.server.stages[stageID] s.Unlock() - // Tell the client to cleanup its current stage objects + // Tell the client to cleanup its current stage objects. + // Use blocking send to ensure this critical cleanup packet is not dropped. s.QueueSendMHF(&mhfpacket.MsgSysCleanupObject{}) // Confirm the stage entry @@ -151,10 +152,12 @@ func doStageTransfer(s *Session, ackHandle uint32, stageID string) { s.stage.RUnlock() } + // FIX: Always send stage transfer packet, even if empty. + // The client expects this packet to complete the zone change, regardless of content. + // Previously, if newNotif was empty (no users, no objects), no packet was sent, + // causing the client to timeout after 60 seconds. newNotif.WriteUint16(0x0010) // End it. - if len(newNotif.Data()) > 2 { - s.QueueSend(newNotif.Data()) - } + s.QueueSend(newNotif.Data()) } func destructEmptyStages(s *Session) { @@ -172,17 +175,36 @@ func destructEmptyStages(s *Session) { } func removeSessionFromStage(s *Session) { + // Acquire stage lock to protect concurrent access to clients and objects maps + // This prevents race conditions when multiple goroutines access these maps + s.stage.Lock() + // Remove client from old stage. delete(s.stage.clients, s) - // Delete old stage objects owned by the client. + // Collect objects to delete while holding lock s.logger.Info("Sending notification to old stage clients") + var objectsToDelete []*Object for _, object := range s.stage.objects { if object.ownerCharID == s.charID { - s.stage.BroadcastMHF(&mhfpacket.MsgSysDeleteObject{ObjID: object.id}, s) - delete(s.stage.objects, object.ownerCharID) + objectsToDelete = append(objectsToDelete, object) } } + + // Delete from map while still holding lock + for _, object := range objectsToDelete { + delete(s.stage.objects, object.ownerCharID) + } + + // CRITICAL FIX: Unlock BEFORE broadcasting to avoid deadlock + // BroadcastMHF also tries to lock the stage, so we must release our lock first + s.stage.Unlock() + + // Now broadcast the deletions (without holding the lock) + for _, object := range objectsToDelete { + s.stage.BroadcastMHF(&mhfpacket.MsgSysDeleteObject{ObjID: object.id}, s) + } + destructEmptyStages(s) destructEmptySemaphores(s) } diff --git a/server/channelserver/sys_session.go b/server/channelserver/sys_session.go index 358192e1b..b80ddf7fb 100644 --- a/server/channelserver/sys_session.go +++ b/server/channelserver/sys_session.go @@ -147,7 +147,10 @@ func (s *Session) Start() { // // Thread Safety: Safe for concurrent calls from multiple goroutines. func (s *Session) QueueSend(data []byte) { - s.logMessage(binary.BigEndian.Uint16(data[0:2]), data, "Server", s.Name) + // FIX: Check data length before reading opcode to prevent crash on empty packets + if len(data) >= 2 { + s.logMessage(binary.BigEndian.Uint16(data[0:2]), data, "Server", s.Name) + } select { case s.sendPackets <- packet{data, false}: // Enqueued data @@ -182,7 +185,9 @@ func (s *Session) QueueSend(data []byte) { func (s *Session) QueueSendNonBlocking(data []byte) { select { case s.sendPackets <- packet{data, true}: - s.logMessage(binary.BigEndian.Uint16(data[0:2]), data, "Server", s.Name) + if len(data) >= 2 { + s.logMessage(binary.BigEndian.Uint16(data[0:2]), data, "Server", s.Name) + } default: s.logger.Warn("Packet queue too full, dropping!") } @@ -231,10 +236,13 @@ func (s *Session) sendLoop() { if s.closed { return } - pkt := <-s.sendPackets - err := s.cryptConn.SendPacket(append(pkt.data, []byte{0x00, 0x10}...)) - if err != nil { - s.logger.Warn("Failed to send packet") + // Send each packet individually with its own terminator + for len(s.sendPackets) > 0 { + pkt := <-s.sendPackets + err := s.cryptConn.SendPacket(append(pkt.data, []byte{0x00, 0x10}...)) + if err != nil { + s.logger.Warn("Failed to send packet", zap.Error(err)) + } } time.Sleep(10 * time.Millisecond) }