Skip to content

Commit fca448c

Browse files
newmanifoldbenaadams
newmanifold
and
benaadams
authored
fix: IByteBuffer leak in EnrResponseMsgSerializer & ArrayPoolSpan OOB (#10853)
* fix leak & array oob * add leak detector * Add claude rule * remove docs * fix formatting & add allocator cost note to LeakDetector * fix: address PR review feedback - ArrayPoolSpan.Slice: validate against logical length, not rented array - ArrayPoolSpan indexer: add nameof(index) to ThrowIfGreaterThanOrEqual - Add Slice tests: boundary, out-of-range, and logical length enforcement - PooledBufferLeakDetector: add AssertNoLeaks() for explicit assertion, make Dispose() non-throwing to avoid masking test-body exceptions - Add explicit FluentAssertions package reference to Discovery.Test csproj - Add buffer refcount leak tests for all discovery message serializers (Ping, Pong, FindNode, Neighbors, EnrResponse — happy and error paths) - Fix leak test: use ReferenceCount check instead of PooledBufferLeakDetector (pool NumActiveAllocations metric is unreliable with zero-cache config) --------- Co-authored-by: Ben Adams <thundercat@illyriad.co.uk>
1 parent 1b3c887 commit fca448c

File tree

8 files changed

+344
-32
lines changed

8 files changed

+344
-32
lines changed

‎.agents/rules/robustness.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ Patterns that cause silent failures, resource leaks, or deadlocks in production.
1212
## Resource management
1313

1414
- `IDisposable` / `IAsyncDisposable` objects (especially `IDb`, streams, channels) must be wrapped in `using` — otherwise they leak.
15+
- `ReadBytes(n)` allocates a new ref-counted `IByteBuffer`. The method that allocates the buffer owns it; if ownership transfers (e.g. passing to a handler or message), the receiver becomes responsible for calling `Release()` / `SafeRelease()`. Forgetting to release, or releasing after ownership has transferred, are the two most common leak/corruption sources in the networking layer.
1516
- Never swallow exceptions in an empty `catch` block — at minimum log the exception. Silent failures are the hardest to diagnose on a running node.
1617

1718
## Thread safety

‎src/Nethermind/Nethermind.Core.Test/Collections/ArrayPoolSpanTests.cs‎

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
// SPDX-FileCopyrightText: 2026 Demerzel Solutions Limited
2+
// SPDX-License-Identifier: LGPL-3.0-only
3+
4+
using System;
5+
using FluentAssertions;
6+
using Nethermind.Core.Collections;
7+
using NUnit.Framework;
8+
9+
namespace Nethermind.Core.Test.Collections;
10+
11+
public class ArrayPoolSpanTests
12+
{
13+
[TestCase(8, 8, Description = "At index == Length")]
14+
[TestCase(4, 5, Description = "Beyond Length")]
15+
public void Indexer_out_of_bounds_should_throw(int length, int index)
16+
{
17+
using ArrayPoolSpan<int> span = new(length);
18+
span.Invoking(s => { int _ = s[index]; }).Should().Throw<ArgumentOutOfRangeException>();
19+
}
20+
21+
[TestCase(4, -1)]
22+
[TestCase(8, -5)]
23+
public void Indexer_negative_should_throw(int length, int index)
24+
{
25+
using ArrayPoolSpan<int> span = new(length);
26+
span.Invoking(s => { int _ = s[index]; }).Should().Throw<IndexOutOfRangeException>();
27+
}
28+
29+
[Test]
30+
public void Indexer_within_bounds_should_round_trip()
31+
{
32+
using ArrayPoolSpan<int> span = new(4);
33+
span[0] = 10;
34+
span[3] = 40;
35+
span[0].Should().Be(10);
36+
span[3].Should().Be(40);
37+
}
38+
39+
[TestCase(0)]
40+
[TestCase(1)]
41+
[TestCase(7)]
42+
[TestCase(16)]
43+
public void Length_should_return_requested_size(int length)
44+
{
45+
using ArrayPoolSpan<int> span = new(length);
46+
span.Length.Should().Be(length);
47+
}
48+
49+
[Test]
50+
public void Implicit_span_conversion_should_respect_length()
51+
{
52+
using ArrayPoolSpan<int> span = new(4);
53+
for (int i = 0; i < 4; i++) span[i] = i;
54+
55+
Span<int> s = span;
56+
s.Length.Should().Be(4);
57+
s[0].Should().Be(0);
58+
s[3].Should().Be(3);
59+
}
60+
61+
[Test]
62+
public void Implicit_readonly_span_conversion_should_respect_length()
63+
{
64+
using ArrayPoolSpan<int> span = new(4);
65+
for (int i = 0; i < 4; i++) span[i] = i;
66+
67+
ReadOnlySpan<int> s = span;
68+
s.Length.Should().Be(4);
69+
s[0].Should().Be(0);
70+
s[3].Should().Be(3);
71+
}
72+
73+
[Test]
74+
public void Enumeration_should_yield_exactly_length_elements()
75+
{
76+
using ArrayPoolSpan<int> span = new(5);
77+
for (int i = 0; i < 5; i++) span[i] = i * 10;
78+
79+
int count = 0;
80+
foreach (int val in span)
81+
{
82+
val.Should().Be(count * 10);
83+
count++;
84+
}
85+
count.Should().Be(5);
86+
}
87+
88+
[Test]
89+
public void Slice_respects_logical_length()
90+
{
91+
using ArrayPoolSpan<int> span = new(5);
92+
for (int i = 0; i < 5; i++) span[i] = i;
93+
94+
Span<int> slice = span.Slice(1, 3);
95+
slice.Length.Should().Be(3);
96+
slice[0].Should().Be(1);
97+
slice[2].Should().Be(3);
98+
}
99+
100+
[Test]
101+
public void Slice_throws_when_exceeding_logical_length()
102+
{
103+
using ArrayPoolSpan<int> span = new(5);
104+
105+
// Start + length exceeds logical length (5), even though rented array may be larger
106+
Assert.Throws<ArgumentOutOfRangeException>(() => span.Slice(3, 5));
107+
}
108+
109+
[Test]
110+
public void Slice_at_boundary_succeeds()
111+
{
112+
using ArrayPoolSpan<int> span = new(5);
113+
114+
// Exactly at the boundary should work
115+
Span<int> slice = span.Slice(0, 5);
116+
slice.Length.Should().Be(5);
117+
}
118+
}

‎src/Nethermind/Nethermind.Core/Collections/ArrayPoolSpan.cs‎

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33

44
using System.Buffers;
55
using System;
6-
using System.Diagnostics.CodeAnalysis;
76
using System.Collections.Generic;
87
using System.Collections;
98

@@ -20,25 +19,19 @@ public readonly ref T this[int index]
2019
{
2120
get
2221
{
23-
if (index > _length)
24-
{
25-
ThrowArgumentOutOfRangeException();
26-
}
27-
22+
ArgumentOutOfRangeException.ThrowIfGreaterThanOrEqual(index, _length, nameof(index));
2823
return ref _array[index];
29-
30-
[DoesNotReturn]
31-
static void ThrowArgumentOutOfRangeException()
32-
{
33-
throw new ArgumentOutOfRangeException(nameof(index));
34-
}
3524
}
3625
}
3726

3827
public static implicit operator Span<T>(ArrayPoolSpan<T> arrayPoolSpan) => arrayPoolSpan._array.AsSpan(0, arrayPoolSpan._length);
3928
public static implicit operator ReadOnlySpan<T>(ArrayPoolSpan<T> arrayPoolSpan) => arrayPoolSpan._array.AsSpan(0, arrayPoolSpan._length);
4029

41-
public Span<T> Slice(int start, int length) => _array.AsSpan(start, length);
30+
public Span<T> Slice(int start, int length)
31+
{
32+
ArgumentOutOfRangeException.ThrowIfGreaterThan(start + length, _length, nameof(length));
33+
return _array.AsSpan(start, length);
34+
}
4235

4336
public readonly void Dispose() => arrayPool.Return(_array);
4437

‎src/Nethermind/Nethermind.Network.Discovery.Test/DiscoveryMessageSerializerTests.cs‎

Lines changed: 141 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,10 @@
1414
using Nethermind.Network.Config;
1515
using Nethermind.Network.Discovery.Messages;
1616
using Nethermind.Network.Enr;
17+
using Nethermind.Network.Test;
1718
using Nethermind.Network.Test.Builders;
1819
using Nethermind.Stats.Model;
20+
using FluentAssertions;
1921
using Nethermind.Serialization.Rlp;
2022
using NUnit.Framework;
2123

@@ -141,12 +143,7 @@ public void Enr_request_contains_hash()
141143
[Test]
142144
public void Enr_response_there_and_back()
143145
{
144-
NodeRecord nodeRecord = new();
145-
nodeRecord.SetEntry(new Secp256K1Entry(_privateKey.CompressedPublicKey));
146-
nodeRecord.EnrSequence = 5;
147-
NodeRecordSigner signer = new(new Ecdsa(), _privateKey);
148-
signer.Sign(nodeRecord);
149-
EnrResponseMsg msg = new(TestItem.PublicKeyA, nodeRecord, TestItem.KeccakA);
146+
EnrResponseMsg msg = BuildEnrResponse(_privateKey.CompressedPublicKey);
150147

151148
IByteBuffer serialized = _messageSerializationService.ZeroSerialize(msg);
152149
EnrResponseMsg deserialized = _messageSerializationService.Deserialize<EnrResponseMsg>(serialized);
@@ -156,6 +153,134 @@ public void Enr_response_there_and_back()
156153
Assert.That(deserialized.NodeRecord.Signature, Is.EqualTo(msg.NodeRecord.Signature));
157154
}
158155

156+
[Test]
157+
public void Enr_response_deserialize_does_not_leak_buffer_on_invalid_signature()
158+
{
159+
// ENR with mismatched signature: Secp256K1 entry uses differentKey, but ENR is
160+
// signed with _privateKey. The outer Discovery envelope is valid, but the inner
161+
// ENR signature verification fails because the recovered signer doesn't match.
162+
PrivateKey differentKey = new("3a1076bf45ab87712ad64ccb3b10217737f7faacbf2872e88fdd9a537d8fe266");
163+
EnrResponseMsg msg = BuildEnrResponse(differentKey.CompressedPublicKey);
164+
IByteBuffer serialized = _messageSerializationService.ZeroSerialize(msg);
165+
int refCountBefore = serialized.ReferenceCount;
166+
167+
_messageSerializationService
168+
.Invoking(s => s.Deserialize<EnrResponseMsg>(serialized))
169+
.Should().Throw<NetworkingException>()
170+
.Where(ex => ex.Message.Contains("Invalid ENR signature"));
171+
172+
// Buffer refcount should not have increased — no retained slices or leaked copies
173+
serialized.ReferenceCount.Should().Be(refCountBefore,
174+
"deserializer should not retain additional references to the buffer on error");
175+
serialized.SafeRelease();
176+
}
177+
178+
[Test]
179+
public void Enr_response_deserialize_does_not_leak_buffer_on_success()
180+
{
181+
EnrResponseMsg msg = BuildEnrResponse(_privateKey.CompressedPublicKey);
182+
IByteBuffer serialized = _messageSerializationService.ZeroSerialize(msg);
183+
int refCountBefore = serialized.ReferenceCount;
184+
185+
EnrResponseMsg deserialized = _messageSerializationService.Deserialize<EnrResponseMsg>(serialized);
186+
187+
serialized.ReferenceCount.Should().Be(refCountBefore,
188+
"deserializer should not retain additional references to the buffer on success");
189+
deserialized.NodeRecord.EnrSequence.Should().Be(5);
190+
deserialized.RequestKeccak.Should().Be(TestItem.KeccakA);
191+
serialized.SafeRelease();
192+
}
193+
194+
[Test]
195+
public void Ping_deserialize_does_not_leak_buffer()
196+
{
197+
PingMsg msg = new(_privateKey.PublicKey, 60 + _timestamper.UnixTime.MillisecondsLong,
198+
new IPEndPoint(IPAddress.Parse("192.168.1.1"), 30303),
199+
new IPEndPoint(IPAddress.Parse("192.168.1.2"), 30303),
200+
new byte[32])
201+
{
202+
FarAddress = _farAddress
203+
};
204+
IByteBuffer serialized = _messageSerializationService.ZeroSerialize(msg);
205+
int refCountBefore = serialized.ReferenceCount;
206+
207+
_messageSerializationService.Deserialize<PingMsg>(serialized);
208+
209+
serialized.ReferenceCount.Should().Be(refCountBefore,
210+
"deserializer should not retain additional references to the buffer");
211+
serialized.SafeRelease();
212+
}
213+
214+
[Test]
215+
public void Pong_deserialize_does_not_leak_buffer()
216+
{
217+
PongMsg msg = new(_privateKey.PublicKey, 60 + _timestamper.UnixTime.MillisecondsLong, new byte[] { 1, 2, 3 })
218+
{
219+
FarAddress = _farAddress
220+
};
221+
IByteBuffer serialized = _messageSerializationService.ZeroSerialize(msg);
222+
int refCountBefore = serialized.ReferenceCount;
223+
224+
_messageSerializationService.Deserialize<PongMsg>(serialized);
225+
226+
serialized.ReferenceCount.Should().Be(refCountBefore,
227+
"deserializer should not retain additional references to the buffer");
228+
serialized.SafeRelease();
229+
}
230+
231+
[Test]
232+
public void FindNode_deserialize_does_not_leak_buffer()
233+
{
234+
FindNodeMsg msg = new(_privateKey.PublicKey, 60 + _timestamper.UnixTime.MillisecondsLong, new byte[] { 1, 2, 3 })
235+
{
236+
FarAddress = _farAddress
237+
};
238+
IByteBuffer serialized = _messageSerializationService.ZeroSerialize(msg);
239+
int refCountBefore = serialized.ReferenceCount;
240+
241+
_messageSerializationService.Deserialize<FindNodeMsg>(serialized);
242+
243+
serialized.ReferenceCount.Should().Be(refCountBefore,
244+
"deserializer should not retain additional references to the buffer");
245+
serialized.SafeRelease();
246+
}
247+
248+
[Test]
249+
public void Neighbors_deserialize_does_not_leak_buffer()
250+
{
251+
NeighborsMsg msg = new(_privateKey.PublicKey, 60 + _timestamper.UnixTime.MillisecondsLong,
252+
new[] { new Node(TestItem.PublicKeyA, "192.168.1.2", 1) })
253+
{
254+
FarAddress = _farAddress
255+
};
256+
IByteBuffer serialized = _messageSerializationService.ZeroSerialize(msg);
257+
int refCountBefore = serialized.ReferenceCount;
258+
259+
_messageSerializationService.Deserialize<NeighborsMsg>(serialized);
260+
261+
serialized.ReferenceCount.Should().Be(refCountBefore,
262+
"deserializer should not retain additional references to the buffer");
263+
serialized.SafeRelease();
264+
}
265+
266+
[Test]
267+
public void Neighbors_deserialize_does_not_leak_buffer_on_port_zero_rejection()
268+
{
269+
NeighborsMsg msg = new(_privateKey.PublicKey, 60 + _timestamper.UnixTime.MillisecondsLong,
270+
new Node[] { new(TestItem.PublicKeyA, "192.168.1.2", 0) })
271+
{
272+
FarAddress = _farAddress
273+
};
274+
IByteBuffer serialized = _messageSerializationService.ZeroSerialize(msg);
275+
int refCountBefore = serialized.ReferenceCount;
276+
277+
Assert.Throws<NetworkingException>(() => _messageSerializationService.Deserialize<NeighborsMsg>(serialized));
278+
279+
serialized.ReferenceCount.Should().Be(refCountBefore,
280+
"deserializer should not retain additional references to the buffer on error");
281+
serialized.SafeRelease();
282+
}
283+
159284
[Test]
160285
public void Ping_with_node_id_address_is_rejected()
161286
{
@@ -240,6 +365,16 @@ public void NeighborsMessageTest()
240365
}
241366
}
242367

