#### Brief overview of PR changes/additions
- The `MAIN_LOOP_END` block that reacts to a received GA sits **inside**
the per-byte loop of `processSocketData()`. The normal path clears
`recvdGA` before handing the line to `gotPrompt()`; the `mFORCE_GA_OFF`
path only appended a newline and left the flag set.
- So once a GA arrived, every remaining byte of that read re-entered the
block and appended another newline: the rest of the read was emitted one
character per line, and each of those lines ran the full trigger set.
- Clear `recvdGA` in that branch too.
#### Motivation for adding to Mudlet
With this option enabled, a single GA turned the remainder of the read
into a vertical column of single characters and made the client crawl,
since every character was processed as its own line.
#### Other info (issues closed, discussion etc)
Only reachable with "Force telnet GA signal interpretation off" enabled
(Settings -> Special Options). That flag is copied into `cTelnet` at
connect time (`ctelnet.cpp:497`), so it applies from the next connection
- which is also why the group box is labelled as needing a restart.
The option exists so Mudlet ignores GA signalling from older game
drivers, and ignoring the signal is precisely what this branch is for;
it simply never consumed the flag.
**Test case:** with a server that sends text, then `IAC GA`, then more
text **in a single write** so they arrive in one read, and with the
option enabled before connecting: before this change everything after
the GA appears one character per line; after it, the trailing text
renders on one line as expected. `cTelnetBufferTest`,
`TelnetSgrDefaultColorTest`, `TelnetStringSequenceRecoveryTest`,
`GMCPCharLoginTest` and `TriggerSameLineMatchTest` all pass.
---------
Signed-off-by: Jay Howard <jay.patrick.howard@gmail.com>
#### Brief overview of PR changes/additions
- Converts the hand-balanced re-entrancy depth counters to
`qScopeGuard`, matching the pattern already used by `TTimer`, `TAction`,
`TScript` and `Host`: `cTelnet::mDecompressionRecursionDepth` (four
exits across a ~290-line function), `cTelnet::mLoopbackProcessingDepth`,
and `mProcessingDepth` in `AliasUnit`, `TriggerUnit` and `KeyUnit`.
- Strictly behaviour-preserving. Each guard fires exactly where the
manual decrement did, `Q_ASSERT` and the depth-0 drain (`doCleanup()`,
`mRootNodesAddedWhileProcessing.clear()`) included, and the over-limit
cap still trips on the same value and reports the same message. The
recursion cap moves from a file-local constant to
`cTelnet::scmMaxDecompressionRecursion` next to its sibling so a test
can pin the threshold.
- Adds `AliasUnit::processingDepth()` and `KeyUnit::processingDepth()`
(mirroring `TriggerUnit` and `ActionUnit`), a new
`UnitProcessingDepthTest` and a new slot in `cTelnetBufferTest` that
drive each converted exit and assert the count comes back - including an
item that deletes itself mid-pass, so the drain step is covered too.
#### Motivation for adding to Mudlet
The counters are members, so a level leaked by a future early `return`
is permanent for that object rather than for that call. Eight leaks in
`cTelnet::mDecompressionRecursionDepth` and the connection refuses all
further data for the rest of the session, endlessly printing "Too much
data to process at once, some may have been lost" - a sticky, crash-free
hang that no test or sanitizer would catch.
`KeyUnit::processDataStream()` had the same hazard in miniature: it
returned from inside its match loop, so a second copy of the
decrement-assert-drain block had to be kept in step by hand (and it ran
`doCleanup()` while the range-`for` over the list it deletes from was
still in scope). All of these are balanced correctly today; this makes
it impossible for them not to be.
#### Other info (issues closed, discussion etc)
Follows up a review note on PR #9677 ("fix: telnet NUL terminator heap
overflow"). No behaviour change, so nothing to demo.
**Test case:** full `ctest` 72/73 and busted twice (2158 successes / 1
failure), the two failures being `TKeySequenceEditTest` and `UI_spec`
`getMainWindowSize`, both reproduced with `src/` reverted to development
so neither is from this change; sabotage check - restoring the
hand-balanced form with the over-limit decrement omitted, the `KeyUnit`
match-exit decrement omitted, and the `AliasUnit` drain omitted turned
`cTelnetBufferTest` and `UnitProcessingDepthTest` red on exactly those
three points ("a recursion level was leaked", "the drain did not run"),
and restoring the guards turned them green.
Assisted-by: Claude:claude-opus-5
#### Brief overview of PR changes/additions
* `cTelnet::processSocketData()` terminated its input at
`in_buffer[amount + 1]`, one byte past the data it was given, and did so
*before* checking the `-1`/`0` returns from `QIODevice::read()`. It now
guards first, then terminates at `in_buffer[amount]`.
* The guard is `amount <= 0` rather than `== -1`, because
`loopbackTest()` narrows a `qsizetype` into an `int` and can produce a
negative that is not `-1`.
* Adds `cTelnetBufferTest` (5 slots) and drops the `reserve(size + 16)`
slack that three existing telnet tests carried purely to absorb the
stray write, which turns them into regression guards too.
#### Motivation for adding to Mudlet
The socket path survived this because `slot_socketReadyToBeRead()`
over-allocates its stack buffer, but the same function is reached from
Lua's `feedTelnet()` via `loopbackTest()`, which passes a `QByteArray`
sized exactly to its contents - so the stray NUL landed one byte past a
heap allocation. That is a real out-of-bounds write reachable from any
script, and the workarounds already sitting in our test suite show it
has been quietly worked around rather than fixed.
#### Other info (issues closed, discussion etc)
Closes#1065. Supersedes the closed#8438, which carried the same fix
under 19 commits of unrelated history and had CI red on a faulty
assertion in its own test.
**Test case:** reverting the `src/ctelnet.cpp` hunk makes 3 of the 5 new
slots fail and AddressSanitizer report `heap-buffer-overflow ... in
cTelnet::processSocketData(char*, int, bool)`; restoring it gives 7
passed / 0 failed, and the full suite is 72/72 serially.
One thing to flag for review: this adds `friend class
cTelnetBufferTest;` to `cTelnet`, since `processSocketData()` is private
and the public `loopbackTest()` cannot express a caller-laid-out buffer.
It sits beside the existing `friend class TelnetTlsPromptTest;`, so
there is precedent, but it is a test name in a shipped header and worth
a second opinion.
Three review findings were deliberately left out of scope. The MCCP
decompression path hit the same overflow, via the re-entry that passes
`remainingData`/`remainingAmount` back into `processSocketData()` - the
fix covers it, but nothing under `test/` exercises compression at all,
so it is fixed-but-unguarded and a dedicated MCCP test belongs in its
own PR. The other two are pre-existing and orthogonal: a read error
(`amount == -1`) is still silent, because `slot_socketError()` has been
commented out as unused since 2017; and `mDecompressionRecursionDepth`
is hand-balanced across four decrements rather than held by a scope
guard (verified balanced today, but fragile). Happy to do any of them as
a follow-up.
Assisted-by: Claude:claude-opus-5
