fix: serialize socket writes to prevent protocol corruption under concurrent goroutines - #2
Open
amathxbt wants to merge 1 commit into
Open
Conversation
…ed mutex ListenForInbound() spawns a new goroutine per inbound FSM message. Each goroutine can independently call sendProtoMsg() -> sendLengthPrefixed() -> conn.Write(), either as a direct response to the FSM (Genesis/Begin/Check/ Deliver/End) or via a nested StateRead/StateWrite call made from within the contract handler. The pending/requestContract maps are correctly guarded by the existing mutex (p.l), but the underlying net.Conn write path had no equivalent protection. Concurrent writers can interleave mid-message on the wire, corrupting the length-prefixed framing that both sides rely on to delimit messages -- this would manifest as garbled/undecodable protobuf payloads or a stuck connection under any real concurrency (e.g. a contract issuing a StateRead while another goroutine sends a response for a prior message). Fix: add Plugin.writeMu and hold it for the duration of the length-prefixed write in sendLengthPrefixed(), so at most one goroutine writes to the connection at a time.
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.
Bug
ListenForInbound()spawns a new goroutine per inbound FSM message:Each of these goroutines can independently reach
sendProtoMsg()->sendLengthPrefixed()->p.conn.Write(...):StateRead/StateWritecall made from within a contract handler (e.g.DeliverMessageSendissuing aStateReadwhile another goroutine is still finishing a response for a prior message).The
pending/requestContractmaps are correctly protected byp.l, but the underlyingnet.Connwrite path has no equivalent protection.net.Conn.Writeis not guaranteed to be safe for concurrent callers writing a single logical message; concurrent writers can interleave partial writes on the wire, corrupting the length-prefixed framing both sides of the connection rely on to delimit messages. Under any real concurrency this manifests as garbled/undecodable protobuf payloads or a wedged connection.Fix
Added
Plugin.writeMu sync.Mutexand hold it for the duration of the length-prefixed write insendLengthPrefixed(), so at most one goroutine writes to the connection at a time. Reads are unaffected — they only ever happen sequentially inside the singleListenForInboundloop.Testing
Read-through verification of all call sites of
sendProtoMsg/sendLengthPrefixed(viaHandshake,StateRead,StateWrite, and the inbound response path) to confirm all writers now go throughwriteMu. This repo has no existing test suite to extend.