diff --git a/mobile/library/common/jni/BUILD b/mobile/library/common/jni/BUILD index 13536a8f68d77..caaa9cc616dde 100644 --- a/mobile/library/common/jni/BUILD +++ b/mobile/library/common/jni/BUILD @@ -21,6 +21,7 @@ cc_library( deps = [ "//library/common/jni/import:jni_import_lib", "//library/common/types:c_types_lib", + "//library/common/types:managed_types_lib", "@envoy//source/common/common:assert_lib", ], ) @@ -48,6 +49,7 @@ cc_library( ":ndk_jni_support", "//library/common:envoy_main_interface_lib", "//library/common/api:c_types", + "//library/common/types:managed_types_lib", ], # We need this to ensure that we link this into the .so even though there are no code references. alwayslink = True, @@ -115,6 +117,7 @@ cc_binary( "//library/common:envoy_main_interface_lib", "//library/common/api:c_types", "//library/common/jni/import:jni_import_lib", + "//library/common/types:managed_types_lib", ], ) @@ -169,6 +172,7 @@ cc_library( "//library/common/jni/import:jni_import_lib", "//library/common:envoy_main_interface_lib", "//library/common/types:c_types_lib", + "//library/common/types:managed_types_lib", "@envoy//source/common/common:assert_lib", ] + TEST_EXTENSIONS, ) diff --git a/mobile/library/common/jni/jni_interface.cc b/mobile/library/common/jni/jni_interface.cc index cc9942f22459a..8dded5a7a4f11 100644 --- a/mobile/library/common/jni/jni_interface.cc +++ b/mobile/library/common/jni/jni_interface.cc @@ -8,6 +8,7 @@ #include "library/common/jni/jni_utility.h" #include "library/common/jni/jni_version.h" #include "library/common/main_interface.h" +#include "library/common/types/managed_envoy_headers.h" // NOLINT(namespace-envoy) @@ -278,22 +279,23 @@ Java_io_envoyproxy_envoymobile_engine_JniLibrary_recordHistogramValue(JNIEnv* en // JvmCallbackContext -static void pass_headers(const char* method, envoy_headers headers, jobject j_context) { +static void passHeaders(const char* method, const Envoy::Types::ManagedEnvoyHeaders& headers, + jobject j_context) { JNIEnv* env = get_env(); jclass jcls_JvmCallbackContext = env->GetObjectClass(j_context); jmethodID jmid_passHeader = env->GetMethodID(jcls_JvmCallbackContext, method, "([B[BZ)V"); jboolean start_headers = JNI_TRUE; - for (envoy_map_size_t i = 0; i < headers.length; i++) { + for (envoy_map_size_t i = 0; i < headers.get().length; i++) { // Note: this is just an initial implementation, and we will pass a more optimized structure in // the future. // Note: the JNI function NewStringUTF would appear to be an appealing option here, except it // requires a null-terminated *modified* UTF-8 string. // Create platform byte array for header key - jbyteArray j_key = native_data_to_array(env, headers.entries[i].key); + jbyteArray j_key = native_data_to_array(env, headers.get().entries[i].key); // Create platform byte array for header value - jbyteArray j_value = native_data_to_array(env, headers.entries[i].value); + jbyteArray j_value = native_data_to_array(env, headers.get().entries[i].value); // Pass this header pair to the platform env->CallVoidMethod(j_context, jmid_passHeader, j_key, j_value, start_headers); @@ -306,19 +308,18 @@ static void pass_headers(const char* method, envoy_headers headers, jobject j_co } env->DeleteLocalRef(jcls_JvmCallbackContext); - release_envoy_headers(headers); } // Platform callback implementation // These methods call jvm methods which means the local references created will not be // released automatically. Manual bookkeeping is required for these methods. -static void* jvm_on_headers(const char* method, envoy_headers headers, bool end_stream, - envoy_stream_intel stream_intel, void* context) { +static void* jvm_on_headers(const char* method, const Envoy::Types::ManagedEnvoyHeaders& headers, + bool end_stream, envoy_stream_intel stream_intel, void* context) { jni_log("[Envoy]", "jvm_on_headers"); JNIEnv* env = get_env(); jobject j_context = static_cast(context); - pass_headers("passHeader", headers, j_context); + passHeaders("passHeader", headers, j_context); jclass jcls_JvmCallbackContext = env->GetObjectClass(j_context); jmethodID jmid_onHeaders = @@ -327,24 +328,52 @@ static void* jvm_on_headers(const char* method, envoy_headers headers, bool end_ jlongArray j_stream_intel = native_stream_intel_to_array(env, stream_intel); // Note: be careful of JVM types. Before we casted to jlong we were getting integer problems. // TODO: make this cast safer. - jobject result = env->CallObjectMethod(j_context, jmid_onHeaders, (jlong)headers.length, + jobject result = env->CallObjectMethod(j_context, jmid_onHeaders, (jlong)headers.get().length, end_stream ? JNI_TRUE : JNI_FALSE, j_stream_intel); + // TODO(Augustyniak): Pass the name of the filter in here so that we can instrument the origin of + // the JNI exception better. + bool exception_cleared = clear_pending_exceptions(env); env->DeleteLocalRef(j_stream_intel); env->DeleteLocalRef(jcls_JvmCallbackContext); - return result; + if (!exception_cleared) { + return result; + } + + // Create a "no operation" result: + // 1. Tell the filter chain to continue the iteration. + // 2. Return headers received on as method's input as part of the method's output. + jclass jcls_object_array = env->FindClass("java/lang/Object"); + jobjectArray noopResult = env->NewObjectArray(2, jcls_object_array, NULL); + + jclass jcls_int = env->FindClass("java/lang/Integer"); + jmethodID jmid_intInit = env->GetMethodID(jcls_int, "", "(I)V"); + jobject j_status = env->NewObject(jcls_int, jmid_intInit, 0); + // Set status to "0" (FilterHeadersStatus::Continue). Signal that the intent + // is to continue the iteration of the filter chain. + env->SetObjectArrayElement(noopResult, 0, j_status); + + // Since the "on headers" call threw an exception set input headers as output headers. + env->SetObjectArrayElement(noopResult, 1, ToJavaArrayOfObjectArray(env, headers)); + + env->DeleteLocalRef(jcls_object_array); + env->DeleteLocalRef(jcls_int); + + return noopResult; } static void* jvm_on_response_headers(envoy_headers headers, bool end_stream, envoy_stream_intel stream_intel, void* context) { - return jvm_on_headers("onResponseHeaders", headers, end_stream, stream_intel, context); + const auto managed_headers = Envoy::Types::ManagedEnvoyHeaders(headers); + return jvm_on_headers("onResponseHeaders", managed_headers, end_stream, stream_intel, context); } static envoy_filter_headers_status -jvm_http_filter_on_request_headers(envoy_headers headers, bool end_stream, +jvm_http_filter_on_request_headers(envoy_headers input_headers, bool end_stream, envoy_stream_intel stream_intel, const void* context) { JNIEnv* env = get_env(); + const auto headers = Envoy::Types::ManagedEnvoyHeaders(input_headers); jobjectArray result = static_cast(jvm_on_headers( "onRequestHeaders", headers, end_stream, stream_intel, const_cast(context))); @@ -363,9 +392,10 @@ jvm_http_filter_on_request_headers(envoy_headers headers, bool end_stream, } static envoy_filter_headers_status -jvm_http_filter_on_response_headers(envoy_headers headers, bool end_stream, +jvm_http_filter_on_response_headers(envoy_headers input_headers, bool end_stream, envoy_stream_intel stream_intel, const void* context) { JNIEnv* env = get_env(); + const auto headers = Envoy::Types::ManagedEnvoyHeaders(input_headers); jobjectArray result = static_cast(jvm_on_headers( "onResponseHeaders", headers, end_stream, stream_intel, const_cast(context))); @@ -484,7 +514,7 @@ static void* jvm_on_trailers(const char* method, envoy_headers trailers, JNIEnv* env = get_env(); jobject j_context = static_cast(context); - pass_headers("passHeader", trailers, j_context); + passHeaders("passHeader", trailers, j_context); jclass jcls_JvmCallbackContext = env->GetObjectClass(j_context); jmethodID jmid_onTrailers = @@ -493,6 +523,7 @@ static void* jvm_on_trailers(const char* method, envoy_headers trailers, jlongArray j_stream_intel = native_stream_intel_to_array(env, stream_intel); // Note: be careful of JVM types. Before we casted to jlong we were getting integer problems. // TODO: make this cast safer. + // TODO(Augustyniak): check for pending exceptions after returning from JNI call. jobject result = env->CallObjectMethod(j_context, jmid_onTrailers, (jlong)trailers.length, j_stream_intel); @@ -632,7 +663,7 @@ jvm_http_filter_on_resume(const char* method, envoy_headers* headers, envoy_data jlong headers_length = -1; if (headers) { headers_length = (jlong)headers->length; - pass_headers("passHeader", *headers, j_context); + passHeaders("passHeader", *headers, j_context); } jbyteArray j_in_data = nullptr; if (data) { @@ -641,7 +672,7 @@ jvm_http_filter_on_resume(const char* method, envoy_headers* headers, envoy_data jlong trailers_length = -1; if (trailers) { trailers_length = (jlong)trailers->length; - pass_headers("passTrailer", *trailers, j_context); + passHeaders("passTrailer", *trailers, j_context); } jlongArray j_stream_intel = native_stream_intel_to_array(env, stream_intel); @@ -714,6 +745,8 @@ static void* call_jvm_on_complete(envoy_stream_intel stream_intel, jobject result = env->CallObjectMethod(j_context, jmid_onComplete, j_stream_intel, j_final_stream_intel); + clear_pending_exceptions(env); + env->DeleteLocalRef(j_stream_intel); env->DeleteLocalRef(j_final_stream_intel); env->DeleteLocalRef(jcls_JvmObserverContext); @@ -737,6 +770,8 @@ static void* call_jvm_on_error(envoy_error error, envoy_stream_intel stream_inte jobject result = env->CallObjectMethod(j_context, jmid_onError, error.error_code, j_error_message, error.attempt_count, j_stream_intel, j_final_stream_intel); + clear_pending_exceptions(env); + env->DeleteLocalRef(j_stream_intel); env->DeleteLocalRef(j_final_stream_intel); env->DeleteLocalRef(j_error_message); @@ -769,6 +804,8 @@ static void* call_jvm_on_cancel(envoy_stream_intel stream_intel, jobject result = env->CallObjectMethod(j_context, jmid_onCancel, j_stream_intel, j_final_stream_intel); + clear_pending_exceptions(env); + env->DeleteLocalRef(j_stream_intel); env->DeleteLocalRef(j_final_stream_intel); env->DeleteLocalRef(jcls_JvmObserverContext); diff --git a/mobile/library/common/jni/jni_utility.cc b/mobile/library/common/jni/jni_utility.cc index 8fbc0da0b8d94..3a4debc698e61 100644 --- a/mobile/library/common/jni/jni_utility.cc +++ b/mobile/library/common/jni/jni_utility.cc @@ -66,6 +66,16 @@ void jni_delete_const_global_ref(const void* context) { jni_delete_global_ref(const_cast(context)); } +bool clear_pending_exceptions(JNIEnv* env) { + if (env->ExceptionCheck() == JNI_TRUE) { + env->ExceptionClear(); + // TODO(Augustyniak): Log exception details. + return true; + } else { + return false; + } +} + int unbox_integer(JNIEnv* env, jobject boxedInteger) { jclass jcls_Integer = env->FindClass("java/lang/Integer"); jmethodID jmid_intValue = env->GetMethodID(jcls_Integer, "intValue", "()I"); @@ -278,6 +288,21 @@ envoy_map to_native_map(JNIEnv* env, jobjectArray entries) { return native_map; } +jobjectArray ToJavaArrayOfObjectArray(JNIEnv* env, const Envoy::Types::ManagedEnvoyHeaders& map) { + jclass jcls_byte_array = env->FindClass("java/lang/Object"); + jobjectArray javaArray = env->NewObjectArray(2 * map.get().length, jcls_byte_array, nullptr); + + for (envoy_map_size_t i = 0; i < map.get().length; i++) { + jbyteArray key = native_data_to_array(env, map.get().entries[i].key); + jbyteArray value = native_data_to_array(env, map.get().entries[i].value); + + env->SetObjectArrayElement(javaArray, 2 * i, key); + env->SetObjectArrayElement(javaArray, 2 * i + 1, value); + } + + return javaArray; +} + jobjectArray ToJavaArrayOfByteArray(JNIEnv* env, const std::vector& v) { jclass jcls_byte_array = env->FindClass("[B"); jobjectArray joa = env->NewObjectArray(v.size(), jcls_byte_array, nullptr); diff --git a/mobile/library/common/jni/jni_utility.h b/mobile/library/common/jni/jni_utility.h index 2c7f2875f5eff..572fee8a0dc32 100644 --- a/mobile/library/common/jni/jni_utility.h +++ b/mobile/library/common/jni/jni_utility.h @@ -5,6 +5,7 @@ #include "library/common/jni/import/jni_import.h" #include "library/common/types/c_types.h" +#include "library/common/types/managed_envoy_headers.h" // NOLINT(namespace-envoy) @@ -43,6 +44,15 @@ void jni_delete_global_ref(void* context); void jni_delete_const_global_ref(const void* context); +/** + * Clears any pending exceptions that may have been rides in result to a call into Java code. + * + * @param env, the JNI env pointer. + * + * @return Whether any pending JNI exception was cleared. + */ +bool clear_pending_exceptions(JNIEnv* env); + int unbox_integer(JNIEnv* env, jobject boxedInteger); envoy_data array_to_native_data(JNIEnv* env, jbyteArray j_data); @@ -100,6 +110,8 @@ jbyteArray ToJavaByteArray(JNIEnv* env, const uint8_t* bytes, size_t len); jbyteArray ToJavaByteArray(JNIEnv* env, const std::string& str); +jobjectArray ToJavaArrayOfObjectArray(JNIEnv* env, const Envoy::Types::ManagedEnvoyHeaders& map); + void JavaArrayOfByteArrayToStringVector(JNIEnv* env, jobjectArray array, std::vector* out); diff --git a/mobile/library/common/types/BUILD b/mobile/library/common/types/BUILD index e4a61c2cdbb7b..3d1c6b8fbf5f6 100644 --- a/mobile/library/common/types/BUILD +++ b/mobile/library/common/types/BUILD @@ -18,3 +18,15 @@ envoy_cc_library( "@envoy//source/common/common:assert_lib", ], ) + +envoy_cc_library( + name = "managed_types_lib", + srcs = [ + "managed_envoy_headers.h", + ], + repository = "@envoy", + visibility = ["//visibility:public"], + deps = [ + "//library/common/types:c_types_lib", + ], +) diff --git a/mobile/library/common/types/managed_envoy_headers.h b/mobile/library/common/types/managed_envoy_headers.h new file mode 100644 index 0000000000000..2e2072eb7245e --- /dev/null +++ b/mobile/library/common/types/managed_envoy_headers.h @@ -0,0 +1,32 @@ +#pragma once + +#include "library/common/types/c_types.h" + +namespace Envoy { +namespace Types { + +/** + * A wrapper around envoy_headers that's responsible for freeing + * the underlying headers when they are not needed anymore. + */ +class ManagedEnvoyHeaders { +public: + /** + * Initialize a new instance of the receiver using a given instance of envoy headers. + * + * @param headers, that should be wrapped by the receiver. The wrapper will hold onto + * the passed headers and free them once the receiver is not used anymore. + */ + ManagedEnvoyHeaders(envoy_headers headers) : headers_(headers){}; + ~ManagedEnvoyHeaders() { release_envoy_headers(headers_); } + const envoy_headers& get() const { return headers_; } + +private: + envoy_headers headers_; + // Make copy and assignment operators private to prevent copying of the receiver. + ManagedEnvoyHeaders(const ManagedEnvoyHeaders&); + ManagedEnvoyHeaders& operator=(const ManagedEnvoyHeaders&); +}; + +} // namespace Types +} // namespace Envoy diff --git a/mobile/test/kotlin/integration/BUILD b/mobile/test/kotlin/integration/BUILD index a6ce4f629f006..3325a82f6c97c 100644 --- a/mobile/test/kotlin/integration/BUILD +++ b/mobile/test/kotlin/integration/BUILD @@ -1,4 +1,5 @@ -load("@envoy_mobile//bazel:kotlin_test.bzl", "envoy_mobile_jni_kt_test") +load("@envoy_mobile//bazel:kotlin_test.bzl", "envoy_mobile_android_test", "envoy_mobile_jni_kt_test") +load("@envoy_mobile//bazel:kotlin_lib.bzl", "envoy_mobile_kt_library") envoy_mobile_jni_kt_test( name = "engine_start_test", @@ -200,3 +201,23 @@ envoy_mobile_jni_kt_test( "//library/kotlin/io/envoyproxy/envoymobile:envoy_interfaces_lib", ], ) + +envoy_mobile_android_test( + name = "filter_throwing_exception_test", + srcs = [ + "FilterThrowingExceptionTest.kt", + ], + exec_properties = { + # TODO(lfpino): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. + "sandboxNetwork": "standard", + "dockerNetwork": "standard", + }, + native_deps = [ + "//library/common/jni:libndk_envoy_jni.so", + "//library/common/jni:libndk_envoy_jni.jnilib", + ], + deps = [ + "//library/kotlin/io/envoyproxy/envoymobile:envoy_interfaces_lib", + "//library/kotlin/io/envoyproxy/envoymobile:envoy_lib", + ], +) diff --git a/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt b/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt new file mode 100644 index 0000000000000..aa6f993dadabf --- /dev/null +++ b/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt @@ -0,0 +1,134 @@ +package test.kotlin.integration + +import android.content.Context +import androidx.test.core.app.ApplicationProvider + +import io.envoyproxy.envoymobile.AndroidEngineBuilder +import io.envoyproxy.envoymobile.EnvoyError +import io.envoyproxy.envoymobile.Engine +import io.envoyproxy.envoymobile.engine.JniLibrary +import io.envoyproxy.envoymobile.FilterDataStatus +import io.envoyproxy.envoymobile.FilterHeadersStatus +import io.envoyproxy.envoymobile.FilterTrailersStatus +import io.envoyproxy.envoymobile.FinalStreamIntel +import io.envoyproxy.envoymobile.LogLevel +import io.envoyproxy.envoymobile.RequestFilter +import io.envoyproxy.envoymobile.RequestHeaders +import io.envoyproxy.envoymobile.RequestHeadersBuilder +import io.envoyproxy.envoymobile.RequestMethod +import io.envoyproxy.envoymobile.RequestTrailers +import io.envoyproxy.envoymobile.ResponseFilter +import io.envoyproxy.envoymobile.ResponseHeaders +import io.envoyproxy.envoymobile.ResponseTrailers +import io.envoyproxy.envoymobile.StreamIntel + +import java.nio.ByteBuffer +import java.util.concurrent.CountDownLatch +import java.util.concurrent.Executors +import java.util.concurrent.TimeUnit + +import org.assertj.core.api.Assertions.assertThat +import org.junit.Assert.assertNotNull +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +class ThrowingFilter: RequestFilter, ResponseFilter { + override fun onRequestHeaders( + headers: RequestHeaders, + endStream: Boolean, + streamIntel: StreamIntel + ): FilterHeadersStatus { + throw Exception("Simulated onRequestHeaders exception") + } + + override fun onRequestData(body: ByteBuffer, endStream: Boolean, streamIntel: StreamIntel): + FilterDataStatus { + return FilterDataStatus.Continue(body) + } + + override fun onRequestTrailers(trailers: RequestTrailers, streamIntel: StreamIntel): + FilterTrailersStatus { + return FilterTrailersStatus.Continue(trailers) + } + + override fun onResponseHeaders( + headers: ResponseHeaders, + endStream: Boolean, + streamIntel: StreamIntel + ): FilterHeadersStatus { + throw Exception("Simulated onResponseHeaders exception") + } + + override fun onResponseData( + body: ByteBuffer, + endStream: Boolean, + streamIntel: StreamIntel + ): FilterDataStatus { + return FilterDataStatus.Continue(body) + } + + override fun onResponseTrailers( + trailers: ResponseTrailers, + streamIntel: StreamIntel + ): FilterTrailersStatus { + return FilterTrailersStatus.Continue(trailers) + } + + override fun onError( + error: EnvoyError, + finalStreamIntel: FinalStreamIntel + ) {} + + override fun onCancel(finalStreamIntel: FinalStreamIntel) {} + + override fun onComplete(finalStreamIntel: FinalStreamIntel) {} +} + +@RunWith(RobolectricTestRunner::class) +class FilterThrowingExceptionTest { + init { + JniLibrary.loadTestLibrary() + } + + @Test + fun `registers a filter that throws an exception and performs an HTTP request`() { + val onEngineRunningLatch = CountDownLatch(1) + val onRespondeHeadersLatch = CountDownLatch(1) + + val context = ApplicationProvider.getApplicationContext() + val builder = AndroidEngineBuilder(context) + val engine = builder + .addLogLevel(LogLevel.DEBUG) + .addPlatformFilter(::ThrowingFilter) + .setOnEngineRunning { onEngineRunningLatch.countDown() } + .build() + + onEngineRunningLatch.await(10, TimeUnit.SECONDS) + assertThat(onEngineRunningLatch.count).isEqualTo(0) + + val requestHeaders = RequestHeadersBuilder( + method = RequestMethod.GET, + scheme = "https", + authority = "api.lyft.com", + path = "/ping" + ) + .build() + + engine + .streamClient() + .newStreamPrototype() + .setOnResponseHeaders { responseHeaders, _, _ -> + val status = responseHeaders.httpStatus ?: 0L + assertThat(status).isEqualTo(200) + onRespondeHeadersLatch.countDown() + } + .start(Executors.newSingleThreadExecutor()) + .sendHeaders(requestHeaders, true) + + onRespondeHeadersLatch.await(15, TimeUnit.SECONDS) + assertThat(onRespondeHeadersLatch.count).isEqualTo(0) + + engine.terminate() + } +}