This is an automated email from the ASF dual-hosted git repository.

remm pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/main by this push:
     new 1f56910  Partially revert 590877b
1f56910 is described below

commit 1f56910cf16b895857d3313f79ddd149774f5dc8
Author: remm <r...@apache.org>
AuthorDate: Tue Nov 23 21:39:43 2021 +0100

    Partially revert 590877b
    
    This causes a regression crash with the TestSSLHostConfigCompat test
    [which I had stopped running since the previous Tomcat release]. I don't
    understand the difference between the two (an implicit scope is a shared
    scope with a cleaner), but no reason to leave this in.
---
 .../util/net/openssl/panama/OpenSSLContext.java    | 143 +++++++++++----------
 1 file changed, 76 insertions(+), 67 deletions(-)

diff --git 
a/modules/openssl-java17/src/main/java/org/apache/tomcat/util/net/openssl/panama/OpenSSLContext.java
 
b/modules/openssl-java17/src/main/java/org/apache/tomcat/util/net/openssl/panama/OpenSSLContext.java
index 15b075a..69f1f71 100644
--- 
a/modules/openssl-java17/src/main/java/org/apache/tomcat/util/net/openssl/panama/OpenSSLContext.java
+++ 
b/modules/openssl-java17/src/main/java/org/apache/tomcat/util/net/openssl/panama/OpenSSLContext.java
@@ -30,6 +30,8 @@ import java.io.File;
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodHandles;
 import java.lang.invoke.MethodType;
+import java.lang.ref.Cleaner;
+import java.lang.ref.Cleaner.Cleanable;
 import java.nio.charset.StandardCharsets;
 import java.security.PrivateKey;
 import java.security.SecureRandom;
@@ -147,6 +149,8 @@ public class OpenSSLContext implements 
org.apache.tomcat.util.net.SSLContext {
         }
     }
 
+    private static final Cleaner cleaner = Cleaner.create();
+
     private final SSLHostConfig sslHostConfig;
     private final SSLHostConfigCertificate certificate;
     private final boolean alpn;
@@ -167,7 +171,7 @@ public class OpenSSLContext implements 
org.apache.tomcat.util.net.SSLContext {
     }
 
     private final ContextState state;
