Avoid holding a GlobalRef to ALPN selector

.travis.yml

@@ -1,5 +1,4 @@
-language: android
-dist: trusty
+language: minimal
 
 env:
   global:
@@ -18,16 +17,19 @@ matrix:
     ### Linux build is the only platform that builds Android here.
     ###
     - os: linux
-      jdk: openjdk8
+      dist: xenial
 
       env:
         - ANDROID_TOOLS_URL="https://dl.google.com/android/repository/sdk-tools-linux-4333796.zip"
         - ANDROID_HOME="$HOME/android-sdk-linux"
         - ANDROID_NDK_HOME="$ANDROID_HOME/ndk-bundle"
+        - JAVA_HOME=/usr/lib/jvm/java-8-openjdk-amd64
         - JAVA7_HOME=/usr/lib/jvm/java-7-openjdk-amd64
         - JAVA11_HOME=/usr/lib/jvm/java-11-openjdk-amd64
         - CC=clang-5.0
         - CXX=clang++-5.0
+        - PATH="$JAVA_HOME/bin:$PATH"
+        - TERM=dumb # to stop verbose build output
 
       before_install:
         - curl -L $ANDROID_TOOLS_URL -o $HOME/tools.zip
@@ -43,11 +45,13 @@ matrix:
         - $ANDROID_HOME/tools/bin/sdkmanager 'extras;android;m2repository' | tr '\r' '\n' | uniq
         - $ANDROID_HOME/tools/bin/sdkmanager ndk-bundle | tr '\r' '\n' | uniq
         - $ANDROID_HOME/tools/bin/sdkmanager 'cmake;3.6.4111459' | tr '\r' '\n' | uniq
+        - gimme 1.13 # Needed for BoringSSL build
+        - source ~/.gimme/envs/go1.13.env
 
       addons:
         apt:
           sources:
-            - llvm-toolchain-trusty-5.0
+            - llvm-toolchain-xenial-5.0
             - openjdk-r-java
             - ubuntu-toolchain-r-test
           packages:
@@ -57,8 +61,10 @@ matrix:
             - gcc-multilib
             - libc6-dev-i386
             - libc6-dev:i386
+            - linux-libc-dev
             - ninja-build
             - openjdk-7-jre # for running tests with Java 7
+            - openjdk-8-jdk # for building
             - openjdk-11-jre # for running tests with Java 11
 
     ###
@@ -69,6 +75,7 @@ matrix:
       env:
         - CC=clang
         - CXX=clang++
+        - TERM=dumb # to stop verbose build output
 
       before_install:
         - brew update

common/src/jni/main/cpp/conscrypt/native_crypto.cc

