Skip to content

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

Open
mcfnord wants to merge 1 commit into
jamulussoftware:mainfrom
mcfnord:rpc-readbuf-bound
Open

JSON-RPC: bound the per-connection read buffer so unterminated input cannot exhaust memory#3861
mcfnord wants to merge 1 commit into
jamulussoftware:mainfrom
mcfnord:rpc-readbuf-bound

Conversation

@mcfnord

@mcfnord mcfnord commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

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.

An unauthenticated client that sends no newline grows the QTcpSocket read
buffer without bound (setReadBufferSize is never called), before auth, until
std::bad_alloc aborts the process and drops all clients. Bound the buffer to
64 KiB per connection and drop a connection whose buffer fills with no
complete line.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@pljones

pljones commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

@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

ann0see commented Aug 6, 2026

Copy link
Copy Markdown
Member

Service Unavailable

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

@mcfnord mcfnord closed this Aug 8, 2026
@mcfnord mcfnord reopened this Aug 8, 2026
@mcfnord

mcfnord commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

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

dtinth commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

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.

Comment thread src/rpcserver.cpp
return object;
}

// Maximum size of a single JSON-RPC request line. An unauthenticated client that

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shorter comment, please. This declaration only needs a comment saying what it is, not where it's used.

The reasoning is repeated later anyway.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you'd like to save tokens, use the Add a suggestion button, which might shortcut to Ctrl+g. The AI will still learn your preference.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The size should definitely allow about a welcome message/chat message + some slack. That's how I would define the maximum.

@dingodoppelt dingodoppelt added the AI AI generated or potentially AI generated label Aug 8, 2026
Comment thread src/rpcserver.cpp
// reject and close rather than hold the bytes indefinitely.
if ( !pSocket->canReadLine() && pSocket->bytesAvailable() >= MAX_JSON_RPC_REQUEST_BYTES )
{
Send ( pSocket, QJsonDocument ( CreateJsonRpcErrorReply ( iErrParseError, "Parse error: Request exceeds maximum size" ) ) );

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maximum size of which length -> return it.

Comment thread src/rpcserver.cpp
// (canReadLine()), so without a bound the received bytes accumulate in the socket read
// buffer without limit until the process is killed by the allocator. Requests larger
// than this, or unterminated data that fills the buffer, are rejected instead of held.
static constexpr int MAX_JSON_RPC_REQUEST_BYTES = 64 * 1024;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This number needs explaining - where does the 64K bound actually come from?

Is there a bound in the Server GUI Welcome Message text box? This shouldn't exceed that, for example.

I don't know if there's a bound on the command line welcome message file size. The command line bounds as plain text would be bounded by the shell -- I've probably cut-n-paste over 64K into bash before now.

So those other sources should be checked, too.

@mcfnord

mcfnord commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

🤖 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI AI generated or potentially AI generated

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants