net: receive TCP urgent data inline #36688

darosior commented at 6:58 PM on October 28, 2026: member

There is currently nothing preventing us to receive TCP urgent data, which would never be returned by recv(). Linux's poll() is indifferent to this, but the select() socket monitoring may keep reporting it as pending on some platforms.

This would not prevent progress nor lead to a crash anymore after #36665, but it's still undesirable. Clean this up by setting SO_OOBINLINE on our sockets, so recv() returns any urgent data sent to us (and likely leads us to disconnect the peer).

This is a slight behaviour change on Linux, but i think a desirable one. First of all because if a peer is sending us funky data we might as well hear about it and disconnect them, but also because i think one lesson to draw from the recent socket related security vulnerability is that we should reduce the amount of custom logic for less well-tested platforms.

Failing to set SO_OOBINLINE was conservatively made a warning and not a failure, because its absence does not prevent node operations, it's just annoying.

DrahtBot added the label P2P on Oct 28, 2026

DrahtBot commented at 6:58 PM on October 28, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36688
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36688.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36688#author-ralph

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	vasild, optout21, pinheadmz, winterrdog, xyzconstant, ViniciusCestarii, jpk68

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36434 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36434.svg"></sub> (http: support unix sockets by pinheadmz)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

pinheadmz commented at 7:07 PM on October 28, 2026: member

concept ACK "urgent" data is dumb https://datatracker.ietf.org/doc/html/rfc6093#section-6

ViniciusCestarii commented at 8:12 PM on October 28, 2026: contributor

ACK 4655c2305e80ef19353739a6f66e49442945ed44

Ran the test and checked it fails with the SO_OOBINLINE change reverted

DrahtBot requested review from pinheadmz on Oct 28, 2026

optout21 commented at 10:08 PM on October 28, 2026: contributor

ACK 4655c2305e80ef19353739a6f66e49442945ed44

Reviewed the code, verified the test locally (on Linux). Checked that without setting the option the new test fails. Checked other socket creation places -- other instance found only in ipc/capnp. Checked other SetSockOpt usages. Looked into why TCP_NODELAY option is set again after accept, while SO_OOBINLINE is not. Apparently even setting TCP_NODELAY again is extra cautious, and not needed on any of the supported platforms. Inheritance of SO_OOBINLINE should be even more reliable, as it is a socket-level option, not TCP level. If the socket option is not inherited, the test would fail. Verified this by unsetting SO_OOBINLINE right after the Accept in the test (that causes the test to fail).

I would've liked more adding the test first, then adding the fix and updating the test. It's fine this way too.

xyzconstant commented at 2:10 AM on October 29, 2026: contributor

ACK 4655c230

Receiving urgent data inline makes total sense. Nice test and code change.

in src/test/netbase_tests.cpp:643 in 4655c2305e

 638 | +    // Connect to server from client. Use `CreateSockOS` directly instead of the `ConnectDirectly`
 639 | +    // helper to make sure the peer doesn't become a mock (which could happen for instance if
 640 | +    // the test suite was updated to use SocketTestingSetup instead of BasicTestingSetup).
 641 | +    const auto peer{CreateSockOS(AF_INET, SOCK_STREAM, IPPROTO_TCP)};
 642 | +    BOOST_REQUIRE(peer);
 643 | +    (void)peer->Connect(reinterpret_cast<const sockaddr*>(&addr), sizeof(addr));

vasild commented at 9:04 AM on October 29, 2026:

nit: GetSockName() fills addr with some data and sets its size in the out parameter addr_len. I think logically, Connect() should take addr_len, not sizeof(addr). I guess this is mostly theoretical concern because my understanding is that both will be equal. Maybe add BOOST_REQUIRE_EQUAL(addr_len, sizeof(addr));, or s/sizeof(addr)/addr_len/ or just ignore.

darosior commented at 1:43 PM on October 29, 2026:

Yes, using addr_len is cleaner.

darosior commented at 1:58 PM on October 29, 2026:

Done.

in src/netbase.cpp:574 in 4655c2305e

 568 | @@ -569,6 +569,12 @@ std::unique_ptr<Sock> CreateSockOS(int domain, int type, int protocol)
 569 |          if (sock->SetSockOpt(IPPROTO_TCP, TCP_NODELAY, &on, sizeof(on)) == SOCKET_ERROR) {
 570 |              LogDebug(BCLog::NET, "Unable to set TCP_NODELAY on a newly created socket, continuing anyway\n");
 571 |          }
 572 | +
 573 | +        // Receive TCP urgent data inline, like any other data. Out of band, it is not returned by
 574 | +        // recv() but some platforms keep reporting it as pending. Accepted sockets inherit this.

