8143952: JShell: space in class path causes remote launch failure
authorjlahoda
Wed, 16 Dec 2015 14:29:49 +0100
changeset 34755 135cde5b66cf
parent 34754 025d0b180221
child 34756 d31f11c4cc75
8143952: JShell: space in class path causes remote launch failure Summary: Simplification of handling of JDI connector arguments Reviewed-by: mcimadamore, rfield
langtools/src/jdk.jshell/share/classes/jdk/jshell/ExecutionControl.java
langtools/src/jdk.jshell/share/classes/jdk/jshell/JDIConnection.java
langtools/src/jdk.jshell/share/classes/jdk/jshell/JDIEnv.java
--- a/langtools/src/jdk.jshell/share/classes/jdk/jshell/ExecutionControl.java	Tue Dec 15 18:54:53 2015 -0800
+++ b/langtools/src/jdk.jshell/share/classes/jdk/jshell/ExecutionControl.java	Wed Dec 16 14:29:49 2015 +0100
@@ -33,6 +33,7 @@
 import java.net.Socket;
 import com.sun.jdi.*;
 import java.io.EOFException;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import jdk.jshell.ClassTracker.ClassInfo;
@@ -247,17 +248,18 @@
         //MessageOutput.textResources = ResourceBundle.getBundle("impl.TTYResources",
         //        Locale.getDefault());
 
-        String connect = "com.sun.jdi.CommandLineLaunch:";
-        String cmdLine = "jdk.internal.jshell.remote.RemoteAgent";
+        String connectorName = "com.sun.jdi.CommandLineLaunch";
         String classPath = System.getProperty("java.class.path");
         String bootclassPath = System.getProperty("sun.boot.class.path");
-        String javaArgs = "-classpath " + classPath + " -Xbootclasspath:" + bootclassPath;
+        String javaArgs = "-classpath \"" + classPath + "\" -Xbootclasspath:\"" + bootclassPath + "\"";
+        Map<String, String> argumentName2Value = new HashMap<>();
+        argumentName2Value.put("main", "jdk.internal.jshell.remote.RemoteAgent " + port);
+        argumentName2Value.put("options", javaArgs);
 
-        String connectSpec = connect + "main=" + cmdLine + " " + port + ",options=" + javaArgs + ",";
         boolean launchImmediately = true;
         int traceFlags = 0;// VirtualMachine.TRACE_SENDS | VirtualMachine.TRACE_EVENTS;
 
