Fix broadcastRPC sending an empty body after an out-RPC event handler reads the bitstream - #1270
Open
exc3pt1ongit wants to merge 1 commit into
Open
Conversation
… reads the bitstream RakNetLegacyNetwork::broadcastRPC passed bs.GetNumberOfUnreadBits() as the payload length to rakNetServer.RPC(). The onSendRPC / per-RPC onSend out-event handlers reset the read pointer and read from the bitstream, so by the time the send happens the read pointer sits at the end and GetNumberOfUnreadBits() returns 0 - the RPC goes out with a zero-length body. sendRPC, sendPacket and broadcastPacket all use GetNumberOfBitsUsed(); broadcastRPC was the only sender using the unread-bit count. Switch it to GetNumberOfBitsUsed() so the full written body is transmitted regardless of what out-event handlers do with the read pointer. Concrete impact: with any registered network out-event handler that reads the stream (for example a Pawn.RakNet OnOutgoingRPC hook), a broadcast RPC is sent empty. For a one-byte RPC broadcast such as SetWeather (152, via SetWeather) or SetWorldTime (94, via SetWorldTime), a legacy 0.3.7 client then reads its single byte from a null RPC parameter bitstream and crashes (samp.dll +0x1bf0e, ACCESS_VIOLATION reading 0x00000000).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RakNetLegacyNetwork::broadcastRPCtransmits RPCs with an empty body whenever anetwork out-event handler reads the outgoing bitstream. It passes
bs.GetNumberOfUnreadBits()as the payload length torakNetServer.RPC(), but theonSendRPC/ per-RPConSendhandlers reset and consume the read pointer first, so the"unread" count is
0by the time the packet is sent.Every other sender uses the amount actually written:
sendRPCGetNumberOfBitsUsed()sendPacketGetNumberOfBitsUsed()broadcastPacketGetNumberOfBitsUsed()broadcastRPCGetNumberOfUnreadBits()← the odd one outThe fix is to use
GetNumberOfBitsUsed()inbroadcastRPCas well (both theexceptPeerand broadcast-to-all paths).Root cause
broadcastRPCbuilds a read/write bitstream over the caller's data, then runs theout-event handlers:
GetNumberOfUnreadBits()isnumberOfBitsUsed - readOffset. A handler that reads thewhole payload leaves
readOffset == numberOfBitsUsed, so the RPC is sent with a0-bit body. Handlers are not required to restore the read pointer, and the packet/RPC
siblings above already don't depend on it.
Impact
Any broadcast RPC observed by an out-event handler that reads the stream (e.g. a
Pawn.RakNetOnOutgoingRPChook) is sent empty. This is silent for most modernclients, but it crashes legacy 0.3.7 clients on RPCs that carry a fixed body:
SetWeather()→ RPC 152 (SetWeather, 1 byte)SetWorldTime()→ RPC 94 (SetWorldTime, 1 byte)On the client,
RakPeer::HandleRPCPacketsetsrpcParms.input = NULLwhennumberOfBitsOfData == 0; the stock 0.3.7 RPC handler then wraps{NULL, 0}in ano-copy
BitStreamand reads its first byte, dereferencing a null data pointer:Reproduced deterministically: a server that periodically broadcasts
SetWeather/SetWorldTime(with aPawn.RakNetout-RPC hook loaded) crashes a stock 0.3.7 clientin-world ~a minute after spawn, every session. A wire capture shows the broadcast
SetWeather/SetWorldTimeRPC leaving with a zero-length body; the per-playerSetPlayerWeather/SetPlayerTimeequivalents (which go throughsendRPC) carry theirbyte correctly. With this change the broadcast RPCs carry their body and the crash no
longer occurs.
Change
Server/Components/LegacyNetwork/legacy_network_impl.hpp—broadcastRPC: usebs.GetNumberOfBitsUsed()instead ofbs.GetNumberOfUnreadBits()for the transmittedlength (two call sites), matching
sendRPC/sendPacket/broadcastPacket.