vasild commented at 9:11 AM on October 29, 2026:

I think it would be useful to mention which platforms report it as pending.

darosior commented at 1:59 PM on October 29, 2026:

Updated comment to apply to all platforms using select() to monitor.

vasild approved

vasild commented at 9:13 AM on October 29, 2026: contributor

ACK 4655c2305e80ef19353739a6f66e49442945ed44

Linux's poll() is indifferent to this, but the select() socket monitoring may keep reporting it as pending on some platforms.

Do you plan to completely remove the select() branch and use poll() on all platforms?

<details> <summary>Show Signature</summary>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK 4655c2305e80ef19353739a6f66e49442945ed44

> Linux's poll() is indifferent to this, but the select() socket monitoring may keep reporting it as pending on some platforms.

Do you plan to completely remove the `select()` branch and use `poll()` on all platforms?
-----BEGIN PGP SIGNATURE-----

iQRPBAEBCAA5FiEE5k2NRWFNsHVF2czBVN8G9ktVy78FAmrjDhEbFIAAAAAABAAO
bWFudTIsMi41KzEuMTIsMiwzAAoJEFTfBvZLVcu/EzMgAIP+VL+22/8xlzvwT4oj
s96hKoKR5iukh2Lyd7M3VIhCG/ltDcQD93bO92TrvJABHxRTeAWOzYGogJlg02js
LlIF9/6iliUXlB8JyhFpWaAAb8cygAVjmK0IsAFzWOmaH96b+7PDpLR0mgN7yce9
Yrv5Ag/PNcsd0APnGJSuONGfonPWr98QFL2yVQ/0RoSt9PUh4pjF74Ejvj5h0pSr
23C0zMGudapj3TgBcdsmi455YD11DInkJdltBex3QnCa/zQIcroYv1RAYsZx+lIu
ZlZbcJlI7SiRLPfGCaseQv8RMa3WO5CD6HgO7F8gQlWuvTa+RXTZKIgTyMSkxIaU
hw1roQ/Fv9LdiC6Z7Tn/vQzBhKSc6EeUSJKE1O8d8FdABHkOo1cxJJLzOVxiGh6u
7RR7fZIfhSrb4oN7H6kfVyS5PZmuE4FlH2y5UakdJHqq3T99SkQnz/3syFSvay9J
zGRcMC2a5+lv38guZHHNYJLGSSxD0z8DbT+jU8uP0ji6UslhHY8LRbk04UbFQKh5
h27p+PpNoP3TmleXWLbJZga9eCNSQXul37JV7+lgqciYGGJcrKUYYV8r8wkdpR5T
kxw1SDSdIM5KQBnSPIH29fU5FN2DXqrPtyYXPPGC8Gg01e76KBtGzNk27Wb/LSP7
iu0Zyx1QruhsHdF4IvUkPVlFWlsRtDPxF6pZSE/OUD6uaJ2jwuuQdB01sqbTaDjf
QlRnddEci9GMAWJ8cvyenNowgDruk6U9FbeTUvCematHrcrXd6IntAZHQbH5bRxy
HQ5RPdpUAqMgMXr01hlnZEHmi3wHSdOBONVOUUi+JLa5aFmkD/18cBsjIevv22wq
VA9ETbhKeNfQNWm4GrBrAfCKJkU48kHe11qHUqq8Ige7z3GDUlzk4ELSpjALGkL/
UYdeoEk1QZpuc9jt+rmFmi7QxWWCdn26E95Lxd9kC3SZZI18rVriF/oEmxt1hvTx
vhWKA+zfcU7d3u0K89DI3MsBykuW0417RJIFAdNCc5f5LekgB7oTWNLvKUqO3q92
HOlDivcg07kL4BrkCd8gZ459yAb3J+rrOn+OzhxZox1a6TiT53H0MlvQajv3liMK
ybGyu8VBl0g32ajm6CS6y0bjIneZ8++dUJh1p1RtGnkh1ux4Fun/MIeWUZnGFRW/
vXdQ0oPT4AKfEuWMriOD48yECYJe7E0XPwVzQTtO2MforCnf5GwD+z23cUhh+D3x
chKd0CpITKAVayBDz3dBBNv/ITL1b58vP7u0D6kZ4Pu6wcbQsaqiwTehihKC9QJk
4Hy0yzspZ3g6wodtLGd4+CGTegZmVh8rQbndyaMijdfcCb9EqtVHSaIUOWAfymHE
5WI=
=aXRI
-----END PGP SIGNATURE-----

vasild's public key is on openpgp.org

</details>

fanquake added this to the milestone 33.0 on Oct 29, 2026

darosior force-pushed on Oct 29, 2026

