JDK-8234076 bug fix candidate

[Nikola Grcevski]

Hello core-libs-dev,

I'm a VM engineer at Microsoft and new to this mailing list. I took a look at JDK- 8234076 and the root cause is similar to a prior thread on a Windows launcher bug for JDK- 8231863, after the command line arguments are processed, the static variable firstAppArgIndex in src/java.base/share/native/libjli/args.c remains set to NOT_FOUND (-1). The reason for not finding a firstAppArgIndex in this case is because the command line supplied to the launcher has every argument prefixed with a dash, for example:

java --module-path=mods --module=com.greetings/com.greetings.Main --help

Because all arguments are prefixed with a dash, the checkArg function inside src/java.base/share/native/libjli/args.c never finds an app arg index.

Currently, this only causes an issue on Windows because of the custom argument expansion code in function CreateApplicationArgs  of src/java.base/windows/native/libjli/java_md.c. The code queries the firstAppArgIndex and proceeds to perform an array access without checking if the app index is greater or equal to 0, causing the randomly observed crash. 

I have made the following patch to address the issue:

--- a/src/java.base/windows/native/libjli/java_md.c
+++ b/src/java.base/windows/native/libjli/java_md.c
@@ -34,6 +34,7 @@
 #include <sys/stat.h>
 #include <wtypes.h>
 #include <commctrl.h>
+#include <assert.h>
 
 #include <jni.h>
 #include "java.h"
@@ -1015,6 +1016,17 @@ CreateApplicationArgs(JNIEnv *env, char **strv, int argc)
 
     // sanity check, match the args we have, to the holy grail
     idx = JLI_GetAppArgIndex();
+
+    // First arg index is NOT_FOUND
+    if (idx < 0) {
+        // The only allowed value should be NOT_FOUND (-1) unless another change introduces 
+        // a different negative index
+        assert (idx == -1);
+        JLI_TraceLauncher("Warning: first app arg index not found, %d\n", idx);
+        JLI_TraceLauncher("passing arguments as-is.\n");
+        return NewPlatformStringArray(env, strv, argc);
+    }
+
     isTool = (idx == 0);
     if (isTool) { idx++; } // skip tool name
     JLI_TraceLauncher("AppArgIndex: %d points to %s\n", idx, stdargs[idx].arg);

I wanted to ask for an advice and opinion if this is the correct way to fix the problem?

I did few tests and I can confirm that the fix above doesn't change the current behaviour on Windows, because in case the negative index array access doesn't cause a crash, the later sanity check below (in java_md.c CreateApplicationArgs ) will always catch the invalid case and simply pass down the arguments as is.

    // sanity check, ensure same number of arguments for application
    if (j != argc) {
        JLI_TraceLauncher("Warning: app args count doesn't match, %d %d\n", j, argc);
        JLI_TraceLauncher("passing arguments as-is.\n");
        JLI_MemFree(appArgIdx);
        return NewPlatformStringArray(env, strv, argc);
    }

However, if the command line is supplied with --module= instead of --module followed by a blank space, Windows will not perform argument expansion on the application arguments. For example:

java --module-path=mods --module=com.greetings/com.greetings.Main ./* --help (the path will not be expanded) 
java --module-path=mods --module com.greetings/com.greetings.Main ./* --help (the path will be correctly expanded)

I tested on Linux x86-64 and the argument expansion is correctly performed in both cases where --module= is used and where --module is followed by a space. Essentially, Windows behaves differently in this case, but it has always been this way. The patch only prevents the random crash, fixing the command line incompatibility will require a more substantial change. 

Please let me know if this approach is acceptable for the current bug report and I'll make a webrev and include appropriate launcher tests. I was thinking the tests should do extra validation on the output from _JAVA_LAUNCHER_DEBUG on Windows.

Thank you!
-Nikola