From f640cfee27db77d4b5468dc798547de9fc9d2f98 Mon Sep 17 00:00:00 2001 From: Houmgaor Date: Sun, 22 Feb 2026 17:01:22 +0100 Subject: [PATCH] fix: log SJIS decoding errors instead of silently discarding them Add SJISToUTF8Lossy() that wraps SJISToUTF8() and logs decode errors at slog.Debug level. Replace all 31 call sites across 17 files that previously discarded the error with `_, _ =`. This makes garbled text from malformed SJIS client data debuggable without adding noise at default log levels. --- cmd/protbot/scenario/chat.go | 4 ++-- common/stringsupport/string_convert.go | 11 +++++++++++ common/stringsupport/string_convert_test.go | 19 +++++++++++++++++++ docs/technical-debt.md | 14 ++------------ network/binpacket/msg_bin_chat.go | 4 ++-- .../mhfpacket/msg_mhf_apply_bbs_article.go | 6 +++--- network/mhfpacket/msg_mhf_create_guild.go | 2 +- network/mhfpacket/msg_mhf_create_joint.go | 2 +- network/mhfpacket/msg_mhf_enumerate_house.go | 2 +- network/mhfpacket/msg_mhf_load_house.go | 2 +- .../mhfpacket/msg_mhf_operate_warehouse.go | 2 +- network/mhfpacket/msg_mhf_send_mail.go | 4 ++-- .../msg_mhf_update_guild_message_board.go | 8 ++++---- network/mhfpacket/msg_mhf_update_house.go | 2 +- server/channelserver/handlers_data.go | 2 +- server/channelserver/handlers_guild_info.go | 8 ++++---- server/channelserver/handlers_guild_ops.go | 4 ++-- server/channelserver/handlers_session.go | 2 +- server/channelserver/model_character.go | 2 +- server/signserver/session.go | 6 +++--- 20 files changed, 63 insertions(+), 43 deletions(-) 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) }