368+
private EnrResponseMsg BuildEnrResponse(CompressedPublicKey enrPublicKey)
369+
{
370+
NodeRecord nodeRecord = new();
371+
nodeRecord.SetEntry(new Secp256K1Entry(enrPublicKey));
372+
nodeRecord.EnrSequence = 5;
373+
NodeRecordSigner signer = new(new Ecdsa(), _privateKey);
374+
signer.Sign(nodeRecord);
375+
return new EnrResponseMsg(TestItem.PublicKeyA, nodeRecord, TestItem.KeccakA);
376+
}
377+
243378
[Test]
244379
public void NeighborsMessage_Rejects_Port_Zero()
245380
{

‎src/Nethermind/Nethermind.Network.Discovery.Test/Nethermind.Network.Discovery.Test.csproj‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
</PropertyGroup>
88

99
<ItemGroup>
10+
<PackageReference Include="FluentAssertions" />
1011
<PackageReference Include="Nethermind.DotNetty.Transport" />
1112
</ItemGroup>
1213

‎src/Nethermind/Nethermind.Network.Discovery/Serializers/EnrResponseMsgSerializer.cs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
using Autofac.Features.AttributeFilters;
55
using DotNetty.Buffers;
66
using Nethermind.Core.Crypto;
7+
using Nethermind.Core.Extensions;
78
using Nethermind.Crypto;
89
using Nethermind.Network.Discovery.Messages;
910
using Nethermind.Network.Enr;
@@ -49,7 +50,7 @@ public EnrResponseMsg Deserialize(IByteBuffer msgBytes)
4950
NodeRecord nodeRecord = _nodeRecordSigner.Deserialize(ref ctx);
5051
if (!_nodeRecordSigner.Verify(nodeRecord))
5152
{
52-
string resHex = data.ReadBytes(positionForHex).ReadAllHex();
53+
string resHex = data.AsSpan()[..positionForHex].ToHexString();
5354
throw new NetworkingException($"Invalid ENR signature: {resHex}", NetworkExceptionType.Discovery);
5455
}
5556

0 commit comments

Comments
 (0)