@@ -6115,12 +6115,8 @@ static ssl_verify_result_t cert_verify_callback(SSL* ssl, CONSCRYPT_UNUSED uint8
 
     JNI_TRACE("ssl=%p cert_verify_callback calling verifyCertificateChain authMethod=%s", ssl,
               authMethod);
-    jstring authMethodString = env->NewStringUTF(authMethod);
-    env->CallVoidMethod(sslHandshakeCallbacks, methodID, array.get(), authMethodString);
-
-    // We need to delete the local references so we not leak memory as this method is called
-    // via callback.
-    env->DeleteLocalRef(authMethodString);
+    ScopedLocalRef<jstring> authMethodString(env, env->NewStringUTF(authMethod));
+    env->CallVoidMethod(sslHandshakeCallbacks, methodID, array.get(), authMethodString.get());
 
     ssl_verify_result_t result = env->ExceptionCheck() ? ssl_verify_invalid : ssl_verify_ok;
     JNI_TRACE("ssl=%p cert_verify_callback => %d", ssl, result);
@@ -7544,7 +7540,7 @@ static int selectApplicationProtocol(SSL* ssl, unsigned char** out, unsigned cha
 /**
  * Calls out to an application-provided selector to choose the ALPN protocol.
  */
-static int selectApplicationProtocol(SSL* ssl, JNIEnv* env, jobject selector,
+static int selectApplicationProtocol(SSL* ssl, JNIEnv* env, jobject sslHandshakeCallbacks,
                                      unsigned char** out,
                                      unsigned char* outLen, const unsigned char* in,
                                      const unsigned int inLen) {
@@ -7558,9 +7554,9 @@ static int selectApplicationProtocol(SSL* ssl, JNIEnv* env, jobject selector,
         reinterpret_cast<const jbyte*>(in));
 
     // Invoke the selection method.
-    jclass cls = env->GetObjectClass(selector);
+    jclass cls = env->GetObjectClass(sslHandshakeCallbacks);
     jmethodID methodID = env->GetMethodID(cls, "selectApplicationProtocol", "([B)I");
-    jint offset = env->CallIntMethod(selector, methodID, protocols.get());
+    jint offset = env->CallIntMethod(sslHandshakeCallbacks, methodID, protocols.get());
 
     if (offset < 0) {
         JNI_TRACE("ssl=%p selectApplicationProtocol selection failed", ssl);
@@ -7595,7 +7591,7 @@ static int alpn_select_callback(SSL* ssl, const unsigned char** out, unsigned ch
 
     if (in == nullptr ||
         (appData->applicationProtocolsData == nullptr
-         && appData->applicationProtocolSelector == nullptr)) {
+         && !appData->hasApplicationProtocolSelector)) {
         if (out != nullptr && outLen != nullptr) {
             *out = nullptr;
             *outLen = 0;
@@ -7604,8 +7600,8 @@ static int alpn_select_callback(SSL* ssl, const unsigned char** out, unsigned ch
         return SSL_TLSEXT_ERR_NOACK;
     }
 
-    if (appData->applicationProtocolSelector != nullptr) {
-        return selectApplicationProtocol(ssl, env, appData->applicationProtocolSelector,
+    if (appData->hasApplicationProtocolSelector) {
+        return selectApplicationProtocol(ssl, env, appData->sslHandshakeCallbacks,
                                   const_cast<unsigned char**>(out), outLen, in, inLen);
     }
 
@@ -7683,23 +7679,23 @@ static void NativeCrypto_setApplicationProtocols(JNIEnv* env, jclass, jlong ssl_
     }
 }
 
-static void NativeCrypto_setApplicationProtocolSelector(JNIEnv* env, jclass, jlong ssl_address, CONSCRYPT_UNUSED jobject ssl_holder,
-                                                 jobject selector) {
+static void NativeCrypto_setHasApplicationProtocolSelector(JNIEnv* env, jclass, jlong ssl_address, CONSCRYPT_UNUSED jobject ssl_holder,
+                                                 jboolean hasSelector) {
     CHECK_ERROR_QUEUE_ON_RETURN;
     SSL* ssl = to_SSL(env, ssl_address, true);
-    JNI_TRACE("ssl=%p NativeCrypto_setApplicationProtocolSelector selector=%p", ssl, selector);
+    JNI_TRACE("ssl=%p NativeCrypto_setHasApplicationProtocolSelector selector=%d", ssl, hasSelector);
     if (ssl == nullptr) {
         return;
     }
     AppData* appData = toAppData(ssl);
     if (appData == nullptr) {
         conscrypt::jniutil::throwSSLExceptionStr(env, "Unable to retrieve application data");
-        JNI_TRACE("ssl=%p NativeCrypto_setApplicationProtocolSelector appData => 0", ssl);
+        JNI_TRACE("ssl=%p NativeCrypto_setHasApplicationProtocolSelector appData => 0", ssl);
         return;
     }
 
-    appData->setApplicationProtocolSelector(env, selector);
-    if (selector != nullptr) {
+    appData->hasApplicationProtocolSelector = hasSelector;
+    if (hasSelector) {
         SSL_CTX_set_alpn_select_cb(SSL_get_SSL_CTX(ssl), alpn_select_callback, nullptr);
     }
 }
@@ -9948,8 +9944,6 @@ static jlong NativeCrypto_SSL_get1_session(JNIEnv* env, jclass, jlong ssl_addres
 #define FILE_DESCRIPTOR "Ljava/io/FileDescriptor;"
 #define SSL_CALLBACKS \
     "L" TO_STRING(JNI_JARJAR_PREFIX) "org/conscrypt/NativeCrypto$SSLHandshakeCallbacks;"
-#define ALPN_PROTOCOL_SELECTOR \
-    "L" TO_STRING(JNI_JARJAR_PREFIX) "org/conscrypt/ApplicationProtocolSelectorAdapter;"
 #define REF_EC_GROUP "L" TO_STRING(JNI_JARJAR_PREFIX) "org/conscrypt/NativeRef$EC_GROUP;"
 #define REF_EC_POINT "L" TO_STRING(JNI_JARJAR_PREFIX) "org/conscrypt/NativeRef$EC_POINT;"
 #define REF_EVP_CIPHER_CTX \
@@ -10227,7 +10221,7 @@ static JNINativeMethod sNativeCryptoMethods[] = {
         CONSCRYPT_NATIVE_METHOD(d2i_SSL_SESSION, "([B)J"),
         CONSCRYPT_NATIVE_METHOD(getApplicationProtocol, "(J" REF_SSL ")[B"),
         CONSCRYPT_NATIVE_METHOD(setApplicationProtocols, "(J" REF_SSL "Z[B)V"),
-        CONSCRYPT_NATIVE_METHOD(setApplicationProtocolSelector, "(J" REF_SSL ALPN_PROTOCOL_SELECTOR ")V"),
+        CONSCRYPT_NATIVE_METHOD(setHasApplicationProtocolSelector, "(J" REF_SSL "Z)V"),
         CONSCRYPT_NATIVE_METHOD(SSL_CIPHER_get_kx_name, "(J)Ljava/lang/String;"),
         CONSCRYPT_NATIVE_METHOD(get_cipher_names, "(Ljava/lang/String;)[Ljava/lang/String;"),
         CONSCRYPT_NATIVE_METHOD(get_ocsp_single_extension, "([BLjava/lang/String;J" REF_X509 "J" REF_X509 ")[B"),

common/src/jni/main/include/conscrypt/app_data.h

@@ -116,7 +116,7 @@ class AppData {
     jobject sslHandshakeCallbacks;
     char* applicationProtocolsData;
     size_t applicationProtocolsLength;
-    jobject applicationProtocolSelector;
+    bool hasApplicationProtocolSelector;
 
     /**
      * Creates the application data context for the SSL*.
@@ -158,7 +158,6 @@ class AppData {
         }
 #endif
         clearApplicationProtocols();
-        clearApplicationProtocolSelector(env);
         clearCallbackState();
     }
 
@@ -191,19 +190,6 @@ class AppData {
         return true;
     }
 
-    /**
-     * Only called in server mode. Sets the application-provided ALPN protocol selector.
-     * This overrides the list of ALPN protocols, if set.
-     */
-    void setApplicationProtocolSelector(JNIEnv* e, jobject selector) {
-        clearApplicationProtocolSelector(e);
-        if (selector != nullptr) {
-            // Need to a global reference since we keep this around beyond a single JNI
-            // invocation.
-            applicationProtocolSelector = e->NewGlobalRef(selector);
-        }
-    }
-
     /**
      * Used to set the SSL-to-Java callback state before each SSL_*
      * call that may result in a callback. It should be cleared after
@@ -240,7 +226,7 @@ class AppData {
           sslHandshakeCallbacks(nullptr),
           applicationProtocolsData(nullptr),
           applicationProtocolsLength(static_cast<size_t>(-1)),
-          applicationProtocolSelector(nullptr) {
+          hasApplicationProtocolSelector(false) {
 #ifdef _WIN32
         interruptEvent = nullptr;
 #else
@@ -256,16 +242,6 @@ class AppData {
             applicationProtocolsLength = static_cast<size_t>(-1);
         }
     }
-
-    void clearApplicationProtocolSelector(JNIEnv* e) {
-        if (applicationProtocolSelector != nullptr) {
-            if (e == nullptr) {
-                e = conscrypt::jniutil::getJNIEnv();
-            }
-            e->DeleteGlobalRef(applicationProtocolSelector);
-            applicationProtocolSelector = nullptr;
-        }
-    }
 };
 
 }  // namespace conscrypt

common/src/main/java/org/conscrypt/ConscryptEngine.java

@@ -1761,6 +1761,15 @@ void setApplicationProtocolSelector(ApplicationProtocolSelectorAdapter adapter)
         sslParameters.setApplicationProtocolSelector(adapter);
     }
 
+    @Override
+    public int selectApplicationProtocol(byte[] protocols) {
+        ApplicationProtocolSelectorAdapter adapter = sslParameters.getApplicationProtocolSelector();
+        if (adapter == null) {
+            return NativeConstants.SSL_TLSEXT_ERR_NOACK;
+        }
+        return adapter.selectApplicationProtocol(protocols);
+    }
+
     @Override
     public String getApplicationProtocol() {
         return SSLUtils.toProtocolString(ssl.getApplicationProtocol());

common/src/main/java/org/conscrypt/ConscryptFileDescriptorSocket.java

@@ -1097,6 +1097,15 @@ final void setApplicationProtocolSelector(ApplicationProtocolSelectorAdapter sel
         sslParameters.setApplicationProtocolSelector(selector);
     }
 
+    @Override
+    public int selectApplicationProtocol(byte[] protocols) {
+        ApplicationProtocolSelectorAdapter adapter = sslParameters.getApplicationProtocolSelector();
+        if (adapter == null) {
+            return NativeConstants.SSL_TLSEXT_ERR_NOACK;
+        }
+        return adapter.selectApplicationProtocol(protocols);
+    }
+
     @Override
     final void setApplicationProtocols(String[] protocols) {
         sslParameters.setApplicationProtocols(protocols);

common/src/main/java/org/conscrypt/NativeCrypto.java

@@ -1289,6 +1289,16 @@ void verifyCertificateChain(byte[][] certificateChain, String authMethod)
          * @return the cached session or {@code 0} if no session was found matching the given ID.
          */
         @SuppressWarnings("unused") long serverSessionRequested(byte[] id);
+
+        /**
+         * Called when acting as a server, the socket has an {@link
+         * ApplicationProtocolSelectorAdapter} associated with it,  and the application protocol
+         * needs to be selected.
+         *
+         * @param applicationProtocols list of application protocols in length-prefix format
+         * @return the index offset of the selected protocol
+         */
+        @SuppressWarnings("unused") int selectApplicationProtocol(byte[] applicationProtocols);
     }
 
     static native String SSL_CIPHER_get_kx_name(long cipherAddress);
@@ -1330,12 +1340,13 @@ static native void setApplicationProtocols(
             long ssl, NativeSsl ssl_holder, boolean client, byte[] protocols) throws IOException;
 
     /**
-     * Called for a server endpoint only. Enables ALPN and sets a BiFunction that will
-     * be called to delegate protocol selection to the application. Calling this method overrides
+     * Called for a server endpoint only. Enables ALPN and indicates that the {@link
+     * SSLHandshakeCallbacks#selectApplicationProtocol} will be called to select the
+     * correct protocol during a handshake. Calling this method overrides
      * {@link #setApplicationProtocols(long, NativeSsl, boolean, byte[])}.
      */
-    static native void setApplicationProtocolSelector(
-            long ssl, NativeSsl ssl_holder, ApplicationProtocolSelectorAdapter selector) throws IOException;
+    static native void setHasApplicationProtocolSelector(long ssl, NativeSsl ssl_holder, boolean hasSelector)
+            throws IOException;
 
     /**
      * Returns the selected ALPN protocol. If the server did not select a

common/src/main/java/org/conscrypt/NativeSsl.java

@@ -319,7 +319,7 @@ void initialize(String hostname, OpenSSLKey channelIdPrivateKey) throws IOExcept
             NativeCrypto.setApplicationProtocols(ssl, this, isClient(), parameters.applicationProtocols);
         }
         if (!isClient() && parameters.applicationProtocolSelector != null) {
-            NativeCrypto.setApplicationProtocolSelector(ssl, this, parameters.applicationProtocolSelector);
+            NativeCrypto.setHasApplicationProtocolSelector(ssl, this, true);
         }
 
         // setup server certificates and private keys.

common/src/main/java/org/conscrypt/SSLParametersImpl.java

@@ -307,6 +307,13 @@ void setApplicationProtocolSelector(ApplicationProtocolSelectorAdapter applicati
         this.applicationProtocolSelector = applicationProtocolSelector;
     }
 
+    /**
+     * Returns the application protocol (ALPN) selector for this socket.
+     */
+    ApplicationProtocolSelectorAdapter getApplicationProtocolSelector() {
+        return applicationProtocolSelector;
+    }
+
     /**
      * Tunes the peer holding this parameters to work in client mode.
      * @param   mode if the peer is configured to work in client mode

constants/src/gen/cpp/generate_constants.cc

@@ -15,6 +15,7 @@
 /* This program generates output that is expected to become
  * NativeConstants.java. This reifies several OpenSSL constants into Java. */
 
+#include <openssl/tls1.h>
 #include <stdio.h>
 
 #include <openssl/ec.h>
@@ -77,6 +78,8 @@ int main(int /* argc */, char ** /* argv */) {
   CONST(TLS1_2_VERSION);
   CONST(TLS1_3_VERSION);
 
+  CONST(SSL_TLSEXT_ERR_NOACK);
+
   CONST(SSL_SENT_SHUTDOWN);
   CONST(SSL_RECEIVED_SHUTDOWN);