[GR-37635] Fix handling of isolate and JNI arguments.

christianhaeubl · christianhaeubl · commit abd8fa1f9f5d · 2022-03-29T20:48:43.000Z
PullRequest: graal/11439
diff --git a/espresso/src/com.oracle.truffle.espresso.mokapot/include/graal_isolate_dynamic.h b/espresso/src/com.oracle.truffle.espresso.mokapot/include/graal_isolate_dynamic.h
@@ -54,7 +54,7 @@ typedef unsigned long __graal_uword;
 #define NEW_PROTECTION_DOMAIN -1
 
 /* Parameters for the creation of a new isolate. */
-enum { __graal_create_isolate_params_version = 1 };
+enum { __graal_create_isolate_params_version = 4 };
 struct __graal_create_isolate_params_t {
     int version;                                /* Version of this struct */
 
@@ -69,6 +69,10 @@ struct __graal_create_isolate_params_t {
     int            _reserved_1;                 /* Internal usage, do not use. */
     char         **_reserved_2;                 /* Internal usage, do not use. */
     int            pkey;                        /* Isolate protection key or domain. */
+
+    /* Fields introduced in version 4 */
+    char           _reserved_3;                 /* Internal usage, do not use. */
+    char           _reserved_4;                 /* Internal usage, do not use. */
 };
 typedef struct __graal_create_isolate_params_t graal_create_isolate_params_t;
 
diff --git a/substratevm/src/com.oracle.svm.core.posix/src/com/oracle/svm/core/posix/PosixLibCSupport.java b/substratevm/src/com.oracle.svm.core.posix/src/com/oracle/svm/core/posix/PosixLibCSupport.java
@@ -89,6 +89,12 @@ public UnsignedWord strlen(CCharPointer str) {
         return PosixLibC.strlen(str);
     }
 
+    @Override
+    @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
+    public int strcmp(CCharPointer s1, CCharPointer s2) {
+        return PosixLibC.strcmp(s1, s2);
+    }
+
     @Override
     @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
     public int isdigit(int c) {
diff --git a/substratevm/src/com.oracle.svm.core.windows/src/com/oracle/svm/core/windows/WindowsLibCSupport.java b/substratevm/src/com.oracle.svm.core.windows/src/com/oracle/svm/core/windows/WindowsLibCSupport.java
@@ -101,6 +101,12 @@ public UnsignedWord strlen(CCharPointer str) {
         return WindowsLibC.strlen(str);
     }
 
+    @Override
+    @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
+    public int strcmp(CCharPointer s1, CCharPointer s2) {
+        return WindowsLibC.strcmp(s1, s2);
+    }
+
     @Override
     @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
     public int isdigit(int c) {
diff --git a/substratevm/src/com.oracle.svm.core/headers/graal_isolate.preamble b/substratevm/src/com.oracle.svm.core/headers/graal_isolate.preamble
@@ -29,7 +29,7 @@ typedef unsigned long __graal_uword;
 #define NEW_PROTECTION_DOMAIN -1
 
 /* Parameters for the creation of a new isolate. */
-enum { __graal_create_isolate_params_version = 1 };
+enum { __graal_create_isolate_params_version = 4 };
 struct __graal_create_isolate_params_t {
     int version;                                /* Version of this struct */
 
@@ -44,5 +44,9 @@ struct __graal_create_isolate_params_t {
     int            _reserved_1;                 /* Internal usage, do not use. */
     char         **_reserved_2;                 /* Internal usage, do not use. */
     int            pkey;                        /* Isolate protection key or domain. */
+
+    /* Fields introduced in version 4 */
+    char           _reserved_3;                 /* Internal usage, do not use. */
+    char           _reserved_4;                 /* Internal usage, do not use. */
 };
 typedef struct __graal_create_isolate_params_t graal_create_isolate_params_t;
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/IsolateArgumentParser.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/IsolateArgumentParser.java
@@ -155,7 +155,8 @@ public static void parse(CEntryPointCreateIsolateParameters parameters, CLongPoi
         }
 
         CLongPointer numericValue = StackValue.get(Long.BYTES);
-        for (int i = 0; i < argc; i++) {
+        // Ignore the first argument as it represents the executable file name.
+        for (int i = 1; i < argc; i++) {
             CCharPointer arg = argv.read(i);
             if (arg.isNonNull()) {
                 CCharPointer tail = matchPrefix(arg);
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/JavaMainWrapper.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/JavaMainWrapper.java
@@ -107,7 +107,7 @@ public String getJavaCommand() {
         public List<String> getInputArguments() {
             CEntryPointCreateIsolateParameters args = MAIN_ISOLATE_PARAMETERS.get();
             if (args.getArgv().isNonNull() && args.getArgc() > 0) {
-                String[] unmodifiedArgs = SubstrateUtil.getArgs(args.getArgc(), args.getArgv());
+                String[] unmodifiedArgs = SubstrateUtil.convertCToJavaArgs(args.getArgc(), args.getArgv());
                 List<String> inputArgs = new ArrayList<>(Arrays.asList(unmodifiedArgs));
 
                 if (mainArgs != null) {
@@ -271,9 +271,11 @@ private static class EnterCreateIsolateWithCArgumentsPrologue implements CEntryP
         @Uninterruptible(reason = "prologue")
         public static void enter(int paramArgc, CCharPointerPointer paramArgv) {
             CEntryPointCreateIsolateParameters args = MAIN_ISOLATE_PARAMETERS.get();
-            args.setVersion(3);
+            args.setVersion(4);
             args.setArgc(paramArgc);
             args.setArgv(paramArgv);
+            args.setIgnoreUnrecognizedArguments(false);
+            args.setExitWhenArgumentParsingFails(true);
 
             int code = CEntryPointActions.enterCreateIsolate(args);
             if (code != CEntryPointErrors.NO_ERROR) {
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/SubstrateUtil.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/SubstrateUtil.java
@@ -161,7 +161,7 @@ public static FileDescriptor getFileDescriptor(FileOutputStream out) {
      *
      * @return the command line argument strings in a Java string array.
      */
-    public static String[] getArgs(int argc, CCharPointerPointer argv) {
+    public static String[] convertCToJavaArgs(int argc, CCharPointerPointer argv) {
         String[] args = new String[argc - 1];
         for (int i = 1; i < argc; ++i) {
             args[i - 1] = CTypeConversion.toJavaString(argv.read(i));
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/c/function/CEntryPointCreateIsolateParameters.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/c/function/CEntryPointCreateIsolateParameters.java
@@ -90,4 +90,17 @@ public interface CEntryPointCreateIsolateParameters extends PointerBase {
 
     @CField("_reserved_2")
     void setArgv(CCharPointerPointer value);
+
+    /* fields below: version 4 */
+    @CField("_reserved_3")
+    boolean getIgnoreUnrecognizedArguments();
+
+    @CField("_reserved_3")
+    void setIgnoreUnrecognizedArguments(boolean value);
+
+    @CField("_reserved_4")
+    boolean getExitWhenArgumentParsingFails();
+
+    @CField("_reserved_4")
+    void setExitWhenArgumentParsingFails(boolean value);
 }
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/c/function/CEntryPointErrors.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/c/function/CEntryPointErrors.java
@@ -119,6 +119,9 @@ private CEntryPointErrors() {
     @Description("The auxiliary image was built from a different primary image.") //
     public static final int AUX_IMAGE_PRIMARY_IMAGE_MISMATCH = 21;
 
+    @Description("The isolate arguments could not be parsed.") //
+    public static final int ARGUMENT_PARSING_FAILED = 22;
+
     public static String getDescription(int code) {
         String result = null;
         if (code >= 0 && code < DESCRIPTIONS.length) {
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/c/function/IsolateSupportImpl.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/c/function/IsolateSupportImpl.java
@@ -71,10 +71,11 @@ public IsolateThread createIsolate(CreateIsolateParameters parameters) throws Is
             params.setReservedSpaceSize(parameters.getReservedAddressSpaceSize());
             params.setAuxiliaryImagePath(auxImagePath.get());
             params.setAuxiliaryImageReservedSpaceSize(parameters.getAuxiliaryImageReservedSpaceSize());
-            params.setVersion(3);
+            params.setVersion(4);
+            params.setIgnoreUnrecognizedArguments(false);
+            params.setExitWhenArgumentParsingFails(false);
 
             if (MemoryProtectionProvider.isAvailable()) {
-
                 try {
                     int pkey = MemoryProtectionProvider.singleton().asProtectionKey(parameters.getProtectionDomain());
                     params.setProtectionKey(pkey);
@@ -86,21 +87,28 @@ public IsolateThread createIsolate(CreateIsolateParameters parameters) throws Is
             }
 
             // Prepare argc and argv.
-            List<String> args = parameters.getArguments();
-            int argc = args.size();
-            params.setArgc(argc);
-            params.setArgv(WordFactory.nullPointer());
+            int argc = 0;
+            CCharPointerPointer argv = WordFactory.nullPointer();
 
+            List<String> args = parameters.getArguments();
             CTypeConversion.CCharPointerHolder[] pointerHolders = null;
-            if (argc > 0) {
-                CCharPointerPointer argv = UnmanagedMemory.malloc(SizeOf.unsigned(CCharPointerPointer.class).multiply(argc));
-                pointerHolders = new CTypeConversion.CCharPointerHolder[argc];
-                for (int i = 0; i < argc; i++) {
+            if (!args.isEmpty()) {
+                int isolateArgCount = args.size();
+
+                // Internally, we use C-style arguments, i.e., the first argument is reserved for
+                // the name of the binary. We use null when isolates are created manually.
+                argc = isolateArgCount + 1;
+                argv = UnmanagedMemory.malloc(SizeOf.unsigned(CCharPointerPointer.class).multiply(argc));
+                argv.write(0, WordFactory.nullPointer());
+
+                pointerHolders = new CTypeConversion.CCharPointerHolder[isolateArgCount];
+                for (int i = 0; i < isolateArgCount; i++) {
                     CTypeConversion.CCharPointerHolder ph = pointerHolders[i] = CTypeConversion.toCString(args.get(i));
-                    argv.write(i, ph.get());
+                    argv.write(i + 1, ph.get());
                 }
-                params.setArgv(argv);
             }
+            params.setArgc(argc);
+            params.setArgv(argv);
 
             // Try to create the isolate.
             IsolateThreadPointer isolateThreadPtr = StackValue.get(IsolateThreadPointer.class);
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/graal/snippets/CEntryPointSnippets.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/graal/snippets/CEntryPointSnippets.java
@@ -292,8 +292,26 @@ private static int initializeIsolate(CEntryPointCreateIsolateParameters paramete
          */
 
         if (parameters.isNonNull() && parameters.version() >= 3 && parameters.getArgv().isNonNull()) {
-            String[] args = SubstrateUtil.getArgs(parameters.getArgc(), parameters.getArgv());
-            args = RuntimeOptionParser.parseAndConsumeAllOptions(args, false);
+            boolean exitWhenArgumentParsingFails = true;
+            boolean ignoreUnrecognized = false;
+            if (parameters.version() >= 4) {
+                ignoreUnrecognized = parameters.getIgnoreUnrecognizedArguments();
+                exitWhenArgumentParsingFails = parameters.getExitWhenArgumentParsingFails();
+            }
+
+            String[] args = SubstrateUtil.convertCToJavaArgs(parameters.getArgc(), parameters.getArgv());
+            try {
+                args = RuntimeOptionParser.parseAndConsumeAllOptions(args, ignoreUnrecognized);
+            } catch (IllegalArgumentException e) {
+                if (exitWhenArgumentParsingFails) {
+                    Log.logStream().println("error: " + e.getMessage());
+                    System.exit(1);
+                } else {
+                    CEntryPointActions.leaveTearDownIsolate();
+                    return CEntryPointErrors.ARGUMENT_PARSING_FAILED;
+                }
+            }
+
             if (ImageSingletons.contains(JavaMainSupport.class)) {
                 ImageSingletons.lookup(JavaMainSupport.class).mainArgs = args;
             }
@@ -306,6 +324,7 @@ private static int initializeIsolate(CEntryPointCreateIsolateParameters paramete
         }
 
         if (!success) {
+            CEntryPointActions.leaveTearDownIsolate();
             return CEntryPointErrors.ISOLATE_INITIALIZATION_FAILED;
         }
 
@@ -320,6 +339,7 @@ private static int initializeIsolate(CEntryPointCreateIsolateParameters paramete
         } catch (Throwable t) {
             System.err.println("Uncaught exception while running initialization hooks:");
             t.printStackTrace();
+            CEntryPointActions.leaveTearDownIsolate();
             return CEntryPointErrors.ISOLATE_INITIALIZATION_FAILED;
         }
 
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/headers/LibC.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/headers/LibC.java
@@ -102,6 +102,11 @@ public static UnsignedWord strlen(CCharPointer str) {
         return libc().strlen(str);
     }
 
+    @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
+    public static int strcmp(CCharPointer s1, CCharPointer s2) {
+        return libc().strcmp(s1, s2);
+    }
+
     @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
     public static int isdigit(int c) {
         return libc().isdigit(c);
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/headers/LibCSupport.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/headers/LibCSupport.java
@@ -66,6 +66,9 @@ public interface LibCSupport {
     @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
     UnsignedWord strlen(CCharPointer str);
 
+    @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
+    int strcmp(CCharPointer s1, CCharPointer s2);
+
     @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
     int isdigit(int c);
 
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/log/FunctionPointerLogHandler.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/log/FunctionPointerLogHandler.java
@@ -25,12 +25,17 @@
 package com.oracle.svm.core.log;
 
 import com.oracle.svm.core.annotate.RestrictHeapAccess;
+import com.oracle.svm.core.annotate.Uninterruptible;
+import com.oracle.svm.core.c.CGlobalData;
+import com.oracle.svm.core.c.CGlobalDataFactory;
+import com.oracle.svm.core.headers.LibC;
 import org.graalvm.nativeimage.ImageSingletons;
 import org.graalvm.nativeimage.LogHandler;
 import org.graalvm.nativeimage.c.function.CFunctionPointer;
 import org.graalvm.nativeimage.c.function.CodePointer;
 import org.graalvm.nativeimage.c.function.InvokeCFunctionPointer;
 import org.graalvm.nativeimage.c.type.CCharPointer;
+import org.graalvm.nativeimage.c.type.CTypeConversion;
 import org.graalvm.nativeimage.c.type.WordPointer;
 import org.graalvm.word.UnsignedWord;
 
@@ -39,6 +44,10 @@
  * pointer is missing, it forwards the operation to the delegate set in the constructor.
  */
 public class FunctionPointerLogHandler implements LogHandlerExtension {
+    private static final CGlobalData<CCharPointer> LOG_OPTION = CGlobalDataFactory.createCString("_log");
+    private static final CGlobalData<CCharPointer> FATAL_LOG_OPTION = CGlobalDataFactory.createCString("_fatal_log");
+    private static final CGlobalData<CCharPointer> FLUSH_LOG_OPTION = CGlobalDataFactory.createCString("_flush_log");
+    private static final CGlobalData<CCharPointer> FATAL_OPTION = CGlobalDataFactory.createCString("_fatal");
 
     private final LogHandler delegate;
 
@@ -132,42 +141,51 @@ interface FatalContextFunctionPointer extends CFunctionPointer {
         boolean invoke(CodePointer callerIP, String msg, Throwable ex);
     }
 
+    @Uninterruptible(reason = "Called from uninterruptible code", mayBeInlined = true)
+    public static boolean isJniVMOption(CCharPointer optionString) {
+        return LibC.strcmp(optionString, LOG_OPTION.get()) == 0 ||
+                        LibC.strcmp(optionString, FATAL_LOG_OPTION.get()) == 0 ||
+                        LibC.strcmp(optionString, FLUSH_LOG_OPTION.get()) == 0 ||
+                        LibC.strcmp(optionString, FATAL_OPTION.get()) == 0;
+    }
+
     /**
      * Parses a {@code JavaVMOption} passed to {@code JNI_CreateJavaVM}.
      *
      * @param optionString value of the {@code javaVMOption.optionString} field
      * @param extraInfo value of the {@code javaVMOption.extraInfo} field
      * @return {@code true} iff the option was consumed by this method
      */
-    public static boolean parseVMOption(String optionString, WordPointer extraInfo) {
-        if (optionString.equals("_log")) {
+    public static boolean parseJniVMOption(CCharPointer optionString, WordPointer extraInfo) {
+        if (LibC.strcmp(optionString, LOG_OPTION.get()) == 0) {
             handler(optionString).logFunctionPointer = (LogFunctionPointer) extraInfo;
             return true;
-        } else if (optionString.equals("_fatal_log")) {
+        } else if (LibC.strcmp(optionString, FATAL_LOG_OPTION.get()) == 0) {
             handler(optionString).fatalLogFunctionPointer = (LogFunctionPointer) extraInfo;
             return true;
-        } else if (optionString.equals("_flush_log")) {
+        } else if (LibC.strcmp(optionString, FLUSH_LOG_OPTION.get()) == 0) {
             handler(optionString).flushFunctionPointer = (VoidFunctionPointer) extraInfo;
             return true;
-        } else if (optionString.equals("_fatal")) {
+        } else if (LibC.strcmp(optionString, FATAL_OPTION.get()) == 0) {
             handler(optionString).fatalErrorFunctionPointer = (VoidFunctionPointer) extraInfo;
             return true;
         }
         return false;
     }
 
-    private static FunctionPointerLogHandler handler(String optionString) {
+    private static FunctionPointerLogHandler handler(CCharPointer optionString) {
         LogHandler handler = ImageSingletons.lookup(LogHandler.class);
         if (handler == null || !(handler instanceof FunctionPointerLogHandler)) {
-            throw new IllegalArgumentException("The " + optionString + " option is not supported by JNI_CreateJavaVM");
+            String str = CTypeConversion.toJavaString(optionString);
+            throw new IllegalArgumentException("The " + str + " option is not supported by JNI_CreateJavaVM");
         }
         return (FunctionPointerLogHandler) handler;
     }
 
     /**
      * Notifies that {@code JNI_CreateJavaVM} has finished parsing all {@code JavaVMOption}s.
      */
-    public static void afterParsingVMOptions() {
+    public static void afterParsingJniVMOptions() {
         LogHandler handler = ImageSingletons.lookup(LogHandler.class);
         if (handler == null || !(handler instanceof FunctionPointerLogHandler)) {
             return;
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/option/RuntimeOptionParser.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/option/RuntimeOptionParser.java
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/option/XOptions.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/option/XOptions.java
diff --git a/substratevm/src/com.oracle.svm.jni/src/com/oracle/svm/jni/functions/JNIInvocationInterface.java b/substratevm/src/com.oracle.svm.jni/src/com/oracle/svm/jni/functions/JNIInvocationInterface.java

Original file line number	Diff line number	Diff line change
`@@ -155,7 +155,8 @@ public static void parse(CEntryPointCreateIsolateParameters parameters, CLongPoi`
`155`	`155`	`}`
`156`	`156`
`157`	`157`	`CLongPointer numericValue = StackValue.get(Long.BYTES);`
`158`		`- for (int i = 0; i < argc; i++) {`
	`158`	`+ // Ignore the first argument as it represents the executable file name.`
	`159`	`+ for (int i = 1; i < argc; i++) {`
`159`	`160`	`CCharPointer arg = argv.read(i);`
`160`	`161`	`if (arg.isNonNull()) {`
`161`	`162`	`CCharPointer tail = matchPrefix(arg);`
Original file line number	Diff line number	Diff line change
`@@ -161,7 +161,7 @@ public static FileDescriptor getFileDescriptor(FileOutputStream out) {`
`161`	`161`	`*`
`162`	`162`	`* @return the command line argument strings in a Java string array.`
`163`	`163`	`*/`
`164`		`- public static String[] getArgs(int argc, CCharPointerPointer argv) {`
	`164`	`+ public static String[] convertCToJavaArgs(int argc, CCharPointerPointer argv) {`
`165`	`165`	`String[] args = new String[argc - 1];`
`166`	`166`	`for (int i = 1; i < argc; ++i) {`
`167`	`167`	`args[i - 1] = CTypeConversion.toJavaString(argv.read(i));`