8225543: Jcmd fails to attach to the Java process on Linux using the main class name if whitespace options were used to launch the process

[Daniil Titov]

Hi Serguei,


>> How these changes are going to work for options "--module=" and "--module-path"
>> if there is this check at the beginning of each iteration:
>> parts[i].equals("--module")

Let me describe in details these cases you mentioned.

Case 1:
---------
   part[i] is "--module"
   part[i+1] is "myModule/MyClass"

Case 2:
---------
  part[i] is "--module=myModule/MyClass "

Case 3:
---------
   part[i] is "--module-path"
   part[i+1] is "mods"

For case 1 we have the condition on line 94 that is evaluated to TRUE and we return the next part on line 95  (part[i+1]) as a module and class name.

For case 2 the condition on line 94 is evaluated to FALSE so we continue to line 99. On line 99 the condition is evaluated to TRUE so on line 100 we 
return the value part of the option (everything after "--module=") as a module and class name. Please note the condition on line 94 uses 
equals() while on the line 99 it uses startsWith().

For case 3 the condition on lines 94 and 99 are evaluated to FALSE so we continue to lines 105/106 and  isModuleWhiteSpaceOption(parts[i]) on line 106
returns TRUE. As a result, we skip this and the next parts and continue to the beginning of the loop at line 92.





  92	        for (int i = 1; i < parts.length && mainClass == null; i++) {
    93	            if (i < parts.length - 1) {
    94	                if (parts[i].equals("-m") || parts[i].equals("--module") || parts[i].equals("-jar")) {
    95	                    return parts[i + 1];
    96	                }
    97	            }
    98	
    99	            if ((parts[i].startsWith("--module="))) {
   100	                return parts[i].substring("--module=".length());
   101	            }
   102	
   103	            // If this is a classpath or a module whitespace option then skip the next part
   104	            // (the classpath or the option value itself)
   105	            if (parts[i].equals("-cp") || parts[i].equals("-classpath") ||  parts[i].equals("--class-path") ||
   106	                    isModuleWhiteSpaceOption(parts[i])) {
   107	                i++;
   108	                continue;
   109	            }
   110	           


>> Also, this line has unneeded extra "()":  
>> +   if ((parts[i].startsWith("--module="))) {

Yes, you are right! Thanks! I will remove it before pushing or in the new version of a webrev if we will come to it.


Thank you!
-Daniil

From: "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
Date: Wednesday, June 12, 2019 at 12:39 PM
To: Daniil Titov <daniil.x.titov at oracle.com>, David Holmes <david.holmes at oracle.com>
Cc: OpenJDK Serviceability <serviceability-dev at openjdk.java.net>
Subject: Re: FW: 8225543: Jcmd fails to attach to the Java process on Linux using the main class name if whitespace options were used to launch the process

Hi Daniil,

I'm confused with the change:
         for (int i = 1; i < parts.length && mainClass == null; i++) {
             if (i < parts.length - 1) {
                 if (parts[i].equals("-m") || parts[i].equals("--module") || parts[i].equals("-jar")) {  <= ??????
                     return parts[i + 1];
                 }
             }
-            // If this is a classpath or a module path option then skip the next part
-            // (the classpath or the module path itself)
+
+            if ((parts[i].startsWith("--module="))) {                      <= ??????
+                return parts[i].substring("--module=".length());
+            }
+
+            // If this is a classpath or a module whitespace option then skip the next part
+            // (the classpath or the option value itself)
             if (parts[i].equals("-cp") || parts[i].equals("-classpath") ||  parts[i].equals("--class-path") ||
-                    parts[i].equals("-p") || parts[i].equals("--module-path")) {
+                    isModuleWhiteSpaceOption(parts[i])) {

. . .

+    private static boolean isModuleWhiteSpaceOption(String option) {
+        return option.equals("-p") ||
+                option.equals("--module-path") || 

How these changes are going to work for options "--module=" and "--module-path"
if there is this check at the beginning of each iteration:
  parts[i].equals("--module")
?

Also, this line has unneeded extra "()":
  
+            if ((parts[i].startsWith("--module="))) {


Thanks,
Serguei 

On 6/12/19 10:50, Daniil Titov wrote:
Hi David and Serguei,

Could you please have a look at this change? This fix is related with the changes [3] that you reviewed back in February.

Mach5 sun/tools/jcmd, serviceability/dcmd/framework, jdk_svc, hotspot_serviceability, tier1, tier2 and tier3 tests passed.

Thanks a lot!
--Daniil

On 6/10/19, 4:56 PM, "serviceability-dev on behalf of Daniil Titov" mailto:serviceability-dev-bounces at openjdk.java.netonbehalfofdaniil.x.titov@oracle.com wrote:

    Please review the change that fixes jcmd process name matching on Linux platform.
    
    After changes [3] , on Linux platform the proc filesystem is used to find a Java process, however, sun.tools.ProcessHelper  class, 
    introduced in that change, didn't take into account the presence of module whitespace options ( e.g. --patch-module) or the 
    fact that some values of Java options could contain whitespaces.
    
    The latter problem is fixed by keeping '\0' as a separator for command line arguments read from /proc/{pid}/cmdline rather than 
    replacing it with a whitespace. The former problem is fixed by making sun.tools.ProcessHelper  aware of these module whitespace
     options (--add-opens, --add-exports,--add-reads, --add-modules, --patch-module,--limit-modules, or --upgrade-module-path) as well
     as of the case when a source-file mode is used or when --module options is used in "--<name>=<value>" format.
    
    Testing: sun/tools/jcmd and serviceability/dcmd/framework tests succeeded in Mach5. jdk_svc, tier1, tier2, and tier3 tests are in progress. 
    
    
    [1] Webrev: http://cr.openjdk.java.net/~dtitov/8225543/webrev.01/ 
    [2] Bug: https://bugs.openjdk.java.net/browse/JDK-8225543 
    [3]  https://bugs.openjdk.java.net/browse/JDK-8205654 
    
    Thanks!
    --Daniil