-    private final ResourceScope scope;
+    private final Cleanable cleanable;
 
     private static String[] getCiphers(MemoryAddress sslCtx) {
         MemoryAddress sk = SSL_CTX_get_ciphers(sslCtx);
@@ -198,7 +202,7 @@ public class OpenSSLContext implements 
org.apache.tomcat.util.net.SSLContext {
 
         this.sslHostConfig = certificate.getSSLHostConfig();
         this.certificate = certificate;
-        scope = ResourceScope.newImplicitScope();
+        ResourceScope scope = ResourceScope.newSharedScope();
 
         MemoryAddress sslCtx = MemoryAddress.NULL;
         MemoryAddress confCtx = MemoryAddress.NULL;
@@ -335,7 +339,7 @@ public class OpenSSLContext implements 
org.apache.tomcat.util.net.SSLContext {
         } catch(Exception e) {
             throw new SSLException(sm.getString("openssl.errorSSLCtxInit"), e);
         } finally {
-            state = new ContextState(sslCtx, confCtx, 
negotiableProtocolsBytes);
+            state = new ContextState(scope, sslCtx, confCtx, 
negotiableProtocolsBytes);
             /*
              * When an SSLHostConfig is replaced at runtime, it is not 
possible to
              * call destroy() on the associated OpenSSLContext since it is 
likely
@@ -344,11 +348,11 @@ public class OpenSSLContext implements 
org.apache.tomcat.util.net.SSLContext {
              * OpenSSLSessionContext) to ensure that the OpenSSLContext remains
              * ineligible for GC while those connections are alive. Once those
              * connections complete, the OpenSSLContext will become eligible 
for GC
-             * and the implicit scope will ensure that the associated native
-             * resources are cleaned up.
+             * and this method will ensure that the associated native 
resources are
+             * cleaned up.
              */
             states.put(Long.valueOf(sslCtx.address().toRawLongValue()), state);
-            scope.addCloseAction(state);
+            cleanable = cleaner.register(this, state);
 
             if (!success) {
                 destroy();
@@ -370,6 +374,7 @@ public class OpenSSLContext implements 
org.apache.tomcat.util.net.SSLContext {
     @Override
     public synchronized void destroy() {
         states.remove(Long.valueOf(state.sslCtx.address().toRawLongValue()));
+        cleanable.clean();
     }
 
 
@@ -556,7 +561,7 @@ public class OpenSSLContext implements 
org.apache.tomcat.util.net.SSLContext {
             }
 
             // List the ciphers that the client is permitted to negotiate
-            if (SSL_CTX_set_cipher_list(state.sslCtx, 
CLinker.toCString(sslHostConfig.getCiphers(), scope)) <= 0) {
+            if (SSL_CTX_set_cipher_list(state.sslCtx, 
CLinker.toCString(sslHostConfig.getCiphers(), state.scope)) <= 0) {
                 log.warn(sm.getString("engine.failedCipherSuite", 
sslHostConfig.getCiphers()));
             }
 
@@ -592,67 +597,65 @@ public class OpenSSLContext implements 
org.apache.tomcat.util.net.SSLContext {
             // Set int verify_callback(int preverify_ok, X509_STORE_CTX 
*x509_ctx) callback
             MemoryAddress openSSLCallbackVerify =
                     
CLinker.getInstance().upcallStub(openSSLCallbackVerifyHandle,
-                    openSSLCallbackVerifyFunctionDescriptor, scope);
+                    openSSLCallbackVerifyFunctionDescriptor, state.scope);
             // Leave this just in case but in Tomcat this is always set again 
by the engine
             SSL_CTX_set_verify(state.sslCtx, value, openSSLCallbackVerify);
 
             // Trust and certificate verification
-            try (var scope = ResourceScope.newConfinedScope()) {
-                var allocator = SegmentAllocator.ofScope(scope);
-                if (tms != null) {
-                    // Client certificate verification based on custom trust 
managers
-                    state.x509TrustManager = chooseTrustManager(tms);
-                    MemoryAddress openSSLCallbackCertVerify =
-                            
CLinker.getInstance().upcallStub(openSSLCallbackCertVerifyHandle,
-                            openSSLCallbackCertVerifyFunctionDescriptor, 
this.scope);
-                    SSL_CTX_set_cert_verify_callback(state.sslCtx, 
openSSLCallbackCertVerify, state.sslCtx);
-
-                    // Pass along the DER encoded certificates of the accepted 
client
-                    // certificate issuers, so that their subjects can be 
presented
-                    // by the server during the handshake to allow the client 
choosing
-                    // an acceptable certificate
-                    for (X509Certificate caCert : 
state.x509TrustManager.getAcceptedIssuers()) {
-                        //SSLContext.addClientCACertificateRaw(state.ctx, 
caCert.getEncoded());
-                        var rawCACertificate = 
allocator.allocateArray(CLinker.C_CHAR, caCert.getEncoded());
-                        var rawCACertificatePointer = 
allocator.allocate(CLinker.C_POINTER, rawCACertificate);
-                        var x509CACert = d2i_X509(MemoryAddress.NULL, 
rawCACertificatePointer, rawCACertificate.byteSize());
-                        if (MemoryAddress.NULL.equals(x509CACert)) {
-                            logLastError(allocator, 
"openssl.errorLoadingCertificate");
-                        } else if (SSL_CTX_add_client_CA(state.sslCtx, 
x509CACert) <= 0) {
-                            logLastError(allocator, 
"openssl.errorAddingCertificate");
-                        } else if (log.isDebugEnabled()) {
-                            
log.debug(sm.getString("openssl.addedClientCaCert", caCert.toString()));
-                        }
+            var allocator = SegmentAllocator.ofScope(state.scope);
+            if (tms != null) {
+                // Client certificate verification based on custom trust 
managers
+                state.x509TrustManager = chooseTrustManager(tms);
+                MemoryAddress openSSLCallbackCertVerify =
+                        
CLinker.getInstance().upcallStub(openSSLCallbackCertVerifyHandle,
+                                openSSLCallbackCertVerifyFunctionDescriptor, 
state.scope);
+                SSL_CTX_set_cert_verify_callback(state.sslCtx, 
openSSLCallbackCertVerify, state.sslCtx);
+
+                // Pass along the DER encoded certificates of the accepted 
client
+                // certificate issuers, so that their subjects can be presented
+                // by the server during the handshake to allow the client 
choosing
+                // an acceptable certificate
+                for (X509Certificate caCert : 
state.x509TrustManager.getAcceptedIssuers()) {
+                    //SSLContext.addClientCACertificateRaw(state.ctx, 
caCert.getEncoded());
+                    var rawCACertificate = 
allocator.allocateArray(CLinker.C_CHAR, caCert.getEncoded());
+                    var rawCACertificatePointer = 
allocator.allocate(CLinker.C_POINTER, rawCACertificate);
+                    var x509CACert = d2i_X509(MemoryAddress.NULL, 
rawCACertificatePointer, rawCACertificate.byteSize());
+                    if (MemoryAddress.NULL.equals(x509CACert)) {
+                        logLastError(allocator, 
"openssl.errorLoadingCertificate");
+                    } else if (SSL_CTX_add_client_CA(state.sslCtx, x509CACert) 
<= 0) {
+                        logLastError(allocator, 
"openssl.errorAddingCertificate");
+                    } else if (log.isDebugEnabled()) {
+                        log.debug(sm.getString("openssl.addedClientCaCert", 
caCert.toString()));
                     }
-                } else if (sslHostConfig.getCaCertificateFile() != null || 
sslHostConfig.getCaCertificatePath() != null) {
-                    // Client certificate verification based on trusted CA 
files and dirs
-                    //SSLContext.setCACertificate(state.ctx,
-                    //        
SSLHostConfig.adjustRelativePath(sslHostConfig.getCaCertificateFile()),
-                    //        
SSLHostConfig.adjustRelativePath(sslHostConfig.getCaCertificatePath()));
-                    MemorySegment caCertificateFileNative = 
sslHostConfig.getCaCertificateFile() != null
-                            ? 
CLinker.toCString(SSLHostConfig.adjustRelativePath(sslHostConfig.getCaCertificateFile()),
 scope) : null;
-                    MemorySegment caCertificatePathNative = 
sslHostConfig.getCaCertificatePath() != null
-                            ? 
CLinker.toCString(SSLHostConfig.adjustRelativePath(sslHostConfig.getCaCertificatePath()),
 scope) : null;
-                    if (SSL_CTX_load_verify_locations(state.sslCtx,
-                            caCertificateFileNative == null ? 
MemoryAddress.NULL : caCertificateFileNative,
-                            caCertificatePathNative == null ? 
MemoryAddress.NULL : caCertificatePathNative) <= 0) {
-                        logLastError(allocator, 
"openssl.errorConfiguringLocations");
-                    } else {
-                        var caCerts = SSL_CTX_get_client_CA_list(state.sslCtx);
-                        if (MemoryAddress.NULL.equals(caCerts)) {
-                            caCerts = 
SSL_load_client_CA_file(caCertificateFileNative);
-                            if (!MemoryAddress.NULL.equals(caCerts)) {
-                                SSL_CTX_set_client_CA_list(state.sslCtx, 
caCerts);
-                            }
-                        } else {
-                            if (SSL_add_file_cert_subjects_to_stack(caCerts, 
caCertificateFileNative) <= 0) {
-                                caCerts = MemoryAddress.NULL;
-                            }
+                }
+            } else if (sslHostConfig.getCaCertificateFile() != null || 
sslHostConfig.getCaCertificatePath() != null) {
+                // Client certificate verification based on trusted CA files 
and dirs
+                //SSLContext.setCACertificate(state.ctx,
+                //        
SSLHostConfig.adjustRelativePath(sslHostConfig.getCaCertificateFile()),
+                //        
SSLHostConfig.adjustRelativePath(sslHostConfig.getCaCertificatePath()));
+                MemorySegment caCertificateFileNative = 
sslHostConfig.getCaCertificateFile() != null
+                        ? 
CLinker.toCString(SSLHostConfig.adjustRelativePath(sslHostConfig.getCaCertificateFile()),
 state.scope) : null;
+                MemorySegment caCertificatePathNative = 
sslHostConfig.getCaCertificatePath() != null
+                        ? 
CLinker.toCString(SSLHostConfig.adjustRelativePath(sslHostConfig.getCaCertificatePath()),
 state.scope) : null;
+                if (SSL_CTX_load_verify_locations(state.sslCtx,
+                        caCertificateFileNative == null ? MemoryAddress.NULL : 
caCertificateFileNative,
+                                caCertificatePathNative == null ? 
MemoryAddress.NULL : caCertificatePathNative) <= 0) {
+                    logLastError(allocator, 
"openssl.errorConfiguringLocations");
+                } else {
+                    var caCerts = SSL_CTX_get_client_CA_list(state.sslCtx);
+                    if (MemoryAddress.NULL.equals(caCerts)) {
+                        caCerts = 
SSL_load_client_CA_file(caCertificateFileNative);
+                        if (!MemoryAddress.NULL.equals(caCerts)) {
+                            SSL_CTX_set_client_CA_list(state.sslCtx, caCerts);
                         }
-                        if (MemoryAddress.NULL.equals(caCerts)) {
-                            log.warn(sm.getString("openssl.noCACerts"));
+                    } else {
+                        if (SSL_add_file_cert_subjects_to_stack(caCerts, 
caCertificateFileNative) <= 0) {
+                            caCerts = MemoryAddress.NULL;
                         }
                     }
+                    if (MemoryAddress.NULL.equals(caCerts)) {
+                        log.warn(sm.getString("openssl.noCACerts"));
+                    }
                 }
             }
 
@@ -661,7 +664,7 @@ public class OpenSSLContext implements 
org.apache.tomcat.util.net.SSLContext {
                 //        MemoryAddress in, int inlen, MemoryAddress arg
                 MemoryAddress openSSLCallbackAlpnSelectProto =
                         
CLinker.getInstance().upcallStub(openSSLCallbackAlpnSelectProtoHandle,
-                        openSSLCallbackAlpnSelectProtoFunctionDescriptor, 
scope);
+                        openSSLCallbackAlpnSelectProtoFunctionDescriptor, 
state.scope);
                 SSL_CTX_set_alpn_select_cb(state.sslCtx, 
openSSLCallbackAlpnSelectProto, state.sslCtx);
                 // Skip NPN (annoying and likely not useful anymore)
                 //SSLContext.setNpnProtos(state.ctx, protocolsArray, 
SSL.SSL_SELECTOR_FAILURE_NO_ADVERTISE);
@@ -1125,7 +1128,7 @@ public class OpenSSLContext implements 
org.apache.tomcat.util.net.SSLContext {
                 }
                 // Set callback for DH parameters
                 MemoryAddress openSSLCallbackTmpDH = 
CLinker.getInstance().upcallStub(openSSLCallbackTmpDHHandle,
-                        openSSLCallbackTmpDHFunctionDescriptor, scope);
+                        openSSLCallbackTmpDHFunctionDescriptor, state.scope);
                 SSL_CTX_set_tmp_dh_callback(state.sslCtx, 
openSSLCallbackTmpDH);
                 // Set certificate chain file
                 if (certificate.getCertificateChainFile() != null) {
@@ -1213,7 +1216,7 @@ public class OpenSSLContext implements 
org.apache.tomcat.util.net.SSLContext {
                 }
                 // Set callback for DH parameters
                 MemoryAddress openSSLCallbackTmpDH = 
CLinker.getInstance().upcallStub(openSSLCallbackTmpDHHandle,
-                        openSSLCallbackTmpDHFunctionDescriptor, scope);
+                        openSSLCallbackTmpDHFunctionDescriptor, state.scope);
                 SSL_CTX_set_tmp_dh_callback(state.sslCtx, 
openSSLCallbackTmpDH);
                 for (int i = 1; i < chain.length; i++) {
                     //SSLContext.addChainCertificateRaw(state.ctx, 
chain[i].getEncoded());
@@ -1359,13 +1362,15 @@ public class OpenSSLContext implements 
org.apache.tomcat.util.net.SSLContext {
 
     private static class ContextState implements Runnable {
 
+        final ResourceScope scope;
         private final MemoryAddress sslCtx;
         private final MemoryAddress confCtx;
         private final List<byte[]> negotiableProtocols;
 
         private X509TrustManager x509TrustManager = null;
 
-        private ContextState(MemoryAddress sslCtx, MemoryAddress confCtx, 
List<byte[]> negotiableProtocols) {
+        private ContextState(ResourceScope scope, MemoryAddress sslCtx, 
MemoryAddress confCtx, List<byte[]> negotiableProtocols) {
+            this.scope = scope;
             this.sslCtx = sslCtx;
             this.confCtx = confCtx;
             this.negotiableProtocols = negotiableProtocols;
@@ -1373,9 +1378,13 @@ public class OpenSSLContext implements 
org.apache.tomcat.util.net.SSLContext {
 
         @Override
         public void run() {
-            SSL_CTX_free(sslCtx);
-            if (!MemoryAddress.NULL.equals(confCtx)) {
-                SSL_CONF_CTX_free(confCtx);
+            try {
+                SSL_CTX_free(sslCtx);
+                if (!MemoryAddress.NULL.equals(confCtx)) {
+                    SSL_CONF_CTX_free(confCtx);
+                }
+            } finally {
+                scope.close();
             }
         }
     }

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Reply via email to