Prevent UAF issues caused by static destructors
Agents calling exit(3) would cause static destructors to run on
several openjdkjvmti data-structures. This caused problems as other
threads could still be running and might hit UAF issues, causing
errors. We fix this by making sure that no important jvmti
data-structures are statically allocated. Since these data-structures
must be live for the whole program runtime this is not a big issue.
Bug: 69591477
Test: ./test.py --host -j50
Test: while ./test/run-test --host 1944; do; done
Test: ./art/tools/run-libjdwp-tests.sh --mode=host --test \
org.apache.harmony.jpda.tests.jdwp.VirtualMachine.ExitTest.testExit001
Change-Id: I25b01fc129d46c5f15bce1b9ec7952d29088a5a2
diff --git a/openjdkjvmti/OpenjdkJvmTi.cc b/openjdkjvmti/OpenjdkJvmTi.cc
index a0c7f40..ef51519 100644
--- a/openjdkjvmti/OpenjdkJvmTi.cc
+++ b/openjdkjvmti/OpenjdkJvmTi.cc
@@ -73,8 +73,10 @@
namespace openjdkjvmti {
-EventHandler gEventHandler;
-DeoptManager gDeoptManager;
+// NB These are heap allocated to avoid the static destructors being run if an agent calls exit(3).
+// These should never be null.
+EventHandler* gEventHandler;
+DeoptManager* gDeoptManager;
#define ENSURE_NON_NULL(n) \
do { \
@@ -776,7 +778,7 @@
ENSURE_HAS_CAP(env, can_retransform_classes);
std::string error_msg;
jvmtiError res = Transformer::RetransformClasses(ArtJvmTiEnv::AsArtJvmTiEnv(env),
- &gEventHandler,
+ gEventHandler,
art::Runtime::Current(),
art::Thread::Current(),
class_count,
@@ -795,7 +797,7 @@
ENSURE_HAS_CAP(env, can_redefine_classes);
std::string error_msg;
jvmtiError res = Redefiner::RedefineClasses(ArtJvmTiEnv::AsArtJvmTiEnv(env),
- &gEventHandler,
+ gEventHandler,
art::Runtime::Current(),
art::Thread::Current(),
class_count,
@@ -1061,7 +1063,10 @@
}
ArtJvmTiEnv* art_env = ArtJvmTiEnv::AsArtJvmTiEnv(env);
- return gEventHandler.SetEvent(art_env, art_thread, GetArtJvmtiEvent(art_env, event_type), mode);
+ return gEventHandler->SetEvent(art_env,
+ art_thread,
+ GetArtJvmtiEvent(art_env, event_type),
+ mode);
}
static jvmtiError GenerateEvents(jvmtiEnv* env,
@@ -1095,7 +1100,7 @@
return ExtensionUtil::SetExtensionEventCallback(env,
extension_event_index,
callback,
- &gEventHandler);
+ gEventHandler);
}
#define FOR_ALL_CAPABILITIES(FUN) \
@@ -1186,9 +1191,9 @@
FOR_ALL_CAPABILITIES(ADD_CAPABILITY);
#undef ADD_CAPABILITY
- gEventHandler.HandleChangedCapabilities(ArtJvmTiEnv::AsArtJvmTiEnv(env),
- changed,
- /*added*/true);
+ gEventHandler->HandleChangedCapabilities(ArtJvmTiEnv::AsArtJvmTiEnv(env),
+ changed,
+ /*added*/true);
return ret;
}
@@ -1210,9 +1215,9 @@
FOR_ALL_CAPABILITIES(DEL_CAPABILITY);
#undef DEL_CAPABILITY
- gEventHandler.HandleChangedCapabilities(ArtJvmTiEnv::AsArtJvmTiEnv(env),
- changed,
- /*added*/false);
+ gEventHandler->HandleChangedCapabilities(ArtJvmTiEnv::AsArtJvmTiEnv(env),
+ changed,
+ /*added*/false);
return OK;
}
@@ -1302,7 +1307,7 @@
static jvmtiError DisposeEnvironment(jvmtiEnv* env) {
ENSURE_VALID_ENV(env);
ArtJvmTiEnv* tienv = ArtJvmTiEnv::AsArtJvmTiEnv(env);
- gEventHandler.RemoveArtJvmTiEnv(tienv);
+ gEventHandler->RemoveArtJvmTiEnv(tienv);
art::Runtime::Current()->RemoveSystemWeakHolder(tienv->object_tag_table.get());
ThreadUtil::RemoveEnvironment(tienv);
delete tienv;
@@ -1490,10 +1495,10 @@
// Creates a jvmtiEnv and returns it with the art::ti::Env that is associated with it. new_art_ti
// is a pointer to the uninitialized memory for an art::ti::Env.
static void CreateArtJvmTiEnv(art::JavaVMExt* vm, jint version, /*out*/void** new_jvmtiEnv) {
- struct ArtJvmTiEnv* env = new ArtJvmTiEnv(vm, &gEventHandler, version);
+ struct ArtJvmTiEnv* env = new ArtJvmTiEnv(vm, gEventHandler, version);
*new_jvmtiEnv = env;
- gEventHandler.RegisterArtJvmTiEnv(env);
+ gEventHandler->RegisterArtJvmTiEnv(env);
art::Runtime::Current()->AddSystemWeakHolder(
ArtJvmTiEnv::AsArtJvmTiEnv(env)->object_tag_table.get());
@@ -1522,17 +1527,20 @@
extern "C" bool ArtPlugin_Initialize() {
art::Runtime* runtime = art::Runtime::Current();
- gDeoptManager.Setup();
+ gDeoptManager = new DeoptManager;
+ gEventHandler = new EventHandler;
+
+ gDeoptManager->Setup();
if (runtime->IsStarted()) {
PhaseUtil::SetToLive();
} else {
PhaseUtil::SetToOnLoad();
}
- PhaseUtil::Register(&gEventHandler);
- ThreadUtil::Register(&gEventHandler);
- ClassUtil::Register(&gEventHandler);
- DumpUtil::Register(&gEventHandler);
- MethodUtil::Register(&gEventHandler);
+ PhaseUtil::Register(gEventHandler);
+ ThreadUtil::Register(gEventHandler);
+ ClassUtil::Register(gEventHandler);
+ DumpUtil::Register(gEventHandler);
+ MethodUtil::Register(gEventHandler);
SearchUtil::Register();
HeapUtil::Register();
Transformer::Setup();
@@ -1540,7 +1548,7 @@
{
// Make sure we can deopt anything we need to.
art::ScopedObjectAccess soa(art::Thread::Current());
- gDeoptManager.FinishSetup();
+ gDeoptManager->FinishSetup();
}
runtime->GetJavaVM()->AddEnvironmentHook(GetEnvHandler);
@@ -1549,8 +1557,8 @@
}
extern "C" bool ArtPlugin_Deinitialize() {
- gEventHandler.Shutdown();
- gDeoptManager.Shutdown();
+ gEventHandler->Shutdown();
+ gDeoptManager->Shutdown();
PhaseUtil::Unregister();
ThreadUtil::Unregister();
ClassUtil::Unregister();
@@ -1559,6 +1567,11 @@
SearchUtil::Unregister();
HeapUtil::Unregister();
+ // TODO It would be good to delete the gEventHandler and gDeoptManager here but we cannot since
+ // daemon threads might be suspended and we want to make sure that even if they wake up briefly
+ // they won't hit deallocated memory. By this point none of the functions will do anything since
+ // they have already shutdown.
+
return true;
}
diff --git a/openjdkjvmti/deopt_manager.cc b/openjdkjvmti/deopt_manager.cc
index 9e11a25..36998ee 100644
--- a/openjdkjvmti/deopt_manager.cc
+++ b/openjdkjvmti/deopt_manager.cc
@@ -343,9 +343,9 @@
art::Runtime::Current()->GetInstrumentation()->InstrumentThreadStack(target);
}
-extern DeoptManager gDeoptManager;
+extern DeoptManager* gDeoptManager;
DeoptManager* DeoptManager::Get() {
- return &gDeoptManager;
+ return gDeoptManager;
}
} // namespace openjdkjvmti
diff --git a/test/1944-sudden-exit/expected.txt b/test/1944-sudden-exit/expected.txt
new file mode 100644
index 0000000..98f8511
--- /dev/null
+++ b/test/1944-sudden-exit/expected.txt
@@ -0,0 +1,2 @@
+All threads started
+Exiting suddenly
diff --git a/test/1944-sudden-exit/info.txt b/test/1944-sudden-exit/info.txt
new file mode 100644
index 0000000..d575ce5
--- /dev/null
+++ b/test/1944-sudden-exit/info.txt
@@ -0,0 +1,5 @@
+Test to make sure the runtime will not crash if an agent calls exit(3) while
+other threads are performing operations.
+
+In this case we have multiple threads all performing single stepping when we
+call exit(3).
diff --git a/test/1944-sudden-exit/run b/test/1944-sudden-exit/run
new file mode 100755
index 0000000..51875a7
--- /dev/null
+++ b/test/1944-sudden-exit/run
@@ -0,0 +1,18 @@
+#!/bin/bash
+#
+# Copyright 2017 The Android Open Source Project
+#
+# Licensed 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.
+
+# Ask for stack traces to be dumped to a file rather than to stdout.
+./default-run "$@" --jvmti
diff --git a/test/1944-sudden-exit/src/Main.java b/test/1944-sudden-exit/src/Main.java
new file mode 100644
index 0000000..1644c6e
--- /dev/null
+++ b/test/1944-sudden-exit/src/Main.java
@@ -0,0 +1,21 @@
+/*
+ * Copyright (C) 2017 The Android Open Source Project
+ *
+ * Licensed 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.
+ */
+
+public class Main {
+ public static void main(String[] args) throws Exception {
+ art.Test1944.run();
+ }
+}
diff --git a/test/1944-sudden-exit/src/art/Test1944.java b/test/1944-sudden-exit/src/art/Test1944.java
new file mode 100644
index 0000000..36cbb2b
--- /dev/null
+++ b/test/1944-sudden-exit/src/art/Test1944.java
@@ -0,0 +1,69 @@
+/*
+ * Copyright (C) 2017 The Android Open Source Project
+ *
+ * Licensed 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.
+ */
+
+package art;
+
+import java.util.Arrays;
+import java.lang.reflect.Executable;
+import java.lang.reflect.Method;
+import java.util.concurrent.Semaphore;
+
+public class Test1944 {
+ // Just calculate fib forever.
+ public static void fib(Semaphore started) {
+ started.release();
+ long a = 1;
+ long b = 1;
+ while (true) {
+ long c = a + b;
+ a = b;
+ b = c;
+ }
+ }
+
+ // Don't bother actually doing anything.
+ public static void notifySingleStep(Thread thr, Executable e, long loc) { }
+
+ public static native void exitNow();
+
+ private static int num_threads = 10;
+
+ public static void run() throws Exception {
+ final Semaphore started = new Semaphore(-(num_threads - 1));
+
+ Trace.enableSingleStepTracing(Test1944.class,
+ Test1944.class.getDeclaredMethod(
+ "notifySingleStep", Thread.class, Executable.class, Long.TYPE),
+ null);
+
+ Thread[] threads = new Thread[num_threads];
+ for (int i = 0; i < num_threads; i++) {
+ threads[i] = new Thread(() -> { fib(started); });
+ // Make half daemons.
+ threads[i].setDaemon(i % 2 == 0);
+ threads[i].start();
+ }
+ // Wait for all threads to start.
+ started.acquire();
+ System.out.println("All threads started");
+ // sleep a little
+ Thread.sleep(10);
+ // Die.
+ System.out.println("Exiting suddenly");
+ exitNow();
+ System.out.println("FAILED: Should not reach here!");
+ }
+}
diff --git a/test/1944-sudden-exit/src/art/Trace.java b/test/1944-sudden-exit/src/art/Trace.java
new file mode 100644
index 0000000..8999bb1
--- /dev/null
+++ b/test/1944-sudden-exit/src/art/Trace.java
@@ -0,0 +1,68 @@
+/*
+ * Copyright (C) 2017 The Android Open Source Project
+ *
+ * Licensed 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.
+ */
+
+package art;
+
+import java.lang.reflect.Field;
+import java.lang.reflect.Method;
+
+public class Trace {
+ public static native void enableTracing(Class<?> methodClass,
+ Method entryMethod,
+ Method exitMethod,
+ Method fieldAccess,
+ Method fieldModify,
+ Method singleStep,
+ Thread thr);
+ public static native void disableTracing(Thread thr);
+
+ public static void enableFieldTracing(Class<?> methodClass,
+ Method fieldAccess,
+ Method fieldModify,
+ Thread thr) {
+ enableTracing(methodClass, null, null, fieldAccess, fieldModify, null, thr);
+ }
+
+ public static void enableMethodTracing(Class<?> methodClass,
+ Method entryMethod,
+ Method exitMethod,
+ Thread thr) {
+ enableTracing(methodClass, entryMethod, exitMethod, null, null, null, thr);
+ }
+
+ public static void enableSingleStepTracing(Class<?> methodClass,
+ Method singleStep,
+ Thread thr) {
+ enableTracing(methodClass, null, null, null, null, singleStep, thr);
+ }
+
+ public static native void watchFieldAccess(Field f);
+ public static native void watchFieldModification(Field f);
+ public static native void watchAllFieldAccesses();
+ public static native void watchAllFieldModifications();
+
+ // the names, arguments, and even line numbers of these functions are embedded in the tests so we
+ // need to add to the bottom and not modify old ones to maintain compat.
+ public static native void enableTracing2(Class<?> methodClass,
+ Method entryMethod,
+ Method exitMethod,
+ Method fieldAccess,
+ Method fieldModify,
+ Method singleStep,
+ Method ThreadStart,
+ Method ThreadEnd,
+ Thread thr);
+}
diff --git a/test/1944-sudden-exit/sudden_exit.cc b/test/1944-sudden-exit/sudden_exit.cc
new file mode 100644
index 0000000..2f5ce9b
--- /dev/null
+++ b/test/1944-sudden-exit/sudden_exit.cc
@@ -0,0 +1,32 @@
+/*
+ * Copyright (C) 2018 The Android Open Source Project
+ *
+ * Licensed 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 <stdlib.h>
+#include "jni.h"
+
+namespace art {
+namespace Test1944SuddenExit {
+
+extern "C" JNIEXPORT void JNICALL Java_art_Test1944_exitNow(JNIEnv*, jclass)
+ __attribute__((noreturn));
+
+extern "C" JNIEXPORT void JNICALL Java_art_Test1944_exitNow(JNIEnv*, jclass) {
+ exit(0);
+}
+
+} // namespace Test1944SuddenExit
+} // namespace art
+
diff --git a/test/Android.bp b/test/Android.bp
index 72e8eee..98e79ad 100644
--- a/test/Android.bp
+++ b/test/Android.bp
@@ -294,6 +294,7 @@
"936-search-onload/search_onload.cc",
"983-source-transform-verify/source_transform.cc",
"1940-ddms-ext/ddm_ext.cc",
+ "1944-sudden-exit/sudden_exit.cc",
],
}
diff --git a/tools/external_oj_libjdwp_art_failures.txt b/tools/external_oj_libjdwp_art_failures.txt
index 88ffa6e..6c2206f 100644
--- a/tools/external_oj_libjdwp_art_failures.txt
+++ b/tools/external_oj_libjdwp_art_failures.txt
@@ -53,12 +53,6 @@
"org.apache.harmony.jpda.tests.jdwp.VirtualMachine.CapabilitiesNewTest#testCapabilitiesNew001" ]
},
{
- description: "Test crashes",
- result: EXEC_FAILED,
- bug: 69591477,
- name: "org.apache.harmony.jpda.tests.jdwp.VirtualMachine.ExitTest#testExit001"
-},
-{
description: "Test times out on fugu-debug",
result: EXEC_FAILED,
bug: 70459916,
diff --git a/tools/prebuilt_libjdwp_art_failures.txt b/tools/prebuilt_libjdwp_art_failures.txt
index b0475bb..197ca80 100644
--- a/tools/prebuilt_libjdwp_art_failures.txt
+++ b/tools/prebuilt_libjdwp_art_failures.txt
@@ -106,11 +106,5 @@
result: EXEC_FAILED,
bug: 69169846,
name: "org.apache.harmony.jpda.tests.jdwp.DDM.DDMTest#testChunk001"
-},
-{
- description: "Test crashes",
- result: EXEC_FAILED,
- bug: 69591477,
- name: "org.apache.harmony.jpda.tests.jdwp.VirtualMachine.ExitTest#testExit001"
}
]