-        env.init(connectSpec, launchImmediately, traceFlags);
+        env.init(connectorName, argumentName2Value, launchImmediately, traceFlags);
 
         if (env.connection().isOpen() && env.vm().canBeModified()) {
             /*
--- a/langtools/src/jdk.jshell/share/classes/jdk/jshell/JDIConnection.java	Tue Dec 15 18:54:53 2015 -0800
+++ b/langtools/src/jdk.jshell/share/classes/jdk/jshell/JDIConnection.java	Wed Dec 16 14:29:49 2015 +0100
@@ -38,8 +38,9 @@
 import com.sun.jdi.connect.*;
 
 import java.util.*;
-import java.util.regex.*;
+import java.util.Map.Entry;
 import java.io.*;
+
 import static jdk.internal.jshell.debug.InternalDebugControl.DBG_GEN;
 
 /**
@@ -83,239 +84,35 @@
         return null;
     }
 
-    private Map <String, com.sun.jdi.connect.Connector.Argument> parseConnectorArgs(Connector connector, String argString) {
-        Map<String, com.sun.jdi.connect.Connector.Argument> arguments = connector.defaultArguments();
+    private Map <String, Connector.Argument> mergeConnectorArgs(Connector connector, Map<String, String> argumentName2Value) {
+        Map<String, Connector.Argument> arguments = connector.defaultArguments();
 
-        /*
-         * We are parsing strings of the form:
-         *    name1=value1,[name2=value2,...]
-         * However, the value1...valuen substrings may contain
-         * embedded comma(s), so make provision for quoting inside
-         * the value substrings. (Bug ID 4285874)
-         */
-        String regexPattern =
-            "(quote=[^,]+,)|" +           // special case for quote=.,
-            "(\\w+=)" +                   // name=
-            "(((\"[^\"]*\")|" +           //   ( "l , ue"
-            "('[^']*')|" +                //     'l , ue'
-            "([^,'\"]+))+,)";             //     v a l u e )+ ,
-        Pattern p = Pattern.compile(regexPattern);
-        Matcher m = p.matcher(argString);
-        while (m.find()) {
-            int startPosition = m.start();
-            int endPosition = m.end();
-            if (startPosition > 0) {
-                /*
-                 * It is an error if parsing skips over any part of argString.
-                 */
-                throw new IllegalArgumentException("Illegal connector argument" +
-                                          argString);
-            }
+        for (Entry<String, String> argumentEntry : argumentName2Value.entrySet()) {
+            String name = argumentEntry.getKey();
+            String value = argumentEntry.getValue();
+            Connector.Argument argument = arguments.get(name);
 
-            String token = argString.substring(startPosition, endPosition);
-            int index = token.indexOf('=');
-            String name = token.substring(0, index);
-            String value = token.substring(index + 1,
-                                           token.length() - 1); // Remove comma delimiter
-
-            /*
-             * for values enclosed in quotes (single and/or double quotes)
-             * strip off enclosing quote chars
-             * needed for quote enclosed delimited substrings
-             */
-            if (name.equals("options")) {
-                StringBuilder sb = new StringBuilder();
-                for (String s : splitStringAtNonEnclosedWhiteSpace(value)) {
-                    while (isEnclosed(s, "\"") || isEnclosed(s, "'")) {
-                        s = s.substring(1, s.length() - 1);
-                    }
-                    sb.append(s);
-                    sb.append(" ");
-                }
-                value = sb.toString();
-            }
-
-            Connector.Argument argument = arguments.get(name);
             if (argument == null) {
                 throw new IllegalArgumentException("Argument is not defined for connector:" +
                                           name + " -- " + connector.name());
             }
+
             argument.setValue(value);
-
-            argString = argString.substring(endPosition); // Remove what was just parsed...
-            m = p.matcher(argString);                     //    and parse again on what is left.
         }
-        if ((! argString.equals(",")) && (argString.length() > 0)) {
-            /*
-             * It is an error if any part of argString is left over,
-             * unless it was empty to begin with.
-             */
-            throw new IllegalArgumentException("Illegal connector argument" + argString);
-        }
+
         return arguments;
     }
 
-    private static boolean isEnclosed(String value, String enclosingChar) {
-        if (value.indexOf(enclosingChar) == 0) {
-            int lastIndex = value.lastIndexOf(enclosingChar);
-            if (lastIndex > 0 && lastIndex  == value.length() - 1) {
-                return true;
-            }
-        }
-        return false;
-    }
+    JDIConnection(JDIEnv env, String connectorName, Map<String, String> argumentName2Value, int traceFlags, JShell proc) {
+        this.env = env;
+        this.proc = proc;
+        this.connector = findConnector(connectorName);
 
-    private static List<String> splitStringAtNonEnclosedWhiteSpace(String value) throws IllegalArgumentException {
-        List<String> al = new ArrayList<>();
-        char[] arr;
-        int startPosition = 0;
-        int endPosition;
-        final char SPACE = ' ';
-        final char DOUBLEQ = '"';
-        final char SINGLEQ = '\'';
-
-        /*
-         * An "open" or "active" enclosing state is where
-         * the first valid start quote qualifier is found,
-         * and there is a search in progress for the
-         * relevant end matching quote
-         *
-         * enclosingTargetChar set to SPACE
-         * is used to signal a non open enclosing state
-         */
-        char enclosingTargetChar = SPACE;
-
-        if (value == null) {
-            throw new IllegalArgumentException("value string is null");
+        if (connector == null) {
+            throw new IllegalArgumentException("No connector named: " + connectorName);
         }
 
-        // split parameter string into individual chars
-        arr = value.toCharArray();
-
-        for (int i = 0; i < arr.length; i++) {
-            switch (arr[i]) {
-                case SPACE: {
-                    // do nothing for spaces
-                    // unless last in array
-                    if (isLastChar(arr, i)) {
-                        endPosition = i;
-                        // break for substring creation
-                        break;
-                    }
-                    continue;
-                }
-                case DOUBLEQ:
-                case SINGLEQ: {
-                    if (enclosingTargetChar == arr[i]) {
-                        // potential match to close open enclosing
-                        if (isNextCharWhitespace(arr, i)) {
-                            // if peek next is whitespace
-                            // then enclosing is a valid substring
-                            endPosition = i;
-                            // reset enclosing target char
-                            enclosingTargetChar = SPACE;
-                            // break for substring creation
-                            break;
-                        }
-                    }
-                    if (enclosingTargetChar == SPACE) {
-                        // no open enclosing state
-                        // handle as normal char
-                        if (isPreviousCharWhitespace(arr, i)) {
-                            startPosition = i;
-                            // peek forward for end candidates
-                            if (value.indexOf(arr[i], i + 1) >= 0) {
-                                // set open enclosing state by
-                                // setting up the target char
-                                enclosingTargetChar = arr[i];
-                            } else {
-                                // no more target chars left to match
-                                // end enclosing, handle as normal char
-                                if (isNextCharWhitespace(arr, i)) {
-                                    endPosition = i;
-                                    // break for substring creation
-                                    break;
-                                }
-                            }
-                        }
-                    }
-                    continue;
-                }
-                default: {
-                    // normal non-space, non-" and non-' chars
-                    if (enclosingTargetChar == SPACE) {
-                        // no open enclosing state
-                        if (isPreviousCharWhitespace(arr, i)) {
-                            // start of space delim substring
-                            startPosition = i;
-                        }
-                        if (isNextCharWhitespace(arr, i)) {
-                            // end of space delim substring
-                            endPosition = i;
-                            // break for substring creation
-                            break;
-                        }
-                    }
-                    continue;
-                }
-            }
-
-            // break's end up here
-            if (startPosition > endPosition) {
-                throw new IllegalArgumentException("Illegal option values");
-            }
-
-            // extract substring and add to List<String>
-            al.add(value.substring(startPosition, ++endPosition));
-
-            // set new start position
-            i = startPosition = endPosition;
-
-        } // for loop
-
-        return al;
-    }
-
-    static private boolean isPreviousCharWhitespace(char[] arr, int curr_pos) {
-        return isCharWhitespace(arr, curr_pos - 1);
-    }
-
-    static private boolean isNextCharWhitespace(char[] arr, int curr_pos) {
-        return isCharWhitespace(arr, curr_pos + 1);
-    }
-
-    static private boolean isCharWhitespace(char[] arr, int pos) {
-        if (pos < 0 || pos >= arr.length) {
-            // outside arraybounds is considered an implicit space
-            return true;
-        }
-        return (arr[pos] == ' ');
-    }
-
-    static private boolean isLastChar(char[] arr, int pos) {
-        return (pos + 1 == arr.length);
-    }
-
-    JDIConnection(JDIEnv env, String connectSpec, int traceFlags, JShell proc) {
-        this.env = env;
-        this.proc = proc;
-        String nameString;
-        String argString;
-        int index = connectSpec.indexOf(':');
-        if (index == -1) {
-            nameString = connectSpec;
-            argString = "";
-        } else {
-            nameString = connectSpec.substring(0, index);
-            argString = connectSpec.substring(index + 1);
-        }
-
-        connector = findConnector(nameString);
-        if (connector == null) {
-            throw new IllegalArgumentException("No connector named: " + nameString);
-        }
-
-        connectorArgs = parseConnectorArgs(connector, argString);
+        connectorArgs = mergeConnectorArgs(connector, argumentName2Value);
         this.traceFlags = traceFlags;
     }
 
--- a/langtools/src/jdk.jshell/share/classes/jdk/jshell/JDIEnv.java	Tue Dec 15 18:54:53 2015 -0800
+++ b/langtools/src/jdk.jshell/share/classes/jdk/jshell/JDIEnv.java	Wed Dec 16 14:29:49 2015 +0100
@@ -25,6 +25,8 @@
 
 package jdk.jshell;
 
+import java.util.Map;
+
 import com.sun.jdi.*;
 import static jdk.internal.jshell.debug.InternalDebugControl.DBG_GEN;
 
@@ -41,8 +43,8 @@
         this.state = state;
     }
 
-    void init(String connectSpec, boolean openNow, int flags) {
-        connection = new JDIConnection(this, connectSpec, flags, state);
+    void init(String connectorName, Map<String, String> argumentName2Value, boolean openNow, int flags) {
+        connection = new JDIConnection(this, connectorName, argumentName2Value, flags, state);
         if (!connection.isLaunch() || openNow) {
             connection.open();
         }