diff --git a/cmd/protbot/scenario/chat.go b/cmd/protbot/scenario/chat.go index 8e5ba64fb..76dc3dae1 100644 --- a/cmd/protbot/scenario/chat.go +++ b/cmd/protbot/scenario/chat.go @@ -62,8 +62,8 @@ func ListenChat(ch *protocol.ChannelConn, cb ChatCallback) { _ = pbf.ReadUint16() // flags _ = pbf.ReadUint16() // senderNameLen _ = pbf.ReadUint16() // messageLen - msg, _ := stringsupport.SJISToUTF8(pbf.ReadNullTerminatedBytes()) - sender, _ := stringsupport.SJISToUTF8(pbf.ReadNullTerminatedBytes()) + msg := stringsupport.SJISToUTF8Lossy(pbf.ReadNullTerminatedBytes()) + sender := stringsupport.SJISToUTF8Lossy(pbf.ReadNullTerminatedBytes()) cb(ChatMessage{ ChatType: chatType, diff --git a/common/stringsupport/string_convert.go b/common/stringsupport/string_convert.go index 41a3dbeb0..dc7657514 100644 --- a/common/stringsupport/string_convert.go +++ b/common/stringsupport/string_convert.go @@ -4,6 +4,7 @@ import ( "bytes" "fmt" "io" + "log/slog" "strconv" "strings" @@ -40,6 +41,16 @@ func SJISToUTF8(b []byte) (string, error) { return string(result), nil } +// SJISToUTF8Lossy decodes Shift-JIS bytes to a UTF-8 string, logging +// any decoding error at debug level instead of returning it. +func SJISToUTF8Lossy(b []byte) string { + s, err := SJISToUTF8(b) + if err != nil { + slog.Debug("SJIS decode failed", "error", err, "raw_len", len(b)) + } + return s +} + // ToNGWord converts a UTF-8 string into a slice of uint16 values in the // Shift-JIS byte-swapped format used by the MHF NG-word (chat filter) system. func ToNGWord(x string) []uint16 { diff --git a/common/stringsupport/string_convert_test.go b/common/stringsupport/string_convert_test.go index b90582024..33e281a5f 100644 --- a/common/stringsupport/string_convert_test.go +++ b/common/stringsupport/string_convert_test.go @@ -461,6 +461,25 @@ func BenchmarkCSVElems(b *testing.B) { } } +func TestSJISToUTF8Lossy(t *testing.T) { + // Valid SJIS (ASCII subset) decodes correctly. + got := SJISToUTF8Lossy([]byte("Hello")) + if got != "Hello" { + t.Errorf("SJISToUTF8Lossy(valid) = %q, want %q", got, "Hello") + } + + // Truncated multi-byte SJIS sequence (lead byte 0x82 without trail byte) + // does not panic and returns some result (lossy). + got = SJISToUTF8Lossy([]byte{0x82}) + _ = got // must not panic + + // Nil input returns empty string. + got = SJISToUTF8Lossy(nil) + if got != "" { + t.Errorf("SJISToUTF8Lossy(nil) = %q, want %q", got, "") + } +} + func TestUTF8ToSJIS_UnsupportedCharacters(t *testing.T) { // Regression test for PR #116: Characters outside the Shift-JIS range // (e.g. Lenny face, cuneiform) previously caused a panic in UTF8ToSJIS, diff --git a/docs/technical-debt.md b/docs/technical-debt.md index 9b72c85e9..a389b0ca9 100644 --- a/docs/technical-debt.md +++ b/docs/technical-debt.md @@ -102,17 +102,7 @@ Suggested split: `repo_guild.go` (core CRUD), `repo_guild_members.go`, `repo_gui ~~**a) `fmt.Sprintf` inside structured logger calls (6 sites):**~~ **Fixed.** All 6 sites now use `zap.Uint32`/`zap.Uint8`/`zap.String` structured fields instead of `fmt.Sprintf`. -**b) 20 silently discarded SJIS encoding errors in packet parsing:** - -The pattern `m.Field, _ = stringsupport.SJISToUTF8(...)` appears across: -- `network/binpacket/msg_bin_chat.go:43-44` -- `network/mhfpacket/msg_mhf_apply_bbs_article.go:33-35` -- `network/mhfpacket/msg_mhf_send_mail.go:38-39` -- `network/mhfpacket/msg_mhf_update_guild_message_board.go:41-42,50-51` -- `server/channelserver/model_character.go:175` -- And 7+ more packet files - -A malformed SJIS string from a client yields an empty string with no log output, making garbled text impossible to debug. The error return should at least be logged at debug level. +~~**b) 20+ silently discarded SJIS encoding errors in packet parsing:**~~ **Fixed.** All call sites now use `SJISToUTF8Lossy()` which logs decode errors at `slog.Debug` level. ### ~~6. Inconsistent transaction API~~ (Fixed) @@ -151,5 +141,5 @@ Based on impact and the momentum from recent repo-interface refactoring: 4. **Split `repo_guild.go`** — last oversized file after the recent refactoring push 5. ~~**Fix `fmt.Sprintf` in logger calls**~~ — **Done** 6. ~~**Add `LoopDelay` Viper default**~~ — **Done** -7. **Log SJIS decoding errors** — improves debuggability for text issues +7. ~~**Log SJIS decoding errors**~~ — **Done** 8. ~~**Standardize on `BeginTxx`**~~ — **Done** diff --git a/network/binpacket/msg_bin_chat.go b/network/binpacket/msg_bin_chat.go index ebd1636cc..1068e9565 100644 --- a/network/binpacket/msg_bin_chat.go +++ b/network/binpacket/msg_bin_chat.go @@ -40,8 +40,8 @@ func (m *MsgBinChat) Parse(bf *byteframe.ByteFrame) error { m.Flags = bf.ReadUint16() _ = bf.ReadUint16() // lenSenderName _ = bf.ReadUint16() // lenMessage - m.Message, _ = stringsupport.SJISToUTF8(bf.ReadNullTerminatedBytes()) - m.SenderName, _ = stringsupport.SJISToUTF8(bf.ReadNullTerminatedBytes()) + m.Message = stringsupport.SJISToUTF8Lossy(bf.ReadNullTerminatedBytes()) + m.SenderName = stringsupport.SJISToUTF8Lossy(bf.ReadNullTerminatedBytes()) return nil } diff --git a/network/mhfpacket/msg_mhf_apply_bbs_article.go b/network/mhfpacket/msg_mhf_apply_bbs_article.go index 4a83e36d3..f4e0980a2 100644 --- a/network/mhfpacket/msg_mhf_apply_bbs_article.go +++ b/network/mhfpacket/msg_mhf_apply_bbs_article.go @@ -30,9 +30,9 @@ func (m *MsgMhfApplyBbsArticle) Parse(bf *byteframe.ByteFrame, ctx *clientctx.Cl m.AckHandle = bf.ReadUint32() m.Unk0 = bf.ReadUint32() m.Unk1 = bf.ReadBytes(16) - m.Name, _ = stringsupport.SJISToUTF8(bfutil.UpToNull(bf.ReadBytes(32))) - m.Title, _ = stringsupport.SJISToUTF8(bfutil.UpToNull(bf.ReadBytes(128))) - m.Description, _ = stringsupport.SJISToUTF8(bfutil.UpToNull(bf.ReadBytes(256))) + m.Name = stringsupport.SJISToUTF8Lossy(bfutil.UpToNull(bf.ReadBytes(32))) + m.Title = stringsupport.SJISToUTF8Lossy(bfutil.UpToNull(bf.ReadBytes(128))) + m.Description = stringsupport.SJISToUTF8Lossy(bfutil.UpToNull(bf.ReadBytes(256))) return nil } diff --git a/network/mhfpacket/msg_mhf_create_guild.go b/network/mhfpacket/msg_mhf_create_guild.go index a3739a184..44eb117b7 100644 --- a/network/mhfpacket/msg_mhf_create_guild.go +++ b/network/mhfpacket/msg_mhf_create_guild.go @@ -25,7 +25,7 @@ func (m *MsgMhfCreateGuild) Parse(bf *byteframe.ByteFrame, ctx *clientctx.Client m.AckHandle = bf.ReadUint32() bf.ReadUint16() // Zeroed bf.ReadUint16() // Name length - m.Name, _ = stringsupport.SJISToUTF8(bf.ReadNullTerminatedBytes()) + m.Name = stringsupport.SJISToUTF8Lossy(bf.ReadNullTerminatedBytes()) return nil } diff --git a/network/mhfpacket/msg_mhf_create_joint.go b/network/mhfpacket/msg_mhf_create_joint.go index 303571548..045ae5163 100644 --- a/network/mhfpacket/msg_mhf_create_joint.go +++ b/network/mhfpacket/msg_mhf_create_joint.go @@ -27,7 +27,7 @@ func (m *MsgMhfCreateJoint) Parse(bf *byteframe.ByteFrame, ctx *clientctx.Client m.GuildID = bf.ReadUint32() bf.ReadUint16() // Zeroed bf.ReadUint16() // Name length - m.Name, _ = stringsupport.SJISToUTF8(bf.ReadNullTerminatedBytes()) + m.Name = stringsupport.SJISToUTF8Lossy(bf.ReadNullTerminatedBytes()) return nil } diff --git a/network/mhfpacket/msg_mhf_enumerate_house.go b/network/mhfpacket/msg_mhf_enumerate_house.go index fa3cc30cc..f011bec7a 100644 --- a/network/mhfpacket/msg_mhf_enumerate_house.go +++ b/network/mhfpacket/msg_mhf_enumerate_house.go @@ -30,7 +30,7 @@ func (m *MsgMhfEnumerateHouse) Parse(bf *byteframe.ByteFrame, ctx *clientctx.Cli bf.ReadUint16() // Zeroed lenName := bf.ReadUint8() if lenName > 0 { - m.Name, _ = stringsupport.SJISToUTF8(bf.ReadNullTerminatedBytes()) + m.Name = stringsupport.SJISToUTF8Lossy(bf.ReadNullTerminatedBytes()) } return nil } diff --git a/network/mhfpacket/msg_mhf_load_house.go b/network/mhfpacket/msg_mhf_load_house.go index 029307a18..5753517c5 100644 --- a/network/mhfpacket/msg_mhf_load_house.go +++ b/network/mhfpacket/msg_mhf_load_house.go @@ -32,7 +32,7 @@ func (m *MsgMhfLoadHouse) Parse(bf *byteframe.ByteFrame, ctx *clientctx.ClientCo m.CheckPass = bf.ReadBool() bf.ReadUint16() // Zeroed bf.ReadUint8() // Password length - m.Password, _ = stringsupport.SJISToUTF8(bf.ReadNullTerminatedBytes()) + m.Password = stringsupport.SJISToUTF8Lossy(bf.ReadNullTerminatedBytes()) return nil } diff --git a/network/mhfpacket/msg_mhf_operate_warehouse.go b/network/mhfpacket/msg_mhf_operate_warehouse.go index df9222742..db198a3e7 100644 --- a/network/mhfpacket/msg_mhf_operate_warehouse.go +++ b/network/mhfpacket/msg_mhf_operate_warehouse.go @@ -32,7 +32,7 @@ func (m *MsgMhfOperateWarehouse) Parse(bf *byteframe.ByteFrame, ctx *clientctx.C lenName := bf.ReadUint8() bf.ReadUint16() // Zeroed if lenName > 0 { - m.Name, _ = stringsupport.SJISToUTF8(bf.ReadNullTerminatedBytes()) + m.Name = stringsupport.SJISToUTF8Lossy(bf.ReadNullTerminatedBytes()) } return nil } diff --git a/network/mhfpacket/msg_mhf_send_mail.go b/network/mhfpacket/msg_mhf_send_mail.go index e79384b6b..bd3d1a345 100644 --- a/network/mhfpacket/msg_mhf_send_mail.go +++ b/network/mhfpacket/msg_mhf_send_mail.go @@ -35,8 +35,8 @@ func (m *MsgMhfSendMail) Parse(bf *byteframe.ByteFrame, ctx *clientctx.ClientCon bf.ReadUint16() // Zeroed m.Quantity = bf.ReadUint16() m.ItemID = bf.ReadUint16() - m.Subject, _ = stringsupport.SJISToUTF8(bf.ReadNullTerminatedBytes()) - m.Body, _ = stringsupport.SJISToUTF8(bf.ReadNullTerminatedBytes()) + m.Subject = stringsupport.SJISToUTF8Lossy(bf.ReadNullTerminatedBytes()) + m.Body = stringsupport.SJISToUTF8Lossy(bf.ReadNullTerminatedBytes()) return nil } diff --git a/network/mhfpacket/msg_mhf_update_guild_message_board.go b/network/mhfpacket/msg_mhf_update_guild_message_board.go index fb1913aef..d37862068 100644 --- a/network/mhfpacket/msg_mhf_update_guild_message_board.go +++ b/network/mhfpacket/msg_mhf_update_guild_message_board.go @@ -38,8 +38,8 @@ func (m *MsgMhfUpdateGuildMessageBoard) Parse(bf *byteframe.ByteFrame, ctx *clie m.StampID = bf.ReadUint32() m.TitleLength = bf.ReadUint32() m.BodyLength = bf.ReadUint32() - m.Title, _ = stringsupport.SJISToUTF8(bf.ReadBytes(uint(m.TitleLength))) - m.Body, _ = stringsupport.SJISToUTF8(bf.ReadBytes(uint(m.BodyLength))) + m.Title = stringsupport.SJISToUTF8Lossy(bf.ReadBytes(uint(m.TitleLength))) + m.Body = stringsupport.SJISToUTF8Lossy(bf.ReadBytes(uint(m.BodyLength))) case 1: m.PostID = bf.ReadUint32() case 2: @@ -47,8 +47,8 @@ func (m *MsgMhfUpdateGuildMessageBoard) Parse(bf *byteframe.ByteFrame, ctx *clie bf.ReadBytes(8) m.TitleLength = bf.ReadUint32() m.BodyLength = bf.ReadUint32() - m.Title, _ = stringsupport.SJISToUTF8(bf.ReadBytes(uint(m.TitleLength))) - m.Body, _ = stringsupport.SJISToUTF8(bf.ReadBytes(uint(m.BodyLength))) + m.Title = stringsupport.SJISToUTF8Lossy(bf.ReadBytes(uint(m.TitleLength))) + m.Body = stringsupport.SJISToUTF8Lossy(bf.ReadBytes(uint(m.BodyLength))) case 3: m.PostID = bf.ReadUint32() bf.ReadBytes(8) diff --git a/network/mhfpacket/msg_mhf_update_house.go b/network/mhfpacket/msg_mhf_update_house.go index c70e4633d..872ff3e6c 100644 --- a/network/mhfpacket/msg_mhf_update_house.go +++ b/network/mhfpacket/msg_mhf_update_house.go @@ -30,7 +30,7 @@ func (m *MsgMhfUpdateHouse) Parse(bf *byteframe.ByteFrame, ctx *clientctx.Client bf.ReadUint8() // Zeroed bf.ReadUint8() // Zeroed bf.ReadUint8() // Password length - m.Password, _ = stringsupport.SJISToUTF8(bf.ReadNullTerminatedBytes()) + m.Password = stringsupport.SJISToUTF8Lossy(bf.ReadNullTerminatedBytes()) return nil } diff --git a/server/channelserver/handlers_data.go b/server/channelserver/handlers_data.go index bd7e228a0..d90ac040f 100644 --- a/server/channelserver/handlers_data.go +++ b/server/channelserver/handlers_data.go @@ -194,7 +194,7 @@ func handleMsgMhfLoaddata(s *Session, p mhfpacket.MHFPacket) { _, _ = bf.Seek(88, io.SeekStart) name := bf.ReadNullTerminatedBytes() s.server.userBinary.Set(s.charID, 1, append(name, []byte{0x00}...)) - s.Name, _ = stringsupport.SJISToUTF8(name) + s.Name = stringsupport.SJISToUTF8Lossy(name) } func handleMsgMhfSaveScenarioData(s *Session, p mhfpacket.MHFPacket) { diff --git a/server/channelserver/handlers_guild_info.go b/server/channelserver/handlers_guild_info.go index 351b72035..01d5ef424 100644 --- a/server/channelserver/handlers_guild_info.go +++ b/server/channelserver/handlers_guild_info.go @@ -287,14 +287,14 @@ func handleMsgMhfEnumerateGuild(s *Session, p mhfpacket.MHFPacket) { if err == nil { switch pkt.Type { case mhfpacket.ENUMERATE_GUILD_TYPE_GUILD_NAME: - searchName, _ := stringsupport.SJISToUTF8(pkt.Data2.ReadNullTerminatedBytes()) + searchName := stringsupport.SJISToUTF8Lossy(pkt.Data2.ReadNullTerminatedBytes()) for _, guild := range tempGuilds { if strings.Contains(guild.Name, searchName) { guilds = append(guilds, guild) } } case mhfpacket.ENUMERATE_GUILD_TYPE_LEADER_NAME: - searchName, _ := stringsupport.SJISToUTF8(pkt.Data2.ReadNullTerminatedBytes()) + searchName := stringsupport.SJISToUTF8Lossy(pkt.Data2.ReadNullTerminatedBytes()) for _, guild := range tempGuilds { if strings.Contains(guild.LeaderName, searchName) { guilds = append(guilds, guild) @@ -364,14 +364,14 @@ func handleMsgMhfEnumerateGuild(s *Session, p mhfpacket.MHFPacket) { tempAlliances, err = s.server.guildRepo.ListAlliances() switch pkt.Type { case mhfpacket.ENUMERATE_ALLIANCE_TYPE_ALLIANCE_NAME: - searchName, _ := stringsupport.SJISToUTF8(pkt.Data2.ReadNullTerminatedBytes()) + searchName := stringsupport.SJISToUTF8Lossy(pkt.Data2.ReadNullTerminatedBytes()) for _, alliance := range tempAlliances { if strings.Contains(alliance.Name, searchName) { alliances = append(alliances, alliance) } } case mhfpacket.ENUMERATE_ALLIANCE_TYPE_LEADER_NAME: - searchName, _ := stringsupport.SJISToUTF8(pkt.Data2.ReadNullTerminatedBytes()) + searchName := stringsupport.SJISToUTF8Lossy(pkt.Data2.ReadNullTerminatedBytes()) for _, alliance := range tempAlliances { if strings.Contains(alliance.ParentGuild.LeaderName, searchName) { alliances = append(alliances, alliance) diff --git a/server/channelserver/handlers_guild_ops.go b/server/channelserver/handlers_guild_ops.go index 79011acc6..8a76b7225 100644 --- a/server/channelserver/handlers_guild_ops.go +++ b/server/channelserver/handlers_guild_ops.go @@ -108,7 +108,7 @@ func handleMsgMhfOperateGuild(s *Session, p mhfpacket.MHFPacket) { doAckSimpleFail(s, pkt.AckHandle, make([]byte, 4)) return } - guild.Comment, _ = stringsupport.SJISToUTF8(pkt.Data2.ReadNullTerminatedBytes()) + guild.Comment = stringsupport.SJISToUTF8Lossy(pkt.Data2.ReadNullTerminatedBytes()) if err := s.server.guildRepo.Save(guild); err != nil { s.logger.Error("Failed to save guild comment", zap.Error(err)) } @@ -168,7 +168,7 @@ func handleMsgMhfOperateGuild(s *Session, p mhfpacket.MHFPacket) { } func handleRenamePugi(s *Session, bf *byteframe.ByteFrame, guild *Guild, num int) { - name, _ := stringsupport.SJISToUTF8(bf.ReadNullTerminatedBytes()) + name := stringsupport.SJISToUTF8Lossy(bf.ReadNullTerminatedBytes()) switch num { case 1: guild.PugiName1 = name diff --git a/server/channelserver/handlers_session.go b/server/channelserver/handlers_session.go index 0362b0274..3919a244f 100644 --- a/server/channelserver/handlers_session.go +++ b/server/channelserver/handlers_session.go @@ -489,7 +489,7 @@ func handleMsgMhfTransitMessage(s *Session, p mhfpacket.MHFPacket) { bf.ReadUint16() // term length maxResults = bf.ReadUint16() bf.ReadUint8() // Unk - term, _ = stringsupport.SJISToUTF8(bf.ReadNullTerminatedBytes()) + term = stringsupport.SJISToUTF8Lossy(bf.ReadNullTerminatedBytes()) case 3: _ip := bf.ReadBytes(4) ip = fmt.Sprintf("%d.%d.%d.%d", _ip[3], _ip[2], _ip[1], _ip[0]) diff --git a/server/channelserver/model_character.go b/server/channelserver/model_character.go index e63228bb9..047744425 100644 --- a/server/channelserver/model_character.go +++ b/server/channelserver/model_character.go @@ -172,7 +172,7 @@ const ( ) func (save *CharacterSaveData) updateStructWithSaveData() { - save.Name, _ = stringsupport.SJISToUTF8(bfutil.UpToNull(save.decompSave[saveFieldNameOffset : saveFieldNameOffset+saveFieldNameLen])) + save.Name = stringsupport.SJISToUTF8Lossy(bfutil.UpToNull(save.decompSave[saveFieldNameOffset : saveFieldNameOffset+saveFieldNameLen])) if save.decompSave[save.Pointers[pGender]] == 1 { save.Gender = true } else { diff --git a/server/signserver/session.go b/server/signserver/session.go index 4564f2f6c..bdbcf9a07 100644 --- a/server/signserver/session.go +++ b/server/signserver/session.go @@ -155,7 +155,7 @@ func (s *Session) handlePSSGN(bf *byteframe.ByteFrame) { func (s *Session) handlePSNLink(bf *byteframe.ByteFrame) { _ = bf.ReadNullTerminatedBytes() // Client ID - credStr, _ := stringsupport.SJISToUTF8(bf.ReadNullTerminatedBytes()) + credStr := stringsupport.SJISToUTF8Lossy(bf.ReadNullTerminatedBytes()) credentials := strings.Split(credStr, "\n") tok := string(bf.ReadNullTerminatedBytes()) uid, resp := s.server.validateLogin(credentials[0], credentials[1]) @@ -195,8 +195,8 @@ func (s *Session) handlePSNLink(bf *byteframe.ByteFrame) { } func (s *Session) handleDSGN(bf *byteframe.ByteFrame) { - user, _ := stringsupport.SJISToUTF8(bf.ReadNullTerminatedBytes()) - pass, _ := stringsupport.SJISToUTF8(bf.ReadNullTerminatedBytes()) + user := stringsupport.SJISToUTF8Lossy(bf.ReadNullTerminatedBytes()) + pass := stringsupport.SJISToUTF8Lossy(bf.ReadNullTerminatedBytes()) _ = string(bf.ReadNullTerminatedBytes()) // Unk s.authenticate(user, pass) }