Repository navigation
Do not delete the authentication method local reference twice - #1002
Closed
fredericgermain wants to merge 1 commit into
Closed
fredericgermain wants to merge 1 commit into
fredericgermain wants to merge 1 commit into
Conversation
Motivation: netty/netty#17356, which only bumps to 2.0.82.Final, dies in every openssl-dynamic job with FATAL ERROR in native method: Bad global or local ref passed to JNI at io.netty.internal.tcnative.SSL.readFromSSL(Native Method) at io.netty.handler.ssl.ReferenceCountedOpenSslEngine.readPlaintextData netty#996 added an early NETTY_JNI_UTIL_DELETE_LOCAL of authMethodString right after the verifier callback returns, but that macro does not NULL its argument and both cert verify functions already delete the same reference again at their complete: label. Deleting a local reference twice is fatal under -Xcheck:jni, which netty's test JVMs run with, and it is undefined behaviour without it. Every other delete site in this file already clears the variable after deleting it, get_certs included. These two were the only ones that did not. This is only visible on the openssl build because SSL_cert_verify has no task path: it always reaches both deletes. The BoringSSL and AWS-LC twin, tcn_SSL_cert_custom_verify, has the same defect but netty defaults io.netty.handler.ssl.openssl.useTasks to true, so the branch that carries it is not taken and the boringssl jobs stay green. It is fixed here too rather than left waiting for someone to set useTasks to false. Modification: NULL authMethodString after the early delete in SSL_cert_verify and in tcn_SSL_cert_custom_verify, so the complete: block skips it. Result: Reproduced first on macOS aarch64 against OpenSSL 3.6.3, driving a pair of netty SSLEngines through a handshake under -Xcheck:jni: unpatched, the JVM dies on the first handshake with the stack above; patched, 50 handshakes complete and negotiate TLS_AES_128_GCM_SHA256 over TLSv1.3. Confirmed separately with a standalone JNI test that a second DeleteLocalRef on the same reference produces exactly this error on its first occurrence, and that clearing the variable removes it.
Member
|
This was already fixed by #1001 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation:
#996 added an early
NETTY_JNI_UTIL_DELETE_LOCALofauthMethodStringin both cert verify callbacks. The macro does not NULL its argument, and both functions already delete that reference again at theircomplete:label. A doubleDeleteLocalRefis fatal under-Xcheck:jni, so every openssl-dynamic job in netty/netty#17356 dies with:Only openssl is affected because
SSL_cert_verifyhas no task path and always reaches both deletes.tcn_SSL_cert_custom_verifyhas the same defect, but netty defaultsuseTasksto true so that branch is skipped, which is why the boringssl jobs stay green. It is fixed here too rather than left waiting for someone to setuseTasksto false.Modification:
NULL
authMethodStringafter the early delete in both functions, matching whatget_certsand every other delete site in this file already do.Result:
Fixes netty/netty#17356. Verified on macOS aarch64 against OpenSSL 3.6.3, driving a pair of netty SSLEngines through a handshake under
-Xcheck:jni: unpatched the JVM dies on the first handshake, patched 50 handshakes complete over TLSv1.3.https://claude.ai/code/session_01RVaAsJndUk7hxtG38JTxTc