Repository navigation
fix(ssl): release connections when their context terminates - #8541
Conversation
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
This PR does not change docs/dev/debt/libSceSsl.md line 5, which still describes the old
With this PR, Of this, only the |
|
@nikowolf123 Thanks for catching this. I removed both obsolete ownership clauses from This update only changes documentation. The conventions check passes with zero errors and one warning about the existing entry length; the whitespace check also passes. |
|
Independent Windows run of this PR (data only). Environment: Windows 10 Pro 10.0.19045, Ryzen 7 5800X3D, WinLibs GCC 15.2.0 r7 (MinGW-w64 UCRT), Release, BUILD_TESTING=ON. Merge-tree of the PR head against Results (3 runs each):
Not run: other platforms, the other tests in the same family, the full repository CTest, and any title or console run. AI-assisted (Claude Code). |
321e6de to
d450e0c
Compare
d450e0c to
f2173bb
Compare
|
I moved the HTTPS integration test into the new |
harisibrar1
left a comment
There was a problem hiding this comment.
Checked against main 522e32e and the libSceSsl Export.cpp / libkernel Udp.cpp sources on main:
- It still merges cleanly onto current main. Main's
sceSslTermstill only erases the context, andFindConnectionrejects the orphan afterwards, so the leak this fixes is still on main. - I traced the ownership:
Sessionholdsshared_ptr<Context>, andEstablishcopies the session under the registry lock, so an operation in progress keeps both alive after Term; the handshake-failure path looks the connection up again by id; and nothing destroyed understate.mutextakes that mutex again. I found no use-after-free, double-free or deadlock. - Optional nit:
sceSslDeleteConnectiondeclaressessionbefore thelock_guard, so the lastSessionreference is released after unlock. In the new Term,erase_ifdrops idle sessions inside the lock, includingsocketOwner, which can be the last reference to the libkernelSocket(its destructor closes the host socket) if the title already closed the descriptor. Moving the erased sessions into a vector declared before the lock would match DeleteConnection. From reading the code, I don't think it can deadlock; it's only extra work under the global SSL mutex.
Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.
|
@harisibrar1 Thanks for tracing the ownership and destructor paths. I am keeping the current The lock-hold trade-off is real: destroying an idle session can also release the last native socket owner while AI-assisted: yes. |
What
Terminating an SSL context removes the context but retains its connection records. Later connection lookup rejects their missing parent, so
sceSslDeleteConnectionreturns0x8095F006and cannot release the orphaned registry entries. Three Init/Create/Term/Delete cycles leave three records on the tested main.Remove the terminating context's connections under the existing registry mutex before releasing the context. Existing shared session ownership lets operations already in progress finish and keeps their memory pool alive until the last operation releases it. Calls made after termination keep returning the existing invalid-ID result. Record removal uses
std::erase_ifwithout allocating a temporary container. Idle sessions, including a last native socket owner, can be destroyed while the registry mutex is held.Remove the two obsolete ownership clauses from
docs/dev/debt/libSceSsl.mdwhile preserving the console-measured invalid-ID returns and the remaining open gaps.Add a lifecycle regression that compiles the actual SSL implementation into its test translation unit to observe private ownership. It checks both cleanup orders, multiple contexts, repeated concurrent create/delete/term operations and a paused verification callback with initialized TLS backend state. Related to #8005, finding C10.
Tested
Rebase validation on Windows 11 Pro 10.0.26300, WinLibs MinGW-w64 GCC 15.2.0r7 UCRT, Release/O3,
BUILD_TESTING=ON, full runtime configuration. CPU: AMD Ryzen 9 7900X; RAM: 32 GB; GPU: NVIDIA GeForce RTX 4070 Ti SUPER.Validated base:
a6dcea9279b53b3a39c13695eac66f7c8b5b503c. Rebased head:f2173bba34562299f9d90583120ade2205ae12ef. Both original commits are retained. All production build targets and selected fixtures pass without an overlay. All 20 selected CTests pass without skips in 8.42 s, including all six SSL tests, eleven transfer regressions,guest_sce_net,guest_tcpandpatched_libraries.The rebase preserves current-main SSL transport and certificate behavior. The production change, lifecycle regression and CMake registration are unchanged from the previous PR head. The documentation conflict preserves all current-main entries and removes exactly the two obsolete ownership clauses.
Validation commands (
buildis the configured full Windows build directory):Conventions has zero errors and one warning about the existing 7,600-character debt entry. Main-sync and whitespace checks pass. The overlap tool lists this PR itself because its hosted head differs from the rebased local branch. Manual review of uncompared shared files confirms that #6039 edits a separate debt entry, #6044 moves existing test paths, #8951 declares a separate export, and #9173 changes separate asymmetric-key cleanup exports. I also checked related #8658 and #9407; none implements context-termination cleanup or adds a functional prerequisite.
Original regression evidence is pinned to base
d073fe6a1da82441d33f4d3da72cc5f7d3496259and fix15afb86473371f57bb2c6fa0d80bd3e4d7d049b8:The same regression failed on that unchanged base with
termination retained orphan connection records. The original five baseline SSL tests and six fixed SSL tests passed three executions each. One initial fixed run failed the existing net transport handshake assertion with a 200 ms receive timeout; an isolated rerun, those repeated checks and six further diagnostic handshakes passed. The timeout and assertion were not changed, and the exact transient cause was not reproduced. These are historical results for the pinned revisions above.The lifecycle regression checks 32 concurrent create/delete/term cycles, separate context ownership and an in-flight verification callback retaining its session and pool until completion. The callback case initializes TLS backend state but performs no wire I/O; the transport tests exercise loopback TLS through the kernel and libSceNet socket tables.
No complete CTest suite, GPU execution or title/gameplay run is claimed. This fixes host registry ownership; it does not establish a PRAGMATA OOM or crash cause.
Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)