Fix accepted socket ownership and GnuTLS teardown - #298
Conversation
2e33a0b to
82cbf67
Compare
Transfer ownership of an accepted socket explicitly to relpTcp_t by clearing the local descriptor immediately after assignment. If TLS initialization then fails, relpTcpDestruct() is the sole owner responsible for closing the socket; the outer accept cleanup only closes descriptors whose ownership was never transferred. Add a deterministic regression test that forces a post-transfer TLS error, reuses the accepted descriptor after the first close, and verifies that error cleanup does not close the replacement. The test reports two closes on the old path and passed 100 consecutive runs with the fix.
Run GnuTLS session shutdown and deinitialization before closing the transport socket so gnutls_bye() can send close_notify. Retry GNUTLS_E_INTERRUPTED, but do not retry GNUTLS_E_AGAIN, preventing teardown from stalling on a nonblocking socket. OpenSSL retains its existing post-socket-close cleanup ordering. Release GnuTLS sessions, client and server credentials, and DH parameters even when initialization or the handshake ended before bTLSActive was set. Add a lifecycle regression test that verifies the transport remains open during gnutls_bye() and that partially initialized resources are all released.
82cbf67 to
396f43f
Compare
|
@codex review @cubic-dev-ai review |
@rgerhards I have started the AI code review. It will take a few minutes to complete. |
|
To use Codex here, create a Codex account and connect to github. |
![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: 396f43f4b1
ℹ️ 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".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
@codex review @cubic-dev-ai review |
@rgerhards I have started the AI code review. It will take a few minutes to complete. |
|
To use Codex here, create a Codex account and connect to github. |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Addressed the new review findings in 828d767: partial OpenSSL setup now frees its SSL object without attempting shutdown, the GnuTLS EAGAIN fixture fails promptly if retry behavior regresses, and the GNU ld --wrap fixtures are excluded on Solaris Studio where their runtime interposition is incompatible.\n\n@codex review\n\n@cubic-dev-ai review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 828d767d89
ℹ️ 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".
| if BUILD_STATIC | ||
| # These white-box fixtures use GNU ld --wrap interposition. Solaris Studio | ||
| # accepts the option but does not provide compatible runtime interposition. | ||
| if !OS_SOLARIS |
There was a problem hiding this comment.
On supported non-Solaris hosts using a non-GNU-compatible linker—most notably macOS and AIX, which are both handled in configure.ac—this guard still enables the fixtures, but their -Wl,--wrap=... flags rely on the GNU-style symbol wrapping documented by GNU ld. Native Apple and AIX linkers reject these options, causing make check to fail while linking the test programs. Detect --wrap support or restrict these fixtures to compatible linkers rather than excluding only Solaris.
Useful? React with 👍 / 👎.
|
Follow-up fix in a5c63b9: the OpenSSL client error path was freeing a BIO after SSL had taken ownership, leaving SSL with a dangling BIO. It now frees through SSL_free() exactly once; partial SSL teardown occurs before socket close, and the full local suite passes (32 pass, 1 expected Valgrind skip).\n\n@codex review\n\n@cubic-dev-ai review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5c63b9b77
ℹ️ 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".
| if(pThis->pEngine->tls_lib == 0 && pThis->bTLSActive) { | ||
| relpTcpDestructTLS_gtls(pThis); |
There was a problem hiding this comment.
When a GnuTLS accept fails fatally during the initial handshake, gnutls_init() has already populated pThis->session, but bTLSActive is still false because it is set only after the handshake succeeds or becomes retryable. The accept error path then calls relpTcpDestruct(), and this guard skips relpTcpDestructTLS_gtls(), permanently leaking the session for every malformed or failed inbound TLS connection; repeated unauthenticated connections can therefore exhaust server memory. The lifecycle test masks this case by explicitly setting bTLSActive = 1 on its supposedly partial object.
Useful? React with 👍 / 👎.
|
Solaris correction in 7a4b5b4: Automake negated-condition syntax did not suppress the GNU ld --wrap fixtures on Solaris Studio. The explicit OS_SOLARIS/else branch does; Linux local suite remains green.\n\n@codex review\n\n@cubic-dev-ai review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Final Solaris conditional fix in 5303c8a: configure now runs AC_CANONICAL_HOST before evaluating platform conditionals, so OS_SOLARIS is correctly set on the SPARC builder. Full local suite: 32 pass, 1 expected Valgrind skip.\n\n@codex review\n\n@cubic-dev-ai review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Solaris configure follow-up in a67e71d: AC_CANONICAL_HOST is incompatible with this legacy Solaris dependency-tracking setup. Platform selection now falls back to uname when host is unset, avoiding the bootstrap failure while setting OS_SOLARIS on SunOS. Full local suite still passes.\n\n@codex review\n\n@cubic-dev-ai review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a67e71d653
ℹ️ 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".
| case "${host}" in | ||
| *-*-linux*) | ||
| case "${host}:$(uname -s)" in | ||
| *-*-linux*:*|*:Linux) |
There was a problem hiding this comment.
When configuring on Linux with an explicit non-Linux target such as --host=sparc-sun-solaris2.11, the combined value still matches this first *:Linux alternative before reaching the Solaris arm. The configuration consequently defines OS_LINUX while omitting OS_SOLARIS, the Solaris feature flags, and SOL_LIBS; other cross-targets are similarly classified as the build OS. Test host first and consult uname only when it is unset.
Useful? React with 👍 / 👎.
|
Solaris gate correction in b8a4bfb: platform logic is restored unchanged; a new OS_SOLARIS_WRAP conditional uses uname solely to omit the GNU ld --wrap fixtures on SunOS. This avoids the Solaris configure regression. Full local suite passes.\n\n@codex review\n\n@cubic-dev-ai review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Commit 1: Prevent descriptor reuse close after failed accept
The accepted socket now transfers explicitly to
relpTcp_t: the localdescriptor is cleared immediately after assignment. If TLS initialization then
fails,
relpTcpDestruct()is the sole owner responsible for closing thesocket, while outer accept cleanup only closes descriptors whose ownership was
never transferred.
The accompanying deterministic regression test forces a post-transfer TLS
error, reuses the accepted descriptor after the first close, and verifies that
cleanup does not close the replacement. It reports two closes on the old path
and passed 100 consecutive runs with the fix.
Commit 2: Fix GnuTLS shutdown ordering and resource cleanup
GnuTLS session shutdown and deinitialization now happen before the transport
socket is closed, allowing
gnutls_bye()to sendclose_notify.GNUTLS_E_INTERRUPTEDis retried, whileGNUTLS_E_AGAINis not, preventingteardown from stalling on a nonblocking socket. OpenSSL retains its existing
post-socket-close cleanup ordering.
GnuTLS sessions, client and server credentials, and DH parameters are now
released even when initialization or the handshake ends before
bTLSActiveis set. The lifecycle regression test verifies that the transport remains open
during
gnutls_bye()and that partially initialized resources are released.Validation
make distpassedThis PR is separate from and does not modify OpenSSL BIO ownership PR #297.