#### 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
#### Brief overview of PR changes/additions
- When a game re-sends `Client.GUI` while a GUI package download is
still running (typically a reconnect mid-download), the new download is
no longer aborted the instant its progress dialog replaces the old one
- `TMainConsole::showPackageDownloadProgress()` now disconnects the
superseded `QProgressDialog` before closing it, so its `close()` ->
`canceled()` no longer reaches `slot_cancelPackageDownload()`
- `cTelnet::downloadAndInstallGUIPackage()` now aborts an in-flight
predecessor reply *before* assigning the new one, so the old transfer
tears down through its own `finished()` path instead of leaking and
driving the replacement dialog
#### Motivation for adding to Mudlet
Follow-up hardening for #9507. `QProgressDialog::closeEvent()` emits
`canceled()`. Because the new `QNetworkReply` was assigned before
`signal_packageDownloadStarted` was emitted, closing the previous dialog
fired `slot_cancelPackageDownload()` against the just-created reply,
cancelling the fresh download at birth and leaving a frozen,
uncancellable dialog that never received progress or finished events.
The stale reply also kept driving the new dialog with interleaved
progress and wasted bandwidth.
#### Other info (issues closed, discussion etc)
Follow-up to #9507 (findings F1/F4/F9 from an adversarial review of that
PR). Behaviour is otherwise unchanged: a single download still shows,
updates, and cancels exactly as before. Functional suite 17/17 green
locally.
Assisted-by: Claude:claude-opus-4-8
**Test case:** Extends `TelnetTlsPromptTest` with
`test_replacingDownloadDialogKeepsNewDownloadAlive`: it starts a real
GUI download against a TCP server that accepts but never answers (so the
reply stays in flight), triggers a second download that supersedes it,
and asserts the new reply is still alive (not aborted) with exactly one
dialog surviving. Verified fail-without-fix: reverting the two
production changes makes the new assertion fail ("The superseding GUI
download left no active network reply."). Manual check: connect to a
game that serves a `Client.GUI` package, and while its download progress
dialog is up, force the server to re-send `Client.GUI` (e.g. reconnect)
- the download completes and installs instead of freezing.
#### Brief overview of PR changes/additions
Follow-up hardening for #9507. Three changes to the MSSP TLS-upgrade
prompt:
- The frontend's modal `QMessageBox` is now connected to
`signal_promptTlsAvailable` with `Qt::QueuedConnection`, so it runs with
no `cTelnet` socket-parsing frames beneath it.
- A new in-flight latch (`mTlsUpgradePromptInFlight`) stops a second
MSSP advertisement re-emitting the prompt while one is already pending
or open.
- `slot_tlsUpgradeResponse()` now discards a stale answer (with a
`qWarning`) when the connection is gone or already upgraded, instead of
acting on it.
#### Motivation for adding to Mudlet
An adversarial review of #9507 found three re-entrancy issues, all
reachable from a hostile server:
- **Use-after-free:** the prompt was emitted via a direct connection
from deep inside `cTelnet::processSocketData()`, and
`QMessageBox::exec()` spins a nested event loop under that frame. A Lua
`closeProfile()` (a 0 ms `singleShot`) firing in that loop destroys the
`Host` and its `cTelnet` member synchronously; unwinding then returns
into the destroyed object's signal machinery and `processSocketData()`'s
own member writes (e.g. `--mDecompressionRecursionDepth`).
- **Modal stacking:** nothing latched while the modal spun, so further
MSSP subnegotiations serviced by the nested loop re-emitted and stacked
unbounded dialogs.
- **Mid-parse mutation:** the answer's `disconnectIt()`/`connectIt()`
ran beneath the paused parse frame, so the outer loop kept parsing the
old buffer against mutated members and sent negotiation replies to the
new socket.
Delivering the prompt queued lets the parse pass unwind before the
dialog opens (fixing the UAF and the mid-parse mutation; the existing
`QPointer<Host>` capture then fully protects the lambda). The latch
fixes the stacking. Because the emit now returns immediately, the answer
can arrive later, so the slot guards against a connection that has since
dropped or upgraded.
#### Other info (issues closed, discussion etc)
Follow-up to #9507; no behavior change for the normal accept/decline
paths.
**Test case:**
`TelnetTlsPromptTest` updated and extended (all pass under `ctest`):
- `test_secondTlsAdvertisementWhilePendingDoesNotReprompt` - a repeated
MSSP TLS advertisement while a prompt is pending emits exactly once.
- `test_tlsUpgradeAnswerAfterDisconnectIsDiscarded` - an answer arriving
after the connection dropped is discarded with a warning and does not
crash or mutate the profile.
Assisted-by: Claude:claude-opus-4-8
#### Brief overview of PR changes/additions
- Removes all direct Qt Widgets usage from `ctelnet.{h,cpp}`: the
TLS-upgrade question, GUI-download progress dialog, and bell alert/beep
now go through Qt signals; the frontend (mudlet/TMainConsole) owns the
actual widgets
- Establishes the seam template for the libmudlet split: core emits
pre-translated payload -> frontend shows widget -> callback slot with a
state guard
- Adds `TelnetTlsPromptTest`: drives a real MSSP `TLS` subnegotiation
through a stub server and asserts the new signal fires with the right
payload
#### Motivation for adding to Mudlet
First concrete step of the re-scoped libmudlet plan (Widgets-free
`mudlet_core` for headless/testability/WASM); this PR is the pattern
every later extraction (Host, TMap, XMLexport) will copy, so it's a
draft for reviewing the template itself.
#### Other info (issues closed, discussion etc)
Part of #8681 / #9011. Behavior-preserving: the TLS dialog is still
modal and synchronous (same-thread direct connection), all strings stay
in cTelnet's tr() context so existing translations are unaffected. One
hardening: the TLS response slot now guards against the connection
dropping while the dialog is open. Functional suite 17/17, unit 27/28
(lone failure is the pre-existing TKeySequenceEditTest headless flake).
Assisted-by: Claude:claude-opus-4-8
**Test case:** Connect to a game; on a server advertising MSSP TLS (e.g.
one that sends `IAC SB MSSP` with a TLS port) the upgrade question
appears and both Yes/No behave as before; trigger a server BEL and check
the taskbar alert/beep; install a server-offered GUI package and check
the download progress dialog shows, updates, and its Cancel aborts the
download.
#### Demo (before & after)
Behavior-preserving refactor, so this proves parity of the user-visible
flow: a server advertising an MSSP `TLS` port raises the modal
secure-port question, and clicking **No** reconnects in open mode
without re-prompting - identical on `development` (before) and this
branch (after).
https://github.com/user-attachments/assets/77955117-fb95-400b-9e52-7bc08157478d
