Support TCP for protocol messages

[softins]

Short description of changes

Support fallback to TCP for protocol messages, in order to overcome potential loss of large messages due to UDP fragmentation. Currently an incomplete draft, for comment as development continues.

CHANGELOG: Client/Server: Support TCP fallback for protocol messages.

Context: Fixes an issue?

Discussed in issue #3242.

Does this change need documentation? What needs to be documented and how?

It will need documentation once design and development are complete. Particularly need to explain the firewall requirements for a server or directory.

Status of this Pull Request

Incomplete, still under development. Main server side complete and working. Client side development in progress. Complete and ready for review and testing. Still marked draft as it needs some of the debug messages to be commented out before merging.

What is missing until this pull request can be merged?

A lot of testing of both server and client. Intended for Jamulus 4.0.0.

Checklist

• I've verified that this Pull Request follows the general code principles
• I tested my code and it does what I want
• My code follows the style guide
• I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
• I've filled all the content above

[softins]

So far, this implements the server side of the design described here and here

[softins]

So the next stage of implementation has been achieved: client-side support in the Connect dialog.

1. If the server list has not been received via UDP when the associated message indicating TCP support has arrived, the client will retry fetching the server list over TCP.
2. If the client list for a server has not been received via UDP when the associated message indicating TCP support has arrived, the client will retry fetching the client list over TCP, and will continue to use TCP for that server while the Connect dialog is open.
3. A directory or server that does not have TCP support will not send the TCP supported message, and will continue to be handled as in current versions.
4. If the server list or client list is successfully received over UDP, there is no need for the client to try TCP.

It has been tested by using nft to drop outbound Jamulus UDP messages with a specific message ID, to simulate loss due to fragmentation.

Examples for a directory-enabled server running on port 22120:

• drop UDP server list: nft add rule inet filter output udp sport 22120 @ih,16,16 0xee03 drop
• drop UDP client list: nft add rule inet filter output udp sport 22120 @ih,16,16 0xf503 drop
• drop UDP "TCP supported" msg: nft add rule inet filter output udp sport 22120 @ih,16,16 0xfb03 drop

Note that nft rules require network byte order (big-endian), but Jamulus IDs are little-endian:

• CLM_SERVER_LIST = 1006 = 0x03ee => 0xee03 (LE byte order)
• CLM_RED_SERVER_LIST = 1018 = 0x03fa => 0xfa03 (LE byte order)
• CLM_CONN_CLIENTS_LIST = 1013 = 0x03f5 => 0xf503 (LE byte order)
• CLM_TCP_SUPPORTED = 1019 = 0x03fb => 0xfb03 (LE byte order)

[softins]

The next step is to try implementing the connected-mode TCP described here

[ann0see]

