Skip to content

conn_linux: copy pooled receive buffers before parsing#284

Merged
nickgarlis merged 1 commit intomdlayher:mainfrom
ivanlevitsky:fix-pooled-receive-buffer-aliasing
Apr 13, 2026
Merged

conn_linux: copy pooled receive buffers before parsing#284
nickgarlis merged 1 commit intomdlayher:mainfrom
ivanlevitsky:fix-pooled-receive-buffer-aliasing

Conversation

@ivanlevitsky
Copy link
Copy Markdown
Contributor

Problem

On Linux, when Config.MessageBufferSize is enabled, receive buffers are reused from a pool.
Message.UnmarshalBinary keeps Message.Data as a slice into the parsed input buffer, so returned messages may reference pooled memory that is reused by later receives.

This is especially visible when responses are read across multiple recvmsg calls (for example, multipart netlink dumps): a later receive can overwrite data from previously returned messages.

Root cause

ReceiveIter parsed messages directly from the pooled receive buffer and then returned that buffer to the pool before callers were necessarily done using Message.Data.

Fix

  • Add ownedBuffer in conn_linux.go and parse from it in ReceiveIter.
  • Non-pooled path: keep existing behavior (b[:nlmsgAlign(n)]) with no extra copy.
  • Pooled path: copy n received bytes into a fresh aligned buffer and parse from that owned memory.

This keeps the change minimal and targeted to the pooled-buffer path only.

Tests

Add integration regression test:

  • TestIntegrationConnMessageBufferSizeCopiesMessageData in conn_linux_integration_test.go.
  • Uses NETLINK_USERSOCK with MessageBufferSize enabled.
  • Sends two datagrams with different payloads.
  • Verifies payload from the first Receive() remains unchanged after the second Receive().

When MessageBufferSize is set, ReceiveIter parses messages from a pooled
buffer and then returns that buffer to the pool. Message.UnmarshalBinary
keeps Message.Data as a slice into the input buffer, so later receives can
overwrite previously returned message payloads.

Fix this by parsing from owned memory in the pooled path: copy the received
bytes into a fresh aligned buffer before parseMessagesIter. The non-pooled
path is unchanged and still avoids the extra copy.

Add an integration regression test to verify that payload from the first
Receive is not modified by a subsequent Receive when MessageBufferSize is
enabled.
@nickgarlis
Copy link
Copy Markdown
Collaborator

Thanks for this PR @ivanlevitsky !

I had a quick look and I think this looks good. I am wondering whether another alternative could be to copy into Message.Data during Message.UnmarshalBinary. This could maybe simplify things and copy less bytes overall. Of course, this would have the downside of slightly penalizing performance for Message.UnmarshalBinary but I don't think that's not significant enough. WDYT ?

@ivanlevitsky
Copy link
Copy Markdown
Contributor Author

Thanks for this PR @ivanlevitsky !

I had a quick look and I think this looks good. I am wondering whether another alternative could be to copy into Message.Data during Message.UnmarshalBinary. This could maybe simplify things and copy less bytes overall. Of course, this would have the downside of slightly penalizing performance for Message.UnmarshalBinary but I don't think that's not significant enough. WDYT ?

Thank you, this is a very reasonable alternative.

I did consider copying in Message.UnmarshalBinary, but I intentionally kept the copy out of that layer:

  • UnmarshalBinary is currently a low-level zero-copy primitive used in multiple paths. Copying there would introduce allocations globally, including paths not affected by this bug.
  • This issue is specific to Linux pooled receives (MessageBufferSize), so fixing it in conn_linux keeps the overhead localized to the problematic path.
  • The current change does one allocation/copy per recvmsg datagram in the pooled path. Copying in UnmarshalBinary would typically allocate per parsed message, which is usually less favorable for multipart responses.

So I’d prefer to keep this targeted fix. If you’d like, we can discuss changing UnmarshalBinary ownership semantics separately.

@nickgarlis
Copy link
Copy Markdown
Collaborator

Thanks for the detailed explanation!

If you’d like, we can discuss changing UnmarshalBinary ownership semantics separately.

Yes, do let me know if you think there are ways it can be improved.

@nickgarlis nickgarlis merged commit bf38b16 into mdlayher:main Apr 13, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants