From 8e57c45b2c9bd5d32db5239e6ee5c91022295455 Mon Sep 17 00:00:00 2001 From: Matheus Degiovani Date: Tue, 13 Aug 2024 15:38:50 -0300 Subject: [PATCH 1/3] transport: Prove double release is occurring This adds a panic on attempts to double release of a message. This might bubble up to a double release of an Arena, which is a misuse of the Arena contract. Adding this panic causes some tests to fail. This will be fixed in a future commit. --- rpc/transport/transport.go | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/rpc/transport/transport.go b/rpc/transport/transport.go index 17fd7275..a146a4bf 100644 --- a/rpc/transport/transport.go +++ b/rpc/transport/transport.go @@ -165,7 +165,7 @@ func (s *transport) RecvMessage() (IncomingMessage, error) { return nil, err } - return incomingMsg(rmsg), nil + return &incomingMsg{message: rmsg}, nil } // Close closes the underlying ReadWriteCloser. It is not safe to call @@ -235,14 +235,21 @@ func (o *outgoingMsg) Send() error { panic("call to Send() after call to Release()") } -type incomingMsg rpccp.Message +type incomingMsg struct { + message rpccp.Message + released bool +} -func (i incomingMsg) Message() rpccp.Message { - return rpccp.Message(i) +func (i *incomingMsg) Message() rpccp.Message { + return i.message } -func (i incomingMsg) Release() { +func (i *incomingMsg) Release() { if m := i.Message().Message(); m != nil { + if i.released { + panic("double release") + } + i.released = true m.Release() } } From 600025304c738dcfd931e16329581e1056b6d15e Mon Sep 17 00:00:00 2001 From: Matheus Degiovani Date: Tue, 13 Aug 2024 15:40:23 -0300 Subject: [PATCH 2/3] transport: Fix double release of incoming message This fixes the double release panic introduced in the prior commit. The panic was only introduced to prove that double release is possible happening. Note: In my (matheusd) opinion, Release() calls should panic or error in case of double release so that callers are fixed and they themselves do not rely on this protection. This is suggested as a future change because it potentially breaks clients that were relying on this protection. --- rpc/transport/transport.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/rpc/transport/transport.go b/rpc/transport/transport.go index a146a4bf..b8ba1c7c 100644 --- a/rpc/transport/transport.go +++ b/rpc/transport/transport.go @@ -245,10 +245,7 @@ func (i *incomingMsg) Message() rpccp.Message { } func (i *incomingMsg) Release() { - if m := i.Message().Message(); m != nil { - if i.released { - panic("double release") - } + if m := i.Message().Message(); !i.released && m != nil { i.released = true m.Release() } From df83d72e7f2d0f1f94988f5404dce99547b39119 Mon Sep 17 00:00:00 2001 From: Matheus Degiovani Date: Tue, 13 Aug 2024 15:42:57 -0300 Subject: [PATCH 3/3] transport: Track outoingMsg release status This was not correctly tracked. --- rpc/transport/transport.go | 1 + 1 file changed, 1 insertion(+) diff --git a/rpc/transport/transport.go b/rpc/transport/transport.go index b8ba1c7c..bdb90844 100644 --- a/rpc/transport/transport.go +++ b/rpc/transport/transport.go @@ -219,6 +219,7 @@ type outgoingMsg struct { func (o *outgoingMsg) Release() { if m := o.message.Message(); !o.released && m != nil { + o.released = true m.Release() } }