Since we'll have a long time for the 4.0 release, I'd enable it by default soon (of course once we've tested that the basics work)

[softins]

No, I disagree. It's a server-only option, and most servers operators will not need to enable TCP support. Only those running large directories or large servers will need to, and they also need to understand and configure their firewall requirements.

TCP support in the client will indeed be enabled by default, but will only take effect when talking to a directory or server that has enabled it.

If a server operator enables TCP without having configured their firewall correctly, client users could have problems as the server would advertise TCP support to the client, but the client could be unable to connect.

[ann0see]

Can we not give an error message or fallback procedure in case the TCP connection timed out?

[softins]

Yes, I'm sure we can. I haven't yet tested that scenario.

But it doesn't negate my view that server-side TCP support needs to be an explicit option.

[ann0see]

I assume we have the timer now?
https://github.com/jamulussoftware/jamulus/pull/3636/changes#diff-7bc46552432fa03e009f9f455e4729312853d92775935ef20849797f24ac0c60R292-R300

[pljones]

Since when do we have a long time?

There's absolutely no benefit to anyone involved in the project - developers or users - in having long release cycles.

A version cut takes very little time - apart from the translation process. Even then, getting used to more, small changes is likely to speed things up generally.

[softins]

Well I've finished implementing everything I intended to, for directory, server and client, so it's ready for reviewing and trying out, as and when time permits (post 3.12.0).

I have a private directory and server built and running with TCP support, at newjam.softins.co.uk on the standard port 22124.

In order to demonstrate the use of TCP in a new client's connect dialog, it will be necessary to use custom firewall filters on the client end to temporarily drop incoming UDP Jamulus protocol messages containing a server list or connected clients list.

There is full forward and backward compatibility between clients and servers built with TCP support and older versions.

[softins]

Keeping as draft, because it will need quite a few debug messages removed before merging.

[pljones]

This comment needs clarifying: nowhere in the close neighbourhood is TCP_CONNECT_TIMEOUT_MS mentioned. (I can see it's kicked off after all the set up bits have set things up - maybe just mention that.)

[pljones]

Very nice. It makes it more like "here's another way the to handle protocol messages".

[ann0see]

I think we should make clear why the connectionless protocol is used here - I suppose since TCP handles the session, it's enough.

[ann0see]

Suggested change

	qWarning() << "Unsatisfied Client List request via UDP for" << InetAddr.toString();
	qWarning() << "Unsatisfied Client List request via UDP for " << InetAddr.toString();

[softins]

This isn't needed. qWarning() and its relatives automatically put a space between << items.

[ann0see]

Suggested change

	qWarning() << "Unsatisfied Client List request via TCP for" << InetAddr.toString() << "(switching back to UDP)";
	qWarning() << "Unsatisfied Client List request via TCP for " << InetAddr.toString() << "(switching back to UDP)";

[softins]

Not needed, as above

[ann0see]

We might want to log this in production.

[softins]

Possibly. I was actually considering removing it, as it can be quite noisy if a directory contains servers that are either not configured properly or very distant.

[ann0see]

Any security implications if the client sends a wrong id?

[softins]

Not that I can think of, because I made sure the server will only accept it if it comes from the correct IP address. Otherwise it's ignored.

[ann0see]

Any reason why we need to pass CTcpConnection that often and cannot just pass it once to the class?

[softins]

Because we have a separate TCP connection with each different peer.

[ann0see]

CreateCLEmptyMes could also have a default argument == nullptr I suppose.

[softins]

Possibly, I'll check.

[ann0see]

Thanks

[ann0see]

I disagree. The CLM_TCP_SUPPORTED message is not just to let the client know we support TCP. It also enables the client to know that it didn't receive the expected message via UDP that was sent immediately before it.

Then we should probably rename it to express that it is not just TCP support but rather that it request a TCP connection due to network problems.

[softins]

I disagree. The CLM_TCP_SUPPORTED message is not just to let the client know we support TCP. It also enables the client to know that it didn't receive the expected message via UDP that was sent immediately before it.

Then we should probably rename it to express that it is not just TCP support but rather that it request a TCP connection due to network problems.

But it doesn't mean that. The message is still sent and received if there are no network problems. It is the client that decides what to do with it, by inferring network problems only if CLM_TCP_SUPPORTED is received without being preceded by the expected server or client list. It is only telling the client that the server can support TCP, not that TCP must be used.

[pljones]

It's more "TCP is available as an alternative to the UDP response you were expecting (<this one>) - ask and you'll get it reliably".

[softins]

Just rebased to latest main. Now need to work out how to support the newly-introduced --serverbindip6.

[mcfnord]

Remote null-pointer crash: CLM_CLIENT_ID is reachable over UDP

PROTMESSID_CLM_CLIENT_ID (1024) is a connectionless ID, so ParseConnectionLessMessageBody dispatches it regardless of transport. When it arrives over UDP, CSocket emits ProtocolCLMessageReceived(...) with the new pTcpConnection argument defaulting to nullptr, which flows through EvaluateCLClientIDMes(..., nullptr) → CServer::OnCLClientIDReceived(..., nullptr). That handler dereferences pTcpConnection on both the reject path (pTcpConnection->disconnectFromHost()) and the success path (pTcpConnection->SetChannel(...)).

So a single unsolicited UDP datagram — a valid frame with ID 1024 and a 1-byte body containing any channel number ≥ iMaxNumChannels — crashes any --enabletcp server. No auth is required, the source IP is spoofable, and no session is needed. Simplest to reject the message at the protocol layer when it did not arrive over TCP:

Suggested change

	// check size
	if ( vecData.Size() != 1 )
	{
	return true; // return error code
	}
	// check size
	if ( vecData.Size() != 1 )
	{
	return true; // return error code
	}
	// CLM_CLIENT_ID is only meaningful when received over a TCP connection.
	// If it arrives over UDP, pTcpConnection is nullptr; ignore it, since
	// passing a null connection to the server handler would dereference it.
	if ( pTcpConnection == nullptr )
	{
	return true; // return error code
	}

(Optional defense-in-depth: also early-return in CServer::OnCLClientIDReceived when pTcpConnection == nullptr, so any future caller is protected too.)

[pljones]

    if ( pTcpConnection == nullptr )

Why not do this before checking the size? We should only be expecting this message over TCP so size is irrelevant until that's established.

[mcfnord]

Heap overflow: the TCP frame length is only bounds-checked by Q_ASSERT, which is compiled out in release builds

GetBodyLength returns the on-the-wire 2-byte length field (+2 for the CRC), i.e. up to 65537, and stores it in iPayloadRemain. vecbyRecBuf is fixed at MAX_SIZE_BYTES_NETW_BUF (20000). In a release build (QT_NO_DEBUG) this Q_ASSERT is a no-op, and the body branch just below then calls pTcpSocket->read( (char*) &vecbyRecBuf[iPos], iPayloadRemain ) with iPos climbing past the end of the buffer. ParseMessageFrame would reject the bad length/CRC, but only after the payload has already been read into the buffer — i.e. after the overflow.

A peer that sends a 7-byte header declaring length 0xFFFF and then streams ~25 KB overflows the buffer (a write primitive); debug builds instead hit the assert and abort(). The same applies in both directions (a malicious server can do it to a client that opened a fallback connection). A runtime guard is needed here:

Suggested change

	Q_ASSERT ( iPayloadRemain <= MAX_SIZE_BYTES_NETW_BUF - MESS_HEADER_LENGTH_BYTE );
	// Guard against a malformed or malicious length field. The body
	// plus CRC must fit in the fixed-size receive buffer. Q_ASSERT is
	// compiled out in release builds, so a runtime check is required
	// here to stop an oversized frame from overflowing vecbyRecBuf.
	if ( iPayloadRemain < 0 || iPayloadRemain > MAX_SIZE_BYTES_NETW_BUF - MESS_HEADER_LENGTH_BYTE )
	{
	qWarning() << "- Jamulus-TCP: oversized/invalid frame length" << iPayloadRemain << "- dropping connection";
	disconnectFromHost();
	return;
	}

[mcfnord]

No limit on concurrent TCP connections → memory / fd exhaustion

Every accepted connection constructs a CTcpConnection, whose constructor does vecbyRecBuf.Init ( MAX_SIZE_BYTES_NETW_BUF ) — a 20 KB allocation per connection — and there is no per-IP or global cap on how many may be open at once. QTcpServer::maxPendingConnections only bounds the accept backlog, not established connections.

Unlike the UDP path (which holds no per-peer state), a --enabletcp server now exposes an unauthenticated endpoint where an attacker can open thousands of connections and sit idle up to the idle timeout (5 s initially, 20 s after any byte), each costing ~20 KB plus a file descriptor. That is a cheap resource-exhaustion vector against exactly the large public directories this feature targets.

Worth considering a global and/or per-IP connection cap (refuse/close beyond it), and possibly allocating vecbyRecBuf lazily once a full header shows a real message is arriving, rather than 20 KB up front for every idle connection.

[mcfnord]

Clearing pChannel->pTcpConnection here is not serialised against the channel-table readers (matters under -T)

OnDisconnected runs in the event thread and clears the channel's connection pointer, then deleteLater()s this object — but it does not hold CServer::Mutex. Every other server access to vecChannels[] / pTcpConnection is taken under that mutex, including the send path CChannel::CreateConClientListMes → if ( pTcpConnection ) ConnLessProtocol.CreateCLConnClientsListMes ( ..., pTcpConnection ) → pTcpConnection->write(...).

In multithreading mode (-T) a client disconnecting concurrently with a client-list send can therefore produce a torn read of the pointer, or a synchronous write() on a CTcpConnection that is mid-teardown. Serialising the link/unlink of pChannel->pTcpConnection under the same mutex that guards the channel table would close the window. (In single-thread mode the event loop serialises it, so this is -T-only.)

[softins]

Hmm, slightly confused here whether it is concerned with CServer::Mutex or CChannel::Mutex or both.

[mcfnord]

The 2.5 s connect-dialog re-request timer can pre-empt an in-flight TCP fallback (3 s connect timeout)

While a server list has not arrived, CConnectDlg's re-request timer keeps calling back into CreateCLReqServerListMes. Once a request is in CFM_TCP_REQUEST state, the next tick lands on this branch and reverts it to UDP ("switching back to UDP"), re-inserting CFM_UDP_REQUEST. But TCP_CONNECT_TIMEOUT_MS is 3000 ms while the re-request cadence is ~2.5 s, so on a slow path the timer can abandon a TCP attempt that was about to succeed, fall back to UDP, then re-enter TCP on the next CLM_TCP_SUPPORTED — a UDP/TCP flip-flop that also opens a fresh TCP connection each cycle.

It converges eventually (TCP wins and stops the timer), but potentially only after several cycles, on exactly the marginal links where TCP is needed most. Consider not reverting to UDP while a TCP connect/request is genuinely still outstanding, or making the re-request interval longer than the TCP connect timeout. (The client-list equivalent is at the matching branch of CreateCLServerListReqConnClientsListMes.)

[mcfnord]

pendingServerList / pendingClientList are never cleared, and CFM_UDP_RESULT is unused

These hashes gain an entry per directory/server address and are only ever removed on UDP success; CFM_TCP_RESULT / CFM_TCP_REQUEST entries persist. Nothing clears them when the Connect dialog closes or the directory changes — Start() resets iClientID / bTcpSupported but not these. Two consequences:

1. A server that needed TCP once is remembered as CFM_TCP_RESULT and skips straight to TCP on the next dialog session, even after the fragmentation condition is gone — and if TCP is then blocked, that is a worse first experience than UDP. This also seems to contradict the design note in this thread that the per-server TCP status should be "forgotten when changing directories or on closing the dialog."
2. Slow unbounded growth over the client's lifetime (one entry per distinct address ever queried).

Clearing both hashes in Start() (or on dialog close / directory change) would fix it. Separately, CFM_UDP_RESULT is declared but never assigned or read — either dead code to remove, or the UDP-success paths should set it instead of calling .remove().

[softins]

A lot of useful things to go through - thank you!

[mcfnord]

Note

📡 STAND BY FOR AN LLM-AUTHORED MESSAGE.

While testing this branch I noticed the server-side TCP idle timer uses two different values depending on connection state.

On accept, CTcpConnection starts TimerIdleTimeout with TCP_IDLE_TIMEOUT_MS (5000 ms):

jamulus/src/tcpconnection.cpp

Line 79 in b3db800

TimerIdleTimeout.start ( TCP_IDLE_TIMEOUT_MS );

After the first message is received, OnReadyRead resets it to TCP_KEEPALIVE_INTERVAL_MS + TCP_IDLE_TIMEOUT_MS (20000 ms):

jamulus/src/tcpconnection.cpp

Line 195 in b3db800

TimerIdleTimeout.start ( TCP_KEEPALIVE_INTERVAL_MS + TCP_IDLE_TIMEOUT_MS );

So a freshly accepted TCP connection has only 5 seconds to send its first message (CLM_CLIENT_ID) before the server closes it, while an established connection tolerates 20 seconds of silence. I reproduced this directly: a bare TCP connect that sent no data was closed by the server at 5.32 seconds, logged as Jamulus-TCP: idle timeout - disconnecting.

This could disconnect a legitimate client on a slow or high-latency link, if the UDP CLIENT_ID exchange that triggers the TCP connection (or the TCP handshake itself) takes longer than 5 seconds. Is the shorter initial window intentional, for example to bound resource use from connections that never send anything, or should it use the same 20-second value as the steady state?

[mcfnord]

Note

📡 STAND BY FOR AN LLM-AUTHORED MESSAGE.

Ran the F1 (IPv6 directory) and B1 (IPv4 forced-fragmentation) rows of our test matrix against this branch, using a 2-namespace Linux netns harness (real built server + crafted protocol frames, not simulated). Two results, one confirming the design and one flagging a separate gap.

Confirmed: the TCP fallback transport claim holds for IPv6. Forced a directory list to 61 entries (well past the ~35-server fragmentation threshold), then blocked all IPv6 fragments on the client side (ip6tables -m frag -j DROP, a blanket-fragment-intolerant policy matching real hardened firewalls). Result: CLM_SERVER_LIST/CLM_RED_SERVER_LIST were fully lost over UDP as expected, CLM_TCP_SUPPORTED still arrived, and the TCP re-request delivered all 61 entries intact with zero corruption. Repeated the same test over IPv4 with fragments genuinely dropped (iptables -t raw -A PREROUTING -f -j DROP — worth noting a plain filter-table -f rule silently no-ops on IPv4 due to conntrack pre-defragmentation; the raw-table form is the one that actually drops fragments) — same clean result, 61/61 entries recovered via TCP.

Found: a directory entry for a genuinely IPv6-only-registered server comes back with IP 0.0.0.0, independent of TCP vs UDP. CreateCLServerListMes and CreateCLRedServerListMes (protocol.cpp:2154 and :2277) encode each entry's address with HostAddr.InetAddr.toIPv4Address() — a 4-byte, IPv4-only wire field. Reproduced by registering a server from a real (non-v4-mapped) IPv6 socket and querying the list from an IPv4 client: name, city, and port all arrived correctly, but the IP field was 0.0.0.0.

This means TCP fallback is a necessary but not sufficient condition for IPv6 directories — even with fully reliable delivery, the list format itself can't describe an IPv6-only server's address. I believe this is exactly the gap softins's own #3809 ("Add IPv6 support for Directories", explicitly marked still-incomplete) is meant to close, rather than something this PR needs to take on — flagging here mainly so docs/TCP.md's framing ("resolving fragmentation is a prerequisite to IPv6 support in directories") can be read as a prerequisite rather than the whole story, and so the two PRs' scope boundary is explicit for reviewers.

Full harness, scripts, and the F2/B3 analysis (code-verified, not requiring a separate live run) are logged locally; happy to share more detail on the netns setup if useful for anyone else's testing.

[softins]

So a freshly accepted TCP connection has only 5 seconds to send its first message (CLM_CLIENT_ID) before the server closes it, while an established connection tolerates 20 seconds of silence. I reproduced this directly: a bare TCP connect that sent no data was closed by the server at 5.32 seconds, logged as Jamulus-TCP: idle timeout - disconnecting.

This could disconnect a legitimate client on a slow or high-latency link, if the UDP CLIENT_ID exchange that triggers the TCP connection (or the TCP handshake itself) takes longer than 5 seconds. Is the shorter initial window intentional, for example to bound resource use from connections that never send anything, or should it use the same 20-second value as the steady state?

This is intentional. 5 seconds should be plenty for the initial timeout. But the 20 seconds is needed to allow the 15-second keepalives to keep a NAT or firewall session open on a long per-session connection.

[softins]

Found: a directory entry for a genuinely IPv6-only-registered server comes back with IP 0.0.0.0, independent of TCP vs UDP. CreateCLServerListMes and CreateCLRedServerListMes (protocol.cpp:2154 and :2277) encode each entry's address with HostAddr.InetAddr.toIPv4Address() — a 4-byte, IPv4-only wire field. Reproduced by registering a server from a real (non-v4-mapped) IPv6 socket and querying the list from an IPv4 client: name, city, and port all arrived correctly, but the IP field was 0.0.0.0.

This means TCP fallback is a necessary but not sufficient condition for IPv6 directories — even with fully reliable delivery, the list format itself can't describe an IPv6-only server's address. I believe this is exactly the gap softins's own #3809 ("Add IPv6 support for Directories", explicitly marked still-incomplete) is meant to close, rather than something this PR needs to take on — flagging here mainly so docs/TCP.md's framing ("resolving fragmentation is a prerequisite to IPv6 support in directories") can be read as a prerequisite rather than the whole story, and so the two PRs' scope boundary is explicit for reviewers.

Yes, as deduced by AI, IPv6 support is still incomplete with #3809 under development.

Also, TCP fallback is not needed for the server registering with a directory, as the messages are so small they will never be subject to fragmentation.