Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8267989: Exceptions thrown during upcalls should be handled #543

@@ -25,8 +25,6 @@
*/
package jdk.incubator.foreign;

import jdk.internal.access.JavaLangAccess;
import jdk.internal.access.SharedSecrets;
import jdk.internal.foreign.NativeMemorySegmentImpl;
import jdk.internal.foreign.PlatformLayouts;
import jdk.internal.foreign.SystemLookup;
@@ -39,11 +37,9 @@
import java.lang.invoke.MethodType;
import java.nio.charset.Charset;
import java.util.Objects;
import java.util.Optional;
import java.util.function.Consumer;

import static jdk.internal.foreign.PlatformLayouts.*;
import static sun.security.action.GetIntegerAction.privilegedGetProperty;

/**
* A C linker implements the C Application Binary Interface (ABI) calling conventions.
@@ -120,7 +116,7 @@
*
* @see CLinker#upcallStub(MethodHandle, FunctionDescriptor, ResourceScope)
*/
int ERR_UNCAUGHT_EXCEPTION = privilegedGetProperty("jdk.incubator.foreign.uncaught_exception_code", 1);
int ERR_UNCAUGHT_EXCEPTION = 1;
This conversation was marked as resolved by JornVernee

This comment has been minimized.

@mcimadamore

mcimadamore May 31, 2021
Collaborator Outdated

On a second thought, I'm not super sure of the value of documenting what the exit code should be? Maybe we can leave that unspecified?

This comment has been minimized.

@JornVernee

JornVernee Jun 1, 2021
Author Member Outdated

I agree. Without a system property, having a constant doesn't really make sense. I looked around in the documentation of exit codes in the JDK, and most places seem to at least specify the exit code to be non-zero in case of error. I'll add a note like that to the documentation of upcallStub.


/**
* Returns the C linker for the current platform.
@@ -236,9 +232,9 @@ static SymbolLookup systemLookup() {
* The returned memory address is associated with the provided scope. When such scope is closed,
* the corresponding native stub will be deallocated.
* <p>
* Any exceptions that occur during an upcall should be handled during the upcall. The target method handle
* should not throw any exceptions. If the target method handle does throw an exception, it will be handle by
* calling {@link System#exit System.exit(ERR_UNCAUGHT_EXCEPTION)}. (See {@link #ERR_UNCAUGHT_EXCEPTION})
* The target method handle should not throw any exceptions. If the target method handle does throw an exception,
* it will be handle by calling {@link System#exit System.exit(ERR_UNCAUGHT_EXCEPTION)}.
* (See {@link #ERR_UNCAUGHT_EXCEPTION})
* <p>
* This method is <a href="package-summary.html#restricted"><em>restricted</em></a>.
* Restricted method are unsafe, and, if used incorrectly, their use might crash
@@ -36,11 +36,15 @@
import jdk.test.lib.Utils;
import org.testng.annotations.Test;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.nio.file.Paths;
import java.util.concurrent.ThreadLocalRandom;

import static jdk.incubator.foreign.CLinker.ERR_UNCAUGHT_EXCEPTION;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.fail;

public class TestUpcallException {
This conversation was marked as resolved by JornVernee

This comment has been minimized.

@mcimadamore

mcimadamore May 31, 2021
Collaborator

I wonder if we should also test with a security manager (even though that's getting deprecated) - and see what happens when you call System.exit and SM is enabled with policy to block off calls to System.exit.

Using Shutdown.exit might be safer, as it does not SM checks.


@@ -57,10 +61,7 @@ public void testExceptionSpecialized() throws IOException, InterruptedException
}

private void run(boolean useSpec) throws IOException, InterruptedException {
int exitCode = ThreadLocalRandom.current().nextInt();

Process process = new ProcessBuilder()
.inheritIO()
.command(
Paths.get(Utils.TEST_JDK)
.resolve("bin")
@@ -71,13 +72,24 @@ private void run(boolean useSpec) throws IOException, InterruptedException {
"--enable-native-access=ALL-UNNAMED",
"-Djava.library.path=" + System.getProperty("java.library.path"),
"-Djdk.internal.foreign.ProgrammableUpcallHandler.USE_SPEC=" + useSpec,
"-Djdk.incubator.foreign.uncaught_exception_code=" + exitCode,
"-cp", Utils.TEST_CLASS_PATH,
"ThrowingUpcall")
.start();

int result = process.waitFor();
assertEquals(result, exitCode);
assertEquals(result, ERR_UNCAUGHT_EXCEPTION);
assertOutputContains(process.getErrorStream(), "Testing upcall exceptions");
}

private static void assertOutputContains(InputStream stream, String str) throws IOException {
try (BufferedReader reader = new BufferedReader(new InputStreamReader(stream))) {
String line;
while ((line = reader.readLine()) != null) {
if (line.contains(str)) {
return;
}
}
}
fail("Did not find '" + str + "' in stream");
}
}
@@ -49,24 +49,23 @@

try {
MH_throwException = MethodHandles.lookup().findStatic(ThrowingUpcall.class, "throwException",
MethodType.methodType(void.class, Throwable.class));
MethodType.methodType(void.class));
} catch (ReflectiveOperationException e) {
throw new ExceptionInInitializerError(e);
}
}

public static void throwException(Throwable t) throws Throwable {
throw t;
public static void throwException() throws Throwable {
throw new Throwable("Testing upcall exceptions");
}

public static void main(String[] args) throws Throwable {
test(new Throwable());
test();
}

public static void test(Throwable throwable) throws Throwable {
MethodHandle target = MethodHandles.insertArguments(MH_throwException, 0, throwable);
public static void test() throws Throwable {
try (ResourceScope scope = ResourceScope.newConfinedScope()) {
MemoryAddress stub = CLinker.getInstance().upcallStub(target, FunctionDescriptor.ofVoid(), scope);
MemoryAddress stub = CLinker.getInstance().upcallStub(MH_throwException, FunctionDescriptor.ofVoid(), scope);

downcall.invokeExact(stub); // should call System.exit(1);
}