net: receive TCP urgent data inline

Set SO_OOBINLINE on TCP sockets, to receive urgent data like any other
data. Otherwise, recv() does not return it, while select() keeps
reporting it as pending on some platforms.

23fe594b5b

qa: test TCP urgent data is received inline de87cb8407

pinheadmz commented at 2:01 PM on October 29, 2026: member

While all the reviews are stale, quick question: should the test live in sock_tests.cpp? There is a struct TcpSocketPair that might be handy

darosior commented at 2:06 PM on October 29, 2026: member

Thought about this, but CreateSockOS is netbase's function, which is a caller of util/sock. I think sock_tests should be contained to testing util/sock: it would awkward to include netbase.h there when we have netbase_tests.

vasild commented at 2:18 PM on October 29, 2026: contributor

ACK de87cb84079105968d10c8208565e65d5e7670c5

<details> <summary>Show Signature</summary>

vasild's public key is on openpgp.org

</details>

DrahtBot requested review from ViniciusCestarii on Oct 29, 2026

optout21 commented at 2:41 PM on October 29, 2026: contributor

reACK de87cb84079105968d10c8208565e65d5e7670c5

in src/netbase.cpp:575 in de87cb8407

 568 | @@ -569,6 +569,12 @@ std::unique_ptr<Sock> CreateSockOS(int domain, int type, int protocol)
 569 |          if (sock->SetSockOpt(IPPROTO_TCP, TCP_NODELAY, &on, sizeof(on)) == SOCKET_ERROR) {
 570 |              LogDebug(BCLog::NET, "Unable to set TCP_NODELAY on a newly created socket, continuing anyway\n");
 571 |          }
 572 | +
 573 | +        // Receive TCP urgent data inline, like any other data. Out of band, it is not returned by
 574 | +        // recv() but select() reports it as pending.
 575 | +        if (sock->SetSockOpt(SOL_SOCKET, SO_OOBINLINE, &on, sizeof(on)) == SOCKET_ERROR) {

pinheadmz commented at 2:49 PM on October 29, 2026:

nit if u touch, on and set above could be cleaned up like we did in httpserver.cpp

static constexpr int SOCKET_OPTION_TRUE{1};

pinheadmz approved

pinheadmz commented at 2:52 PM on October 29, 2026: member

ACK de87cb84079105968d10c8208565e65d5e7670c5

built and tested on macos/arm64. Generated functional tests that send urgent bytes to the http server: https://gist.github.com/pinheadmz/f5bb77865f870865e44c54d5751b569a these pass on the branch and fail on master. I also wanted to check what happens if the "urgent" flag is set but the urgent byte itself has not been sent yet.

<details><summary>Show Signature</summary>

pinheadmz's public key is on openpgp.org

</details>

darosior commented at 3:03 PM on October 29, 2026: member

Do you plan to completely remove the select() branch and use poll() on all platforms?

Yes. I am currently working on pinning down all the OS-specific behaviour each implementation is relying on first, to provide more assurance a switch to poll() is safe (and help minimize the risk of more security vulns in our socket logic in the future).

winterrdog commented at 3:12 PM on October 29, 2026: contributor

ACK de87cb84079105968d10c8208565e65d5e7670c5

xyzconstant commented at 3:40 PM on October 29, 2026: contributor

re-ACK de87cb84079105968d10c8208565e65d5e7670c5

ViniciusCestarii commented at 4:51 PM on October 29, 2026: contributor

reACK de87cb84079105968d10c8208565e65d5e7670c5

darosior commented at 8:38 PM on October 29, 2026: member

Let's hold off on merging this, i think i found something wrong here.

darosior commented at 10:04 PM on October 29, 2026: member

The patch here is correct, but insufficient to address the issue entirely on macOS/BSD.

This comment from @pinheadmz got me wondering:

I also wanted to check what happens if the "urgent" flag is set but the urgent byte itself has not been sent yet.

So i checked this as part of my socket tests hardening work, and it turns out that if you also set the urgent data pointer in the TCP segment header past the last byte you sent, select() on macOS / BSD keeps waking up while the socket is waiting to receive, until data past the mark has been read. Testing this is annoying (and can't reliably be done in our test suite) because the socket API won't let you set that pointer arbitrarily. So this requires writing IP packets directly in a raw socket, or some more involved hackery on Windows [^1].

So a peer receiving such a packet on:

Linux is unaffected (poll() without POLLPRI)
Windows will not raise the exception set (with SO_OOBINLINE)
macOS will raise the exception set (it does so as soon as it sees the pointer, even if the byte itself hasn't arrived yet, regardless of SO_OOBINLINE)

Thankfully the exception set is only necessary on Windows, to detect a failed connection attempt. So we could ifdef it away, which would fix the macOS/BSD issue. But this is a bigger change, and i think the right solution is to just move to using poll() for all platforms.

Given the patch in this PR is simple, reviewed, and already fixes Windows, i think the complete fix for macOS should happen in a followup that either switches it to using poll() or gates the exception set to only be set for Windows.

[^1]: My bot was able to test this by crafting Ethernet frame and sending them to QEMU over UDP for it to inject them into the emulated NIC of the Windows VM.. Whatever.

darosior commented at 10:05 PM on October 29, 2026: member

Looks like Ralph had caught the issue, too. cc @willcl-ark

jpk68 commented at 10:55 PM on October 29, 2026: contributor

ACK de87cb84079105968d10c8208565e65d5e7670c5

Sjors commented at 7:29 AM on October 30, 2026: member

Would it make sense to add this?

diff --git a/src/net.cpp b/src/net.cpp
index 9bc7887a3c..3e6c1a0ae3 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -2200,4 +2200,15 @@ void CConnman::SocketHandlerConnected(const std::vector<CNode*>& nodes,
         }
 
+#ifndef USE_POLL
+        if (errorSet) {
+            // select() can report an exceptional condition that recv() cannot clear,
+            // such as urgent data if SO_OOBINLINE failed, or an urgent pointer to
+            // data not yet received. Disconnect instead of repeatedly waking.
+            LogDebug(BCLog::NET, "socket exceptional condition, %s", pnode->DisconnectMsg());
+            pnode->CloseSocketDisconnect();
+            continue;
+        }
+#endif
+
         if (sendSet) {
             // Send data

In other words: hang up if we receive an urgent byte, on platforms with select().

We could avoid SO_OOBINLINE altogether if we treat the presence of URG (after a connection is established) as misbehavior. We could do that on poll() implementations too, even though it's not a problem there currently.

I'm not sure if the errorSet() can be triggered by issues other than a peer sending URG, besides what we already handle in ConnectToSocket. That might cause an "underserved" disconnect. But with SO_OOBINLINE such conditions would probably put random bytes in the buffer that would lead to a disconnect, per the original motivation of this PR.

More subjectively, I find SO_OOBINLINE to be a confusing feature. So if we don't need it, that would be nice.

vasild commented at 8:42 AM on October 30, 2026: contributor

The patch is fine and I retain my ACK as it is, but this angle is worth considering:

More subjectively, I find SO_OOBINLINE to be a confusing feature. So if we don't need it, that would be nice.

So, we do not use out-of-bound data and we do not care about it, but we would set SO_OOBINLINE even though we do not expect to receive any OOB, just to fix a problem elsewhere, hmm?

From this angle, @Sjors's idea to disconnect if we receive OOB seems more in line with our expectations to not receive OOB. Also, the #ifndef USE_POLL snippet would naturally go away if we use poll(2) on all platforms.

darosior commented at 2:31 PM on October 30, 2026: member

@Sjors @vasild i think that works but is fragile. The exception set is not specific to urgent data, it is a general set of descriptors being watched for "exceptional conditions". In practice, it is only used for urgent data for sockets on POSIX, but Windows also uses it to report failed outgoing connections for non-blocking sockets. So your patch works because it only disconnects on exceptions after the connection was established. I think this is brittle to rely on the exception set to always indicate presence of urgent data, and to implement a low-level socket check at the connection handler level.

More subjectively, I find SO_OOBINLINE to be a confusing feature. So if we don't need it, that would be nice.

I would say the opposite, actually. It's the default of pulling the urgent byte out of the stream which is confusing, and SO_OOBINLINE restores the socket API to behave as you would expect. As mentioned above, even RFC 6093 calls the default "broken" and states that SO_OOBINLINE restores the "semantics intended by the IETF specifications".

I think a better approach is what i mentioned in my earlier message: at the socket level, only ever register the exception set for Windows.

--- a/src/util/sock.cpp
+++ b/src/util/sock.cpp
@@ -216,7 +216,11 @@ bool Sock::WaitMany(std::chrono::milliseconds timeout, EventsPerSock& events_per
         const auto& s = sock->m_socket;
         if (events.requested & RecvEvent) {
             FD_SET(s, &recv);
+#ifdef WIN32
+            // Windows reports a failed non-blocking connect() only in the exception set. Other
+            // platforms report it as readable and writable.
             FD_SET(s, &err);
+#endif
         }
         if (events.requested & SendEvent) {
             FD_SET(s, &send);

That would solve the remaining issue in this PR. I would still prefer ditching select() for macOS/BSD, but i guess we could do both, so we can backport that smaller change.