From 5745a7dd754dd8f07fad3c5b8a36f89da489aaf7 Mon Sep 17 00:00:00 2001 From: Kevin Cai Date: Sun, 25 Aug 2024 16:47:05 +0800 Subject: [PATCH] HDFS-16084. Fix getJNIEnv crash due to incorrect state set to tls var (#6969). Contributed by Kevin Cai. Signed-off-by: He Xiaoqiao --- .../src/main/native/libhdfs/jni_helper.c | 19 +++++--- .../native/libhdfspp/tests/CMakeLists.txt | 4 ++ .../libhdfspp/tests/libhdfs_getjni_test.cc | 44 +++++++++++++++++++ 3 files changed, 60 insertions(+), 7 deletions(-) create mode 100644 hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/libhdfs_getjni_test.cc diff --git a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfs/jni_helper.c b/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfs/jni_helper.c index 8f00a08b0a..47dce0086a 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfs/jni_helper.c +++ b/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfs/jni_helper.c @@ -818,26 +818,31 @@ JNIEnv* getJNIEnv(void) fprintf(stderr, "getJNIEnv: Unable to create ThreadLocalState\n"); return NULL; } - if (threadLocalStorageSet(state)) { - mutexUnlock(&jvmMutex); - goto fail; - } - THREAD_LOCAL_STORAGE_SET_QUICK(state); state->env = getGlobalJNIEnv(); - mutexUnlock(&jvmMutex); - if (!state->env) { + mutexUnlock(&jvmMutex); goto fail; } jthrowable jthr = NULL; jthr = initCachedClasses(state->env); if (jthr) { + mutexUnlock(&jvmMutex); printExceptionAndFree(state->env, jthr, PRINT_EXC_ALL, "initCachedClasses failed"); goto fail; } + + if (threadLocalStorageSet(state)) { + mutexUnlock(&jvmMutex); + goto fail; + } + + // set the TLS var only when the state passes all the checks + THREAD_LOCAL_STORAGE_SET_QUICK(state); + mutexUnlock(&jvmMutex); + return state->env; fail: diff --git a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/CMakeLists.txt b/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/CMakeLists.txt index 7eb432f31a..3e52c6d965 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/CMakeLists.txt +++ b/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/CMakeLists.txt @@ -74,6 +74,10 @@ add_executable(uri_test uri_test.cc) target_link_libraries(uri_test common gmock_main ${CMAKE_THREAD_LIBS_INIT}) add_memcheck_test(uri uri_test) +add_executable(get_jni_test libhdfs_getjni_test.cc) +target_link_libraries(get_jni_test gmock_main hdfs_static ${CMAKE_THREAD_LIBS_INIT}) +add_memcheck_test(get_jni get_jni_test) + add_executable(remote_block_reader_test remote_block_reader_test.cc) target_link_libraries(remote_block_reader_test test_common reader proto common connection ${PROTOBUF_LIBRARIES} ${OPENSSL_LIBRARIES} gmock_main ${CMAKE_THREAD_LIBS_INIT}) add_memcheck_test(remote_block_reader remote_block_reader_test) diff --git a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/libhdfs_getjni_test.cc b/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/libhdfs_getjni_test.cc new file mode 100644 index 0000000000..b2648da23b --- /dev/null +++ b/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/libhdfs_getjni_test.cc @@ -0,0 +1,44 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include +#include +#include + +// hook the jvm runtime function. expect always failure +_JNI_IMPORT_OR_EXPORT_ jint JNICALL JNI_GetDefaultJavaVMInitArgs(void*) { + return 1; +} + +// hook the jvm runtime function. expect always failure +_JNI_IMPORT_OR_EXPORT_ jint JNICALL JNI_CreateJavaVM(JavaVM**, void**, void*) { + return 1; +} + +TEST(GetJNITest, TestRepeatedGetJNIFailsButNoCrash) { + // connect to nothing, should fail but not crash + EXPECT_EQ(NULL, hdfsConnectNewInstance(NULL, 0)); + + // try again, should fail but not crash + EXPECT_EQ(NULL, hdfsConnectNewInstance(NULL, 0)); +} + +int main(int argc, char* argv[]) { + ::testing::InitGoogleMock(&argc, argv); + return RUN_ALL_TESTS(); +}