From 1eb7143fc5380cbf12f9ebc81a1967fe3599e4a1 Mon Sep 17 00:00:00 2001 From: Matheus Degiovani Date: Wed, 14 Aug 2024 17:20:33 -0300 Subject: [PATCH 1/4] tests: Fix reuse of SingleSegmentArena The arena was reused when it shouldn't after a Release() call. --- integration_test.go | 2 +- message_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/integration_test.go b/integration_test.go index f1530a01..38f375c6 100644 --- a/integration_test.go +++ b/integration_test.go @@ -1770,7 +1770,7 @@ func BenchmarkMarshal_ReuseMsg(b *testing.B) { b.ResetTimer() for i := 0; i < b.N; i++ { a := data[r.Intn(len(data))] - seg, err := msg.Reset(msg.Arena) + seg, err := msg.Reset(capnp.SingleSegment(nil)) if err != nil { b.Fatal(err) } diff --git a/message_test.go b/message_test.go index 5bd8448c..4d3289b3 100644 --- a/message_test.go +++ b/message_test.go @@ -646,11 +646,11 @@ var errReadOnlyArena = errors.New("Allocate called on read-only arena") func BenchmarkMessageGetFirstSegment(b *testing.B) { var msg Message - var arena Arena = SingleSegment(nil) b.ResetTimer() b.ReportAllocs() for i := 0; i < b.N; i++ { + arena := SingleSegment(nil) _, err := msg.Reset(arena) if err != nil { b.Fatal(err) From e5b07cbd9fb62f2f70dc73e95fbf9dc992cf55a9 Mon Sep 17 00:00:00 2001 From: Matheus Degiovani Date: Thu, 15 Aug 2024 15:12:51 -0300 Subject: [PATCH 2/4] transport: Remove double Release call from test In the future, it would be ideal if wrongful Release() calls could be flagged (either through an error or panic). --- rpc/transport/transport_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/rpc/transport/transport_test.go b/rpc/transport/transport_test.go index 5cee8ef6..072ae052 100644 --- a/rpc/transport/transport_test.go +++ b/rpc/transport/transport_test.go @@ -45,12 +45,10 @@ func testTransport(t *testing.T, makePipe func() (t1, t2 Transport, err error)) if err != nil { t.Fatal("t1.NewMessage #1:", err) } - defer callMsg.Release() bootMsg, err := t1.NewMessage() if err != nil { t.Fatal("t1.NewMessage #2:", err) } - defer bootMsg.Release() // Fill in bootstrap message boot, err := bootMsg.Message().NewBootstrap() From 0a324361546fe25aab0c34cf523f219400a634f1 Mon Sep 17 00:00:00 2001 From: Matheus Degiovani Date: Thu, 15 Aug 2024 15:30:15 -0300 Subject: [PATCH 3/4] transport: Test for released messages earlier This moves the check for released messages earlier in the Release() call for transport messages. This prevents triggering the race detector, due to the check happening before an attempt is made at accessing the message (which involves indirection through a segment that may have been released already). While the prior code would not have actually caused a fault in current code (because the conditional checks both for the released flag and whether the message is nil), checking by the release flag first is more correct and may prevent future bugs. --- rpc/transport/transport.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/rpc/transport/transport.go b/rpc/transport/transport.go index bdb90844..31a36563 100644 --- a/rpc/transport/transport.go +++ b/rpc/transport/transport.go @@ -218,7 +218,10 @@ type outgoingMsg struct { } func (o *outgoingMsg) Release() { - if m := o.message.Message(); !o.released && m != nil { + if o.released { + return + } + if m := o.message.Message(); m != nil { o.released = true m.Release() } @@ -246,7 +249,10 @@ func (i *incomingMsg) Message() rpccp.Message { } func (i *incomingMsg) Release() { - if m := i.Message().Message(); !i.released && m != nil { + if i.released { + return + } + if m := i.Message().Message(); m != nil { i.released = true m.Release() } From cf2a39ac3a48b3f693e2e27307b181ca2a0e8005 Mon Sep 17 00:00:00 2001 From: Matheus Degiovani Date: Wed, 14 Aug 2024 15:50:10 -0300 Subject: [PATCH 4/4] codec: Enforce NumSegments limit Previously this was not enforced by the Encode function. While this limit is unlikely to be hit in practice, it's important to assert it due to the type allowing it and a count of segments greater than 2^32 causing an invalid marshaling. --- codec.go | 7 +++++++ codec_test.go | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/codec.go b/codec.go index 83b42a0e..c68ec375 100644 --- a/codec.go +++ b/codec.go @@ -195,6 +195,10 @@ func MustUnmarshalRoot(data []byte) Ptr { return p } +var ( + errTooManySegments = errors.New("message has too many segments") +) + // An Encoder represents a framer for serializing a particular Cap'n // Proto stream. type Encoder struct { @@ -220,6 +224,9 @@ func (e *Encoder) Encode(m *Message) error { if nsegs == 0 { return errors.New("encode: message has no segments") } + if nsegs > 1<<32 { + return exc.WrapError("encode", errTooManySegments) + } e.bufs = append(e.bufs[:0], nil) // first element is placeholder for header maxSeg := SegmentID(nsegs - 1) hdrSize := streamHeaderSize(maxSeg) diff --git a/codec_test.go b/codec_test.go index 1e194123..9a66c9d8 100644 --- a/codec_test.go +++ b/codec_test.go @@ -2,8 +2,11 @@ package capnp import ( "bytes" + "errors" "io" "testing" + + "github.com/stretchr/testify/require" ) func TestEncoder(t *testing.T) { @@ -72,6 +75,40 @@ func TestDecoder(t *testing.T) { } } +type tooManySegsArena struct { + data []byte +} + +func (t *tooManySegsArena) NumSegments() int64 { return 1<<32 + 1 } + +func (t *tooManySegsArena) Data(id SegmentID) ([]byte, error) { + return nil, errors.New("no data") +} + +func (t *tooManySegsArena) Allocate(minsz Size, segs map[SegmentID]*Segment) (SegmentID, []byte, error) { + return 0, nil, errors.New("cannot allocate") +} + +func (t *tooManySegsArena) Release() {} + +// TestEncoderTooManySegments verifies attempting to encode an arena that has +// more segments than possible. +func TestEncoderTooManySegments(t *testing.T) { + t.Parallel() + zeroWord := []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00} + arena := &tooManySegsArena{data: zeroWord} + + // Setup via field because NewMessage checks arena has > 1 segments. + var msg Message + msg.Arena = arena + var buf bytes.Buffer + enc := NewEncoder(&buf) + err := enc.Encode(&msg) + + // Encoding should error with a specific error. + require.ErrorIs(t, err, errTooManySegments) +} + func TestDecoder_MaxMessageSize(t *testing.T) { t.Parallel()