diff --git a/src/game/Server/WorldGateway.cpp b/src/game/Server/WorldGateway.cpp index fa789ce04..c6d108fad 100644 --- a/src/game/Server/WorldGateway.cpp +++ b/src/game/Server/WorldGateway.cpp @@ -85,11 +85,11 @@ bool WorldGateway::FilterAuthPacket(WorldPacket& packet) return true; } -void WorldGateway::TracePacket(WorldPacket const& packet, bool incoming) +void WorldGateway::TracePacket(proto::SessionId session, WorldPacket const& packet, bool incoming) { if (sLog.IsPacketLoggingEnabled()) { - sLog.outWorldPacketDump(0, packet.GetOpcode(), + sLog.outWorldPacketDump(session, packet.GetOpcode(), LookupOpcodeName(packet.GetOpcode()), &packet, incoming); } } diff --git a/src/game/Server/WorldGateway.h b/src/game/Server/WorldGateway.h index d96aa0a71..6af085b8c 100644 --- a/src/game/Server/WorldGateway.h +++ b/src/game/Server/WorldGateway.h @@ -38,7 +38,7 @@ class WorldGateway final : public proto::IWorldGateway { public: bool FilterAuthPacket(WorldPacket& packet) override; - void TracePacket(WorldPacket const& packet, bool incoming) override; + void TracePacket(proto::SessionId session, WorldPacket const& packet, bool incoming) override; proto::AuthLookup LookupAccount(proto::AuthRequest const& request) override; proto::SessionId Attach(proto::AuthRequest const& request, std::shared_ptr const& link, diff --git a/src/proto/ClientConnection.cpp b/src/proto/ClientConnection.cpp index 8d0c3f6dd..f3892e83d 100644 --- a/src/proto/ClientConnection.cpp +++ b/src/proto/ClientConnection.cpp @@ -62,7 +62,7 @@ std::vector ClientConnection::onConnect() { WorldPacket challenge(SMSG_AUTH_CHALLENGE, 4); challenge << m_seed; - m_gateway.TracePacket(challenge, false); + m_gateway.TracePacket(INVALID_SESSION_ID, challenge, false); return EncodePacket(challenge); } catch (...) @@ -99,7 +99,8 @@ std::vector ClientConnection::onData( for (size_t i = 0; i < packets.size() && !m_closed.load(); ++i) { - m_gateway.TracePacket(packets[i], true); + m_gateway.TracePacket(m_traceSession.load(std::memory_order_relaxed), + packets[i], true); if (!HandlePacket(packets[i])) { Close(); @@ -126,6 +127,7 @@ void ClientConnection::onClose() session = m_session; m_session = INVALID_SESSION_ID; } + m_traceSession.store(INVALID_SESSION_ID, std::memory_order_relaxed); if (session != INVALID_SESSION_ID) { try @@ -153,7 +155,7 @@ void ClientConnection::SendPacket(WorldPacket const& packet) return; } - m_gateway.TracePacket(packet, false); + m_gateway.TracePacket(m_traceSession.load(std::memory_order_relaxed), packet, false); std::vector const frame = PacketCodec::Encode(packet, [this](uint8* header, std::size_t len) { @@ -290,6 +292,7 @@ bool ClientConnection::HandleAuthSession(WorldPacket& packet) m_gateway.Detach(session); return false; } + m_traceSession.store(session, std::memory_order_relaxed); return true; } diff --git a/src/proto/ClientConnection.h b/src/proto/ClientConnection.h index 0d0f9ac31..cd08cd777 100644 --- a/src/proto/ClientConnection.h +++ b/src/proto/ClientConnection.h @@ -79,6 +79,11 @@ class ClientConnection final : public net::ISession, public IClientLink std::mutex m_sessionLock; uint32 m_seed; SessionId m_session = INVALID_SESSION_ID; + + /// The same id, readable without m_sessionLock: SendPacket traces while holding + /// m_sendOrderLock, and taking m_sessionLock under it would invert the order + /// HandleAuthSession uses. Tracing only -- m_session stays the authority. + std::atomic m_traceSession{INVALID_SESSION_ID}; bool m_authStarted = false; std::atomic m_closed{false}; net::Sender m_sender; diff --git a/src/proto/IWorldGateway.h b/src/proto/IWorldGateway.h index 8b1b81632..9eb6c38c2 100644 --- a/src/proto/IWorldGateway.h +++ b/src/proto/IWorldGateway.h @@ -95,7 +95,11 @@ class IWorldGateway public: virtual ~IWorldGateway() = default; virtual bool FilterAuthPacket(WorldPacket& packet) = 0; - virtual void TracePacket(WorldPacket const& packet, bool incoming) = 0; + + /// `session` is INVALID_SESSION_ID for the pre-auth handshake only. Without it a dump + /// cannot say which of several connected clients a packet belongs to, which is most of + /// what a packet dump is for. + virtual void TracePacket(SessionId session, WorldPacket const& packet, bool incoming) = 0; virtual AuthLookup LookupAccount(AuthRequest const& request) = 0; virtual SessionId Attach(AuthRequest const& request, std::shared_ptr const& link, diff --git a/src/shared/Log/Log.cpp b/src/shared/Log/Log.cpp index 40b641f59..485eb2bed 100644 --- a/src/shared/Log/Log.cpp +++ b/src/shared/Log/Log.cpp @@ -1225,7 +1225,7 @@ void Log::outErrorScriptLib(const char* err, ...) } } -void Log::outWorldPacketDump(uint32 socket, uint32 opcode, char const* opcodeName, ByteBuffer const* packet, bool incoming) +void Log::outWorldPacketDump(uint32 session, uint32 opcode, char const* opcodeName, ByteBuffer const* packet, bool incoming) { if (!worldLogfile) { @@ -1237,18 +1237,19 @@ void Log::outWorldPacketDump(uint32 socket, uint32 opcode, char const* opcodeNam outTimestamp(worldLogfile); // Build the whole hex dump into one buffer and emit it with a single - // fwrite, instead of one fprintf PER BYTE (16+ stdio calls per row). Output - // is byte-identical to the previous format. Durability via Flush()/shutdown. + // fwrite, instead of one fprintf PER BYTE (16+ stdio calls per row). The hex + // body is byte-identical to that format. Durability via Flush()/shutdown. std::string out; out.reserve(packet->size() * 3 + packet->size() / 16 + 128); - // header[512] is ample for the fixed text plus a (short, compile-time) - // opcode-name constant; snprintf is bounded, so output stays byte-identical - // to the previous fprintf for every real opcode name. + // SESSION, not SOCKET: the field carried a hardcoded 0 for every packet of every + // client, so two clients logged as one interleaved stream that could not be taken + // apart. header[512] is ample for the fixed text plus a (short, compile-time) + // opcode-name constant, and snprintf is bounded. char header[512]; - snprintf(header, sizeof(header), "\n%s:\nSOCKET: %u\nLENGTH: %zu\nOPCODE: %s (0x%.4X)\nDATA:\n", + snprintf(header, sizeof(header), "\n%s:\nSESSION: %u\nLENGTH: %zu\nOPCODE: %s (0x%.4X)\nDATA:\n", incoming ? "CLIENT" : "SERVER", - socket, packet->size(), opcodeName, opcode); + session, packet->size(), opcodeName, opcode); out += header; char hexbuf[4]; diff --git a/src/shared/Log/Log.h b/src/shared/Log/Log.h index 082209559..6039f36ae 100644 --- a/src/shared/Log/Log.h +++ b/src/shared/Log/Log.h @@ -311,16 +311,16 @@ class Log : public MaNGOS::Singleton /** * @brief any log level * - * Called from WorldGateway::Deliver (incoming) and WorldSession::SendPacket - * (outgoing) -- see IsPacketLoggingEnabled()'s comment below. + * Called from WorldGateway::TracePacket, i.e. from ClientConnection on the + * network thread in both directions -- see IsPacketLoggingEnabled() below. * - * @param socket + * @param session which client stream the packet belongs to; 0 pre-auth * @param opcode * @param opcodeName * @param packet * @param incoming */ - void outWorldPacketDump(uint32 socket, uint32 opcode, char const* opcodeName, ByteBuffer const* packet, bool incoming); + void outWorldPacketDump(uint32 session, uint32 opcode, char const* opcodeName, ByteBuffer const* packet, bool incoming); /** * @brief any log level *