Revert "Revert "Prevent UAF issues caused by static destructors""
We were not initializing the frame_pop_enabled field of EventHandler
so it was undefined. If it happened to be true then the FramePop
handlers will never be installed and the FramePop events will never be
triggered.
This reverts commit 6a94cec7343bf006234b62525398c83bb55858eb.
Reason for revert: fixed issue with frame pop
Test: ./test/testrunner/testrunner.py -a --host -t 923
Bug: 69591477
Change-Id: Id47f91a76b6e7c9326e94d7cbdf8c5472bffb58a
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/openjdkjvmti/events.cc b/openjdkjvmti/events.cc
index 62b73c0..8b40a7e 100644
--- a/openjdkjvmti/events.cc
+++ b/openjdkjvmti/events.cc
@@ -1186,8 +1186,9 @@
art::Runtime::Current()->GetInstrumentation()->RemoveListener(method_trace_listener_.get(), ~0);
}
-EventHandler::EventHandler() : envs_lock_("JVMTI Environment List Lock",
- art::LockLevel::kTopLockLevel) {
+EventHandler::EventHandler()
+ : envs_lock_("JVMTI Environment List Lock", art::LockLevel::kTopLockLevel),
+ frame_pop_enabled(false) {
alloc_listener_.reset(new JvmtiAllocationListener(this));
ddm_listener_.reset(new JvmtiDdmChunkListener(this));
gc_pause_listener_.reset(new JvmtiGcPauseListener(this));
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 828c0aa..ba764df 100644
--- a/tools/external_oj_libjdwp_art_failures.txt
+++ b/tools/external_oj_libjdwp_art_failures.txt
@@ -66,12 +66,6 @@
"org.apache.harmony.jpda.tests.jdwp.MultiSession.EnableCollectionTest#testEnableCollection001" ]
},
{
- 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 a9d268d..5251daf 100644
--- a/tools/prebuilt_libjdwp_art_failures.txt
+++ b/tools/prebuilt_libjdwp_art_failures.txt
@@ -119,11 +119,5 @@
bug: 70958370,
names: [ "org.apache.harmony.jpda.tests.jdwp.ObjectReference.EnableCollectionTest#testEnableCollection001",
"org.apache.harmony.jpda.tests.jdwp.MultiSession.EnableCollectionTest#testEnableCollection001" ]
-},
-{
- description: "Test crashes",
- result: EXEC_FAILED,
- bug: 69591477,
- name: "org.apache.harmony.jpda.tests.jdwp.VirtualMachine.ExitTest#testExit001"
}
]