From 8b2667f7a02c8b06ceee56d35a731e210878c0fb Mon Sep 17 00:00:00 2001 From: Houmgaor Date: Mon, 6 Apr 2026 18:04:10 +0200 Subject: [PATCH] fix(handlers): harden gacha_items and boost time limit ACK paths Both bugs surfaced when running protbot against a live Erupe instance. LoadColumnWithDefault now treats an empty bytea ('\x', len 0) the same as NULL. The postgres driver returns a non-nil empty slice for empty bytea, so the prior `data == nil` check let a zero-byte slice reach handleMsgMhfReceiveGachaItem, which forwarded it to the client as a malformed gacha_items response. The MHF client interprets a zero-byte count field as a protocol error and crashes the gacha menu, matching the #175 symptom class. The handler also gains a defensive fallback for len(data) == 0 in case another caller hits the same edge. handleMsgMhfGetBoostTimeLimit was sending two ACKs for a single request: doAckBufSucceed with the real payload, then an unconditional doAckSimpleSucceed on the same ack handle. The second ACK was dead on arrival (the handle was already consumed) but is a latent protocol bug. Drop it and update the regression test that was asserting the buggy 2-packet behavior. --- server/channelserver/handlers_cafe.go | 1 - server/channelserver/handlers_cafe_test.go | 8 +++++--- server/channelserver/handlers_gacha.go | 4 +++- server/channelserver/repo_character.go | 6 +++++- server/channelserver/repo_character_test.go | 21 +++++++++++++++++++++ 5 files changed, 34 insertions(+), 6 deletions(-) diff --git a/server/channelserver/handlers_cafe.go b/server/channelserver/handlers_cafe.go index 4197f7d99..c59e04d16 100644 --- a/server/channelserver/handlers_cafe.go +++ b/server/channelserver/handlers_cafe.go @@ -227,7 +227,6 @@ func handleMsgMhfGetBoostTimeLimit(s *Session, p mhfpacket.MHFPacket) { bf.WriteUint32(uint32(boostLimit.Unix())) } doAckBufSucceed(s, pkt.AckHandle, bf.Data()) - doAckSimpleSucceed(s, pkt.AckHandle, make([]byte, 4)) } func handleMsgMhfGetBoostRight(s *Session, p mhfpacket.MHFPacket) { diff --git a/server/channelserver/handlers_cafe_test.go b/server/channelserver/handlers_cafe_test.go index a28e1334d..5ee147faa 100644 --- a/server/channelserver/handlers_cafe_test.go +++ b/server/channelserver/handlers_cafe_test.go @@ -231,7 +231,9 @@ func TestHandleMsgMhfGetBoostTimeLimit(t *testing.T) { handleMsgMhfGetBoostTimeLimit(session, pkt) - // This handler sends two responses (doAckBufSucceed + doAckSimpleSucceed) + // One response: the stray second ACK (doAckSimpleSucceed) was removed + // because it fired with the same ack handle as the real buf ACK, which + // is a protocol bug. count := 0 for { select { @@ -242,8 +244,8 @@ func TestHandleMsgMhfGetBoostTimeLimit(t *testing.T) { } } done: - if count != 2 { - t.Errorf("Expected 2 response packets, got %d", count) + if count != 1 { + t.Errorf("Expected 1 response packet, got %d", count) } } diff --git a/server/channelserver/handlers_gacha.go b/server/channelserver/handlers_gacha.go index fe8c61678..432df53b7 100644 --- a/server/channelserver/handlers_gacha.go +++ b/server/channelserver/handlers_gacha.go @@ -84,7 +84,9 @@ func handleMsgMhfUseGachaPoint(s *Session, p mhfpacket.MHFPacket) { func handleMsgMhfReceiveGachaItem(s *Session, p mhfpacket.MHFPacket) { pkt := p.(*mhfpacket.MsgMhfReceiveGachaItem) data, err := s.server.charRepo.LoadColumnWithDefault(s.charID, "gacha_items", []byte{0x00}) - if err != nil { + if err != nil || len(data) == 0 { + // The client requires at least one byte (the item count) or it + // treats the response as malformed and crashes (see #175). data = []byte{0x00} } diff --git a/server/channelserver/repo_character.go b/server/channelserver/repo_character.go index 6bc3a9b6b..66b978e1f 100644 --- a/server/channelserver/repo_character.go +++ b/server/channelserver/repo_character.go @@ -167,7 +167,11 @@ func (r *CharacterRepository) LoadColumnWithDefault(charID uint32, column string if err != nil { return defaultVal, err } - if data == nil { + // Treat empty bytea ('\x', len 0) the same as NULL. The postgres driver + // returns a non-nil empty slice for empty bytea, so a bare `data == nil` + // check would send zero bytes to the client — which the MHF client + // interprets as a malformed response and crashes on (see #175). + if len(data) == 0 { return defaultVal, nil } return data, nil diff --git a/server/channelserver/repo_character_test.go b/server/channelserver/repo_character_test.go index 8fb1beaba..7d8424f72 100644 --- a/server/channelserver/repo_character_test.go +++ b/server/channelserver/repo_character_test.go @@ -392,6 +392,27 @@ func TestLoadColumnWithDefaultExistingData(t *testing.T) { } } +// TestLoadColumnWithDefaultEmptyBytea verifies that an empty bytea ('\x', len 0) +// is treated the same as NULL and returns the default value. Without this, the +// postgres driver returns a non-nil empty slice that would reach the client +// as a zero-byte ACK payload and crash the MHF gacha menu (see #175). +func TestLoadColumnWithDefaultEmptyBytea(t *testing.T) { + repo, db, charID := setupCharRepo(t) + + if _, err := db.Exec("UPDATE characters SET skin_hist=''::bytea WHERE id=$1", charID); err != nil { + t.Fatalf("Setup failed: %v", err) + } + + defaultVal := []byte{0x00, 0x01, 0x02} + got, err := repo.LoadColumnWithDefault(charID, "skin_hist", defaultVal) + if err != nil { + t.Fatalf("LoadColumnWithDefault failed: %v", err) + } + if len(got) != 3 || got[0] != 0x00 || got[2] != 0x02 { + t.Errorf("Expected default value for empty bytea, got: %x", got) + } +} + func TestSetDeleted(t *testing.T) { repo, db, charID := setupCharRepo(t)