stoi + uint16_t cast wrapped -1/65536 to unexpected ports. Parse with
stol, require 1..65535, fail loadConfig with a clear error for listeners
and game backends.
parseListenLine left all flags false for typos (e.g. websockt), so
startup created a plaintext Telnet listener. Validate against the
supported set (telnet, telnet+tls, websocket, websocket+tls, grpc-web
[+tls]) and fail loadConfig. Also wire grpc-web+tls flags that the
listener tag path already expected.
#1886: reject orphan CONTINUATION and new TEXT/BINARY while reassembly is
pending (1002); validate TEXT UTF-8 on complete and assembled messages
(1007); clear fragOpcode when reassembly finishes. Mirrors engine #792.
#1887: gate every grpc-web RPC on ParseFromString success; return
INVALID_ARGUMENT (3) on truncated or malformed bodies instead of running
with default-valued requests.
proxy_regression covers fragmentation, UTF-8, and ParseFromString rejects.
charsetEncodeFromUtf8 advanced by the lead-byte width without checking
remaining length, so a truncated multi-byte sequence (trailing C2, E2 82,
F0 9F 92) walked past pe. co_dfa_ascii also reads without a length, so
incomplete/invalid leads fall back one byte with '?'.
Regression: convertInput and renderForClient on truncated sequences.
Follow-up on the review of this PR, applied here rather than left open.
stop() and the wait predicate were right and are unchanged. The stop path
pushed the item past the cap instead, on the reasoning that the future would
be "fulfilled if a later drain runs, broken by ~WorkQueue if not". Neither
happens. hydra_main declares
WorkQueue workQueue; // 338
std::unique_ptr<GrpcServer> grpcServer; // 339
so reverse destruction runs ~GrpcServer first, and server_->Shutdown()
blocks on in-flight RPCs before ~WorkQueue can break any promise. The
released producer pushes its item and the handler immediately calls the
unguarded future.get() that every handler calls, and parks there. The
thread moves from enqueue() to get() and is still an in-flight RPC, so
shutdown still hangs.
Modelled against this branch's own header:
enqueue returned : yes
RPC finished : NO -- still in flight
=> server_->Shutdown() would still wait here.
Complete the item instead, with a value-initialised Result rather than a
broken promise -- the constraint the original commit correctly identified,
since breaking it would throw from the unguarded .get() sites. All five
instantiated Result types are default-constructible and every handler
already reads the default as failure: bool -> false, std::string -> ""
(empty pid == auth failed), pair -> {"",""}, and
shared_ptr<OutputQueue> -> nullptr, which its three call sites already test
with `if (!oq) return NOT_FOUND`. A client mid-shutdown gets a clean
denial. Same model after the change: RPC finished, exit 0.
The regression test asserted only that enqueue() returns, which both
approaches satisfy -- it could not tell them apart. It now keeps the work
future and asserts it resolves with the default value, which is the
assertion that distinguishes completing the item from queueing it.
Verified: root build clean; smoke 1428/1428 on both routes. Note
mux/proxy/proxy_regression still does not link on macOS, before or after
(`-Wl,-z,relro,-z,now` is GNU ld syntax); the new case compiles here and
needs a Linux box to run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`enqueue()` blocks while the queue is full, and `cv_space_` is only ever
notified by `processPending()`. After the main loop's final drain nothing
calls it again, so a producer parked there has no way out -- and
~GrpcServer's Shutdown(), which waits for in-flight RPCs, then waits for
an RPC that can never finish.
Adds `stop()`: sets a flag, notifies all waiters, and is idempotent. The
wait predicate becomes `size() < MAX_PENDING || stopped_`. hydra_main
calls it after the last processPending() and before the gRPC server is
torn down.
A stopped enqueue pushes past the cap rather than failing the promise.
That keeps the caller's future on exactly the path it takes today --
fulfilled if a later drain runs, broken by ~WorkQueue if not -- and the
overshoot is bounded by the RPCs already in flight. Failing the promise
instead would throw from the 29 unguarded `.get()` call sites in
grpc_server.cpp, turning a shutdown hang into a shutdown crash.
Test: proxy_regression gains testWorkQueueStopReleasesBlockedProducer,
which fills to MAX_PENDING, asserts the next enqueue blocks, then asserts
stop() releases it. #1286 noted this mechanism had no behavioural
coverage; it does now.
Control -- reverting the predicate to `size() < MAX_PENDING`:
proxy_regression: stop() releases a producer blocked on a full queue
exit=1
Also fixes header dependency tracking in mux/proxy/Makefile. The
`%.o: %.cpp` rule compared each object against its .cpp only, so editing
work_queue.h relinked without recompiling. The control above first
appeared to PASS against the reverted header for exactly that reason --
the suite was testing a stale object. Adds -MMD -MP, `-include`s the
generated .d files, and cleans them.
Verified: touching work_queue.h now recompiles proxy_regression.o
(previously relink only).
proxy_regression ok; full make test green, smoke 1427/0 on both routes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#1265: Bound WorkQueue to MAX_PENDING=1024 (blocking enqueue) and wait on
futures in GameSession reader so a stream cannot flood the main loop.
#1266: Cap concurrent subscribers per session (MAX_SUBSCRIBERS=8); reject
GameSession/Subscribe/SubscribeGmcp/WS with RESOURCE_EXHAUSTED / close.
#1267: grpc-web SendInput routes through TelnetBridge::convertInput like
native gRPC and WS GameSession.
#1268: Reject gRPC/WS/grpc-web input lines above MAX_INPUT_LINE_LENGTH
(8192, shared with front-door telnet assembly); drop oversized GMCP.
#1269: GetGameStatus returns host PIDs only for admin accounts; any
authenticated session still sees running/up.
#1270: proxy_regression covers subscriber cap, line-limit constants,
work-queue cap constant, and convertInput non-UTF8 target.
The #1093 fix reset lenBytesRead before MaskKey on all three length
paths, but the added regression only covered the 16-bit case — where
the stale index (2) is still in-bounds of maskKey[4], so it catches the
framing desync but not the actual out-of-bounds write.
Add the 64-bit case (len=127), which is the memory-safety path: pre-fix,
lenBytesRead was 8 entering MaskKey and maskKey[8] was written past the
uint8_t[4] array. Verified: with the LenExt64 reset reverted, this test
fails and UBSan reports "index 8 out of bounds for type 'unsigned char
[4]'"; with the fix it decodes "hi" cleanly. proxy_regression green
plain and under -fsanitize=address,undefined.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- #1091: saveSession uses ON CONFLICT upsert so REPLACE cannot CASCADE-wipe scrollback
- #1092: loadFromDb keeps memoryBytes_ in lockstep with append() (no size_t underflow)
- #1093: reset lenBytesRead before MaskKey after extended length (no maskKey OOB)
- #1094: deliver WS CLOSE (and protocol errors) so session paths close the FD;
complete zero-length payloads after the mask key (empty CLOSE)
proxy_regression: ok (ext-len mask + CLOSE + unmasked teardown)
Split charsetPayload into separate request/accepted fields so both
survive in a single read. Suppress duplicate WILL responses when the
server echoes back DO after an unsolicited WILL on connect. Add
regression tests for split IAC EOR and combined charset subneg.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
