JSON-RPC: bound the per-connection read buffer so unterminated input cannot exhaust memory

[mcfnord]

MY LLM WROTE:

The JSON-RPC server consumes input only on a complete line — while ( pSocket->canReadLine() ) in CRpcServer::OnNewConnection (src/rpcserver.cpp). Until a newline arrives the received bytes stay in the QTcpSocket read buffer, which is unbounded because setReadBufferSize() is never called. A client that connects and sends bytes with no \n grows that buffer 1:1 with bytes sent, before authentication, until the process is killed by the allocator (uncaught std::bad_alloc → SIGABRT, dropping every connected client).

Measured on a non-ASan release build of main, one connection sending 200 MiB with no newline:

build	outcome	server RSS delta
current main	connection held open, buffer grows	+204,920 kB (≈ +200 MiB, 1:1 with input)
this change	connection dropped after buffering ≤ 64 KiB	+112 kB (flat)

Well-formed traffic is unaffected on the patched build: jamulus/apiAuth returns "result":"ok" and jamulus/getVersion returns the version, connection stays open.

Scope: reachable only when the RPC server is enabled (--jsonrpcport) and bound off-loopback (--jsonrpcbindip); the default bind is 127.0.0.1. This is the denial-of-service class SECURITY.md documents as a non-guarantee, so the change hardens a documented limitation rather than closing a promised guarantee — the buffer bounds in a few lines.

The change, all in OnNewConnection:

• setReadBufferSize ( MAX_JSON_RPC_REQUEST_BYTES ) on each new connection, so buffering stops at the bound.
• after the read loop, a connection whose buffer is full with no complete line is answered with a parse error and closed.
• MAX_JSON_RPC_REQUEST_BYTES = 64 KiB — far above any real request (secret, method name, or a large batch), and adjustable.

A regression test fits the fork's JSON-RPC test surface directly: send an unterminated payload and assert the server's RSS stays flat and the connection is dropped, versus a terminated payload parsed normally.

@dtinth — flagging you as the JSON-RPC author and since this sits in the surface the fork is bringing under test.

[pljones]

@mcfnord, pass your LLM this PR and get it to work out why it failed in the build: and work out what to put in the appropriate guidance notes to stop it happening again.

[ann0see]

Service Unavailable

NO need to prompt the LLM. Very likely just an issue on GitHubs side

[mcfnord]

MY LLM WROTE:

The blocking check was the "Verify C-like coding style" job, conclusion cancelled. The job record shows zero steps executed, no stored log, and inverted timestamps (started_at 2026-08-07 06:38 UTC, completed_at 2026-08-06 16:24 UTC) — it never ran.

Re-running it directly needs repo write access, so the PR was closed and reopened to fire a fresh pull_request event on the same commit (c1e6175, nothing pushed). The style check now passes, and the full matrix has re-run green: all five platform builds, all three style checks, and the JSON-RPC docs check. So this was a GitHub-side cancellation, as @ann0see said — nothing in the PR needed fixing.

[dtinth]

TIL: Closing and reopening a pull request can retrigger CI. Good to know.

There was a GitHub Actions outage. Details: https://www.githubstatus.com/incidents/qcvjkzcs7j74

Seems like the LLM agent hasn't yet been trained to look at the CI log, and then deduce that they should reach for githubstatus.com.

[mcfnord]

🤖 AI: The 64K was not derived from anything, so here is the number to derive it from.

Two inputs compete for "largest legitimate request", and both land on MAX_LEN_CHAT_TEXT (1600).

Welcome message: every path goes through CServer::SetWelcomeMessage, which truncates to 1600 at src/server.cpp:1656. That covers the GUI box (tedWelcomeMessage is a QTextEdit, no limit of its own), --welcomemessage as text, --welcomemessage as a file (file.readAll(), src/server.cpp:228) and jamulusserver/setWelcomeMessage. The unbounded file and shell paths do not widen the ceiling.

Chat: chatdlg.cpp:111 caps typed text at 1600, and the receive path caps at MAX_LEN_CHAT_TEXT_PLUS_HTML (1800) at protocol.cpp:1424.

Measured against a server built from this branch, request line as sent on the wire:

request	bytes
setWelcomeMessage, 1600 ASCII chars	1,707
setWelcomeMessage, 5000 ASCII chars (stored: 1600)	5,107
broadcastChatMessage, 1600 ASCII chars	1,707
1600 chars with every character JSON-escaped as \uXXXX	9,708

So the largest legitimate request is 9,708 bytes and 64 KiB is 6.8x that.

Worth flagging while looking: of the string-taking methods, only jamulusserver/privateChatMessage enforces a length (serverrpc.cpp:125). Sending 1601 characters to it is rejected, while broadcastChatMessage, setWelcomeMessage, setServerName and setRecordingDirectory all return ok and rely on a downstream truncation. That is out of scope here, and I can open a separate issue if useful.

