HDFS-16021. heap-use-after-free in hdfsThreadDestructor

hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfs/jni_helper.c

@@ -651,9 +651,14 @@ static char* getClassPath()
  * every thread.  You must be holding the jvmMutex when you call this
  * function.
  *
+ * @param[out] attachedByLibhdfs  Set to true if this call attached the current
+ *                                thread to the JVM, false if the thread was
+ *                                already attached by someone else. Only the
+ *                                former may be detached at thread exit.
+ *
  * @return          The JNIEnv on success; error code otherwise
  */
-static JNIEnv* getGlobalJNIEnv(void)
+static JNIEnv* getGlobalJNIEnv(bool *attachedByLibhdfs)
 {
     JavaVM* vmBuf[VM_BUF_LENGTH]; 
     JNIEnv *env;
@@ -672,6 +677,7 @@ static JNIEnv* getGlobalJNIEnv(void)
     JavaVM *vm;
     JavaVMOption *options;
 
+    *attachedByLibhdfs = false;
     rv = JNI_GetCreatedJavaVMs(&(vmBuf[0]), VM_BUF_LENGTH, &noVMs);
     if (rv != 0) {
         fprintf(stderr, "JNI_GetCreatedJavaVMs failed with error: %d\n", rv);
@@ -755,15 +761,30 @@ static JNIEnv* getGlobalJNIEnv(void)
                     "FileSystem: loadFileSystems failed");
             return NULL;
         }
+        *attachedByLibhdfs = true;
     } else {
-        //Attach this thread to the VM
         vm = vmBuf[0];
+        // Reuse an existing attachment rather than creating one. On a thread
+        // the JVM or the embedding application already attached,
+        // AttachCurrentThread succeeds and hands back the same JNIEnv, which
+        // would leave libhdfs believing it owns an attachment it did not make
+        // and detaching it at thread exit.
+        rv = (*vm)->GetEnv(vm, (void**)&env, JNI_VERSION_1_2);
+        if (rv == JNI_OK) {
+            return env;
+        }
+        if (rv != JNI_EDETACHED) {
+            fprintf(stderr, "Call to GetEnv failed with error: %d\n", rv);
+            return NULL;
+        }
+        //Attach this thread to the VM
         rv = (*vm)->AttachCurrentThread(vm, (void*)&env, 0);
         if (rv != 0) {
             fprintf(stderr, "Call to AttachCurrentThread "
                     "failed with error: %d\n", rv);
             return NULL;
         }
+        *attachedByLibhdfs = true;
     }
 
     return env;
@@ -819,7 +840,7 @@ JNIEnv* getJNIEnv(void)
       return NULL;
     }
 
-    state->env = getGlobalJNIEnv();
+    state->env = getGlobalJNIEnv(&state->attachedByLibhdfs);
     if (!state->env) {
         mutexUnlock(&jvmMutex);
         goto fail;

hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfs/os/posix/thread_local_storage.c

@@ -52,8 +52,11 @@ void hdfsThreadDestructor(void *v)
   jthrowable jthr;
   char thr_name[MAXTHRID];
 
-  /* Detach the current thread from the JVM */
-  if ((env != NULL) && (*env != NULL)) {
+  /* Detach only threads that libhdfs attached to the JVM. Detaching a thread
+   * that the JVM (or an embedding application) attached frees a JNIEnv its
+   * owner still holds, and by the time this destructor runs that env may
+   * already have been freed, so the dereference below reads freed memory. */
+  if (state->attachedByLibhdfs && (env != NULL) && (*env != NULL)) {
     ret = (*env)->GetJavaVM(env, &vm);
 
     if (ret != 0) {
@@ -158,6 +161,8 @@ struct ThreadLocalState* threadLocalStorageCreate()
       "threadLocalStorageCreate: OOM - Unable to allocate thread local state\n");
     return NULL;
   }
+  state->attachedByLibhdfs = false;
+  state->env = NULL;
   state->lastExceptionStackTrace = NULL;
   state->lastExceptionRootCause = NULL;
   return state;

hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfs/os/thread_local_storage.h

@@ -28,6 +28,8 @@
 
 #include <jni.h>
 
+#include <stdbool.h>
+
 /*
  * Most operating systems support the more efficient __thread construct, which
  * is initialized by the linker.  The following macros use this technique on the
@@ -52,6 +54,8 @@
 #endif
 
 struct ThreadLocalState {
+  /* Whether libhdfs attached this thread to the JVM. */
+  bool attachedByLibhdfs;
   /* The JNIEnv associated with the current thread */
   JNIEnv *env;
   /* The last exception stack trace that occurred on this thread */