HDFS-16084. Fix getJNIEnv crash due to incorrect state set to tls var

Signed-off-by: Kevin Cai <[email protected]>

Author: kevincai

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:

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)

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 <gmock/gmock.h>
+#include <hdfs/hdfs.h>
+#include <jni.h>
+
+// 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();
+}