Assign credential ids inside newX509/newDelegated so no existing native is renamed

Author: Alex Choulos

boringssl-static/src/test/java/io/netty/internal/tcnative/SSLCredentialIdTest.java

@@ -22,7 +22,6 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotEquals;
-import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 public class SSLCredentialIdTest {
@@ -58,29 +57,6 @@ public void newCredentialsHavePositiveDistinctIds() throws Exception {
         }
     }
 
-    @Test
-    public void idCannotBeReassigned() throws Exception {
-        long cred = SSLCredential.newX509();
-        try {
-            long id = SSLCredential.getId(cred);
-            assertThrows(IllegalStateException.class, () -> SSLCredential.setId0(cred, id + 1));
-            assertEquals(id, SSLCredential.getId(cred));
-        } finally {
-            SSLCredential.free(cred);
-        }
-    }
-
-    @Test
-    public void nonPositiveIdIsRejected() throws Exception {
-        long cred = SSLCredential.newX509();
-        try {
-            assertThrows(IllegalArgumentException.class, () -> SSLCredential.setId0(cred, 0));
-            assertThrows(IllegalArgumentException.class, () -> SSLCredential.setId0(cred, -1));
-        } finally {
-            SSLCredential.free(cred);
-        }
-    }
-
     @Test
     public void freshSslHasNoSelectedCredentialId() throws Exception {
         long ctx = SSLContext.make(SSL.SSL_PROTOCOL_TLSV1_2, SSL.SSL_MODE_SERVER);

openssl-classes/src/main/java/io/netty/internal/tcnative/SSLCredential.java

@@ -15,8 +15,6 @@
  */
 package io.netty.internal.tcnative;
 
-import java.util.concurrent.atomic.AtomicLong;
-
 /**
  * SSL_CREDENTIAL management for BoringSSL.
  * 
@@ -34,8 +32,6 @@
  */
 public final class SSLCredential {
 
-    private static final AtomicLong NEXT_ID = new AtomicLong();
-
     private SSLCredential() { }
 
     /**
@@ -48,32 +44,7 @@ private SSLCredential() { }
      * @return the SSL_CREDENTIAL instance (SSL_CREDENTIAL *)
      * @throws Exception if an error occurred
      */
-    public static long newX509() throws Exception {
-        return assignId(newX509Native());
-    }
-
-    private static native long newX509Native() throws Exception;
-
-    private static long assignId(long cred) throws Exception {
-        try {
-            setId0(cred, NEXT_ID.incrementAndGet());
-        } catch (Throwable t) {
-            // Don't leak the credential if tagging fails.
-            free(cred);
-            throw t;
-        }
-        return cred;
-    }
-
-    /**
-     * Assign the id of an SSL_CREDENTIAL. A credential can only be assigned an id once.
-     *
-     * @param cred the SSL_CREDENTIAL instance (SSL_CREDENTIAL *)
-     * @param id the id, which must be positive
-     * @throws IllegalArgumentException if {@code id} is not positive
-     * @throws IllegalStateException if the credential already has an id
-     */
-    static native void setId0(long cred, long id);
+    public static native long newX509() throws Exception;
 
     /**
      * Get the id that was assigned to an SSL_CREDENTIAL when it was created by {@link #newX509()} or
@@ -224,11 +195,7 @@ private static long assignId(long cred) throws Exception {
      * @return the delegated SSL_CREDENTIAL instance (SSL_CREDENTIAL *)
      * @throws Exception if an error occurred
      */
-    public static long newDelegated() throws Exception {
-        return assignId(newDelegatedNative());
-    }
-
-    private static native long newDelegatedNative() throws Exception;
+    public static native long newDelegated() throws Exception;
 
     /**
      * Set the delegated credential for an SSL_CREDENTIAL.

openssl-dynamic/src/main/c/sslcredential.c

@@ -24,6 +24,7 @@
 
 
 #include "tcn.h"
+#include "apr_atomic.h"
 #include "ssl_private.h"
 #include "sslcredential.h"
 
@@ -39,26 +40,38 @@ static void throw_openssl_error(JNIEnv* env, const char* msg) {
 }
 
 static int tcn_SSL_CREDENTIAL_id_idx = -1;
-
-typedef char tcn_SSL_CREDENTIAL_id_requires_64bit_pointers[sizeof(void*) >= sizeof(jlong) ? 1 : -1];
+static volatile apr_uint32_t tcn_SSL_CREDENTIAL_next_id = 0;
 
 // The id is a scalar packed into the ex_data slot, so no free callback is needed.
 jlong tcn_SSL_CREDENTIAL_get_id(const SSL_CREDENTIAL* cred) {
     if (cred == NULL || tcn_SSL_CREDENTIAL_id_idx < 0) {
         return 0;
     }
-    return (jlong)(intptr_t) SSL_CREDENTIAL_get_ex_data(cred, tcn_SSL_CREDENTIAL_id_idx);
+    return (jlong)(uintptr_t) SSL_CREDENTIAL_get_ex_data(cred, tcn_SSL_CREDENTIAL_id_idx);
+}
+
+static SSL_CREDENTIAL* assign_id(JNIEnv* e, SSL_CREDENTIAL* cred) {
+    apr_uint32_t id;
+    do {
+        id = apr_atomic_inc32(&tcn_SSL_CREDENTIAL_next_id) + 1;
+    } while (id == 0);
+    if (!SSL_CREDENTIAL_set_ex_data(cred, tcn_SSL_CREDENTIAL_id_idx, (void*)(uintptr_t) id)) {
+        SSL_CREDENTIAL_free(cred);
+        throw_openssl_error(e, "Failed to set SSL_CREDENTIAL id");
+        return NULL;
+    }
+    return cred;
 }
 #endif
 
 
 
 // Core SSL_CREDENTIAL functions
-TCN_IMPLEMENT_CALL(jlong, SSLCredential, newX509Native)(TCN_STDARGS) {
+TCN_IMPLEMENT_CALL(jlong, SSLCredential, newX509)(TCN_STDARGS) {
 #ifdef OPENSSL_IS_BORINGSSL
     SSL_CREDENTIAL* cred = SSL_CREDENTIAL_new_x509();
     TCN_CHECK_NULL(cred, credential, 0);
-    return (jlong)(intptr_t)cred;
+    return (jlong)(intptr_t)assign_id(e, cred);
 #else
     tcn_ThrowUnsupportedOperationException(e, "SSL_CREDENTIAL API not available.");
     return 0;
@@ -86,29 +99,6 @@ TCN_IMPLEMENT_CALL(void, SSLCredential, free)(TCN_STDARGS, jlong cred) {
 #endif
 }
 
-TCN_IMPLEMENT_CALL(void, SSLCredential, setId0)(TCN_STDARGS, jlong cred, jlong id) {
-#ifdef OPENSSL_IS_BORINGSSL
-    SSL_CREDENTIAL* c = (SSL_CREDENTIAL*)(intptr_t)cred;
-    TCN_CHECK_NULL(c, credential, /* void */);
-    if (id <= 0) {
-        tcn_ThrowIllegalArgumentException(e, "credential id must be positive");
-        return;
-    }
-    if (tcn_SSL_CREDENTIAL_get_id(c) != 0) {
-        jclass ise = (*e)->FindClass(e, "java/lang/IllegalStateException");
-        if (ise != NULL) {
-            (*e)->ThrowNew(e, ise, "credential already has an id");
-        }
-        return;
-    }
-    if (!SSL_CREDENTIAL_set_ex_data(c, tcn_SSL_CREDENTIAL_id_idx, (void*)(intptr_t) id)) {
-        throw_openssl_error(e, "Failed to set SSL_CREDENTIAL id");
-    }
-#else
-    tcn_ThrowUnsupportedOperationException(e, "SSL_CREDENTIAL API not available.");
-#endif
-}
-
 TCN_IMPLEMENT_CALL(jlong, SSLCredential, getId)(TCN_STDARGS, jlong cred) {
 #ifdef OPENSSL_IS_BORINGSSL
     SSL_CREDENTIAL* c = (SSL_CREDENTIAL*)(intptr_t)cred;
@@ -340,14 +330,14 @@ TCN_IMPLEMENT_CALL(void, SSLCredential, setTrustAnchorId)(TCN_STDARGS, jlong cre
 }
 
 // Delegated credentials
-TCN_IMPLEMENT_CALL(jlong, SSLCredential, newDelegatedNative)(TCN_STDARGS) {
+TCN_IMPLEMENT_CALL(jlong, SSLCredential, newDelegated)(TCN_STDARGS) {
 #ifdef OPENSSL_IS_BORINGSSL
     SSL_CREDENTIAL* credential = SSL_CREDENTIAL_new_delegated();
     if (credential == NULL) {
         throw_openssl_error(e, "Failed to create delegated SSL_CREDENTIAL");
         return 0;
     }
-    return (jlong)(intptr_t)credential;
+    return (jlong)(intptr_t)assign_id(e, credential);
 #else
     tcn_ThrowUnsupportedOperationException(e, "SSL_CREDENTIAL API not available.");
     return 0;
@@ -389,10 +379,9 @@ TCN_IMPLEMENT_CALL(void, SSLCredential, setDelegatedCredential)(TCN_STDARGS, jlo
 // JNI Method Registration Table Begin
 static const JNINativeMethod method_table[] = {
     // Core functions
-    { TCN_METHOD_TABLE_ENTRY(newX509Native, ()J, SSLCredential) },
+    { TCN_METHOD_TABLE_ENTRY(newX509, ()J, SSLCredential) },
     { TCN_METHOD_TABLE_ENTRY(upRef, (J)V, SSLCredential) },
     { TCN_METHOD_TABLE_ENTRY(free, (J)V, SSLCredential) },
-    { TCN_METHOD_TABLE_ENTRY(setId0, (JJ)V, SSLCredential) },
     { TCN_METHOD_TABLE_ENTRY(getId, (J)J, SSLCredential) },
 
     // Configuration
@@ -408,7 +397,7 @@ static const JNINativeMethod method_table[] = {
     { TCN_METHOD_TABLE_ENTRY(setTrustAnchorId, (J[B)V, SSLCredential) },
 
     // Delegated credentials
-    { TCN_METHOD_TABLE_ENTRY(newDelegatedNative, ()J, SSLCredential) },
+    { TCN_METHOD_TABLE_ENTRY(newDelegated, ()J, SSLCredential) },
     { TCN_METHOD_TABLE_ENTRY(setDelegatedCredential, (J[B)V, SSLCredential) }
 };