Harden ACE removal networking and OpenSSL 3 compatibility#455
Conversation
|
@claude review |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 242 |
| Duplication | 2 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28e62f9ec1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| void stopSubmissions() | ||
| { | ||
| std::lock_guard<std::mutex> lock(m_mutex); | ||
| m_acceptingSubmissions = false; |
There was a problem hiding this comment.
On Windows, once stop() calls this, m_acceptingSubmissions stays false forever because IocpServer::start() never reinitializes m_operations. If WorldSocketMgr::StopNetwork()/StartNetwork() or RaServer::Stop()/Start() reuses the same server object, the listener and worker threads come back up but every postAccept() returns at tryBegin(), so the port accepts no clients. Please reset the gate/count when starting a fresh listener.
Useful? React with 👍 / 👎.
| shouldClose = ctx->channel->closeRequested && !ctx->channel->sendShutdown && | ||
| ctx->channel->out.empty(); |
There was a problem hiding this comment.
For IOCP sessions that mark themselves closed by closed() after onConnect()/onData() and rely on the transport contract to flush then close, this condition never becomes true unless they also called the closer callback. The previous IOCP checks used session->closed(), and Reactor/io_uring still do, so Windows can leave those sessions open with a recv reposted instead of delivering EOF after final bytes. Include ctx->session->closed() in the drain-to-close decision or translate it to closeRequested.
Useful? React with 👍 / 👎.
| return; | ||
| } | ||
|
|
||
| std::string const providerVersion = m_legacyProvider.Version(); |
There was a problem hiding this comment.
When OPENSSL_MODULES points at a directory where the default provider comes from a different OpenSSL major but the legacy provider still matches the runtime, this check accepts startup because it only parses m_legacyProvider.Version(). The daemon then runs with a mixed default provider even though normal EVP algorithms come from that provider; query m_defaultProvider.Version() as well and require both provider majors to match the runtime before setting m_initialized.
Useful? React with 👍 / 👎.
| unsigned long const runtimeVersionNumber = OpenSSL_version_num(); | ||
| unsigned const runtimeMajor = static_cast<unsigned>((runtimeVersionNumber >> 28) & 0x0f); | ||
| if (!parsedProviderMajor || providerMajor != runtimeMajor) |
There was a problem hiding this comment.
If a deployment resolves both the runtime library and provider modules from OpenSSL 4.x, providerMajor == runtimeMajor and startup succeeds even though the build-time CMake check only covered the discovered package and this change intends to run only with OpenSSL 3.x. Add an explicit runtimeMajor == 3 check before accepting the provider/runtime pair so a swapped runtime cannot bypass the <4.0.0 policy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2841ed130
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| option(DEBUG "Enable debug build (only on non IDEs)" OFF) | ||
| option(WITHOUT_GIT "Disable Git revision detection" OFF) | ||
| option(BUILD_AH_SERVICE "Build the out-of-process auction-house service" ON) | ||
| option(BUILD_TESTING "Build focused regression tests" OFF) |
There was a problem hiding this comment.
Because this new option defaults to OFF and neither the Windows nor Linux workflow configure commands pass -DBUILD_TESTING=ON or run ctest, the regression targets added in this change are skipped by normal PR CI. That leaves the IOCP/provider/lease coverage described by the patch unused unless a developer opts in locally; enable the option in CI or default it on when the required dependencies are present.
Useful? React with 👍 / 👎.
| { | ||
| if (std::shared_ptr<WorldSocket> socket = m_OwningSocket.lock()) | ||
| { | ||
| socket->DetachSessionAndWait(); |
There was a problem hiding this comment.
When a WorldSession is destroyed while its socket is still open (for example a duplicate/login-loading removal or shutdown cleanup), this detaches the session before CloseSocket() runs later in the destructor. During the intervening logout work, WorldSocket::ProcessIncoming() sees no session and accepts a new CMSG_AUTH_SESSION on the same connection instead of rejecting it as a repeated auth, after which the destructor closes/detaches that newly published session. Mark the socket closed or close it before unpublishing the session while still waiting for active leases before freeing the object.
Useful? React with 👍 / 👎.
2 participants
Summary
Follow-up hardening for the ACE removal in #448 and #452.
OpenSSL
>=3.0.0,<4.0.0.Networking
onConnect()in the io_uring backend.WorldSession lifetime
WorldSessionaccess from network callbacks with move-only session leases.WorldSessiondestruction.Regression coverage
Adds focused tests for:
Testing
Windows RelWithDebInfo:
dumpbinconfirmed both daemons depend only onlibcrypto-3-x64.dll.The Reactor and io_uring runtime cases are platform-gated and will run through the Linux CI matrix.
Dependency
This updates the
src/realmdsubmodule to the companion realmd fix:This change is