Proposed for the declaration, saying what it is plus where the number comes from:

// Largest JSON-RPC request accepted on one line: 6.8x the largest legitimate
// request, which is a 1600-character welcome or chat message (MAX_LEN_CHAT_TEXT),
// 9708 bytes once JSON-escaped.
static constexpr int MAX_JSON_RPC_REQUEST_BYTES = 64 * 1024;

@ann0see, on stating the limit in the error: the reply becomes "Parse error: Request exceeds maximum size of 65536 bytes", built from the constant so the two cannot drift.

If 6.8x reads as too much slack, 16 KiB still leaves 1.6x and is a one-line change.

[ann0see]

Yes. Then bound it to a lower number.

[mcfnord]

🤖 AI: Lowered to 16 KiB, and the error now names the limit. Lowering it surfaced a second thing, which the 64 KiB version had too.

Sizing first: the largest legitimate single request is a 1600-character welcome or chat message with every character JSON-escaped, 9698 bytes as a compact line. 16384 bytes leaves 1.7x that, or a batch of 221 ordinary calls.

The second thing: setWelcomeMessage has accepted an unbounded string since JSON-RPC landed in 2021, and closing the connection on an oversized request also destroys the caller's authenticated session, because isAuthenticated is keyed by the socket. Authenticate, send an oversized setWelcomeMessage, then getClients on the same connection:

request	mainline	branch, closing	branch, discarding
8098 B	ok, follow-up answers	ok, follow-up answers	ok, follow-up answers
20098 B	ok, follow-up answers	error, follow-up ECONNRESET	error, follow-up answers
100098 B	ok, follow-up answers	error, follow-up ECONNRESET	error, follow-up answers

So an oversized request is now answered and then discarded up to its terminating newline, instead of closing the connection. A long-standing script that posts a large welcome message gets an error on that one call and keeps working, rather than silently losing its session. The bound still holds: 200 MiB of unterminated input is absorbed with under 1 MB of RSS growth. The cost is that a flooding connection is no longer severed, so it can keep streaming, at flat memory.

[pljones]

I think I'd like a second limit check. Once the (potentially JSON-escaped as \uXXXX) message has been read and converted into a QString, check the length does not exceed MAX_LEN_CHAT_TEXT_PLUS_HTML. That can also trigger an error back before updating the Server.

Then, in a separate PR -- assuming these aren't in place, which reading the above it sounds like they aren't:

• change --welcomemessage
  • for a file, check the file size against MAX_LEN_CHAT_TEXT_PLUS_HTML before reading it, then check the content of the string read
  • for a string, check the length against MAX_LEN_CHAT_TEXT_PLUS_HTML
  • if either check fails, issue a fatal error (so this is command line argument processing)

[mcfnord]

🤖 AI: The second check is in, on the decoded QString before the server is updated. It catches what the byte bound structurally cannot: 1700 characters sent as \uXXXX escapes is 10222 request bytes, well under the 16 KiB limit, so it passed that check and was stored truncated. It is now rejected on length, and the previously stored message is left alone.

One thing to settle first, because it decides the constant. A bound is already there, one layer down: SetWelcomeMessage truncates to MAX_LEN_CHAT_TEXT, 1600, and says nothing. That path is untouched by this PR; reading the stored value back over CLM_REQ_WELCOME_MESSAGE, against a build from before the check went in:

sent	RPC result	stored
1500	ok	1500
1600	ok	1600
1700	ok	1600
1800	ok	1600
3000	ok	1600

Rows three to five are what the new check now rejects outright.

So a check at MAX_LEN_CHAT_TEXT_PLUS_HTML (1800) would admit 1601-1800 and let them be cut anyway, silently, one call later. 1600 also looks deliberate rather than incidental: the delivered message is prefixed with WELCOME_MESSAGE_PREFIX, 31 characters, and 1600 + 31 = 1631 fits the 1800 the chat-text parser accepts; 1800 + 31 would not. Unless you read it otherwise, the check goes in at MAX_LEN_CHAT_TEXT.

On the separate PR: the string form of --welcomemessage inherits that same silent truncation. The file form does not check size — the whole file is read, then truncated. An 8 MiB file starts the server normally and stores 1600 characters. The cost is transient, not resident — peak RSS above baseline tracks file size at 2.9x for an 8 MiB file and 3.0x for 32 MiB, the read buffer plus its UTF-16 copy, while steady-state RSS is unchanged. So the size check bounds a startup spike rather than a leak, and a fatal error is still the better behaviour than truncating a file someone meant to serve.

[pljones]

MAX_LEN_CHAT_TEXT

Yes, that makes sense.

[ann0see]

Please fix the comment length - otherwise I'd say it's ok