Disable late attach to self by default #158

pdbain-ibm · 2017-09-27T14:41:07Z

This addresses openj9 issue #126
fixes #126

Prevent a JVM from late attaching to itself unless the system property
"jdk.attach.allowAttachSelf" is set either to "true" or an empty string, per
the reference implementation. Enforce this in Java 9 only. Update existing
tests to enable this feature by default so they will continue to run. Add new
Java 9 tests to exercise this feature.

Signed-off-by: Peter Bain [email protected]

pdbain-ibm · 2017-09-28T19:55:44Z

@DanHeidinga do you have any comments further to those in pull request #18?

DanHeidinga · 2017-09-28T18:40:58Z

jcl/src/java.base/share/classes/com/ibm/tools/attach/target/AttachHandler.java

+	 * but parse it lazily because we rarely need the value.
+	 */
+	public final static String allowAttachSelf = 
+			com.ibm.oti.vm.VM.getVMLangAccess().internalGetProperties().getProperty("jdk.attach.allowAttachSelf");  //$NON-NLS-1$


Is there a reason not to import com.ibm.oti.vm?

Historical reasons. Fixed.
Also cleaned up warnings on Java 5-era code.

DanHeidinga · 2017-09-28T18:43:56Z

test/Java9andUp/src/org/openj9/test/java9AttachAPI/TargetManager.java

+	 * @param vmArgs VM arguments
+	 * @param appArgs Application arguments
+	 */
+	public TargetManager(String cmdName, String targetId, String displayName,


For new code, please don't wrap at ~80 chars. The code is way more readable with longer lines then with arbitrary breaks to limit line length.

This code was cloned from the old Target manager, but fixed anyway.

DanHeidinga · 2017-09-28T18:47:08Z

test/Java9andUp/src/org/openj9/test/java9AttachAPI/TestAttachAPI.java

+ * This test must be invoked via testng.  Running the main method will return only the status of the attach API.
+ *
+ */
+@SuppressWarnings({"nls","boxing"})


Please address the NLS and boxing warnings.

Oops. Missed that one.

DanHeidinga · 2017-09-28T19:56:09Z

test/Java9andUp/src/org/openj9/test/java9AttachAPI/TargetManager.java

+	/**
+	 * Reads any available lines of text from a stream without blocking, 
+	 * except to find the terminating newline.
+	 * This assumes the producer emits complete lines 


ready() returns true as soon as a read() operation won't block - i.e.: a single character is available. This comment makes assumptions that aren't guaranteed by the Class libraries.

The usual way to do this is with separate threads that read from the buffered reader and append lines to some shared data structure.

Is there something I'm missing here?

Discussed deskside with Peter. This isn't new code per se, its moving old code to a new location. I'm willing to live it, especially as it's functioned for the past ~8 years.

We should take a hard look at some of this code when the Java 8 vs 9 vs next test code is rationalized.

For new code, I agree with the suggestion. However, as discussed, this is cloned legacy code which we have been running for ~9 years. Rewriting it now would add risk to this changeset.

I would be happy to re-do this code (and the Java8AndUp TargetManager) if you would open an issue.

pdbain-ibm · 2017-09-29T15:16:04Z

Changeset updated.

pdbain-ibm · 2017-10-04T14:08:58Z

Are any further changes required?

DanHeidinga · 2017-10-04T14:44:28Z

I'm at JavaOne this week and likely won't get a chance to finish the review until next week. I have no objections to one of the other committers completing the review / merge.

pdbain-ibm · 2017-10-16T12:39:42Z

Are any further changes required?

pdbain-ibm · 2017-10-31T19:54:50Z

@DanHeidinga Do you have any other comments on this pull request?

pshipton · 2017-11-09T19:32:27Z

jcl/src/java.base/share/classes/com/ibm/tools/attach/target/AttachHandler.java

@@ -50,7 +52,7 @@
 	/**
 	 * Time delay before we give up trying to terminate the wait loop.
 	 */
-	static final long shutdownTimeoutMs = Integer.getInteger("com.ibm.tools.attach.shutdown_timeout", 10000); //$NON-NLS-1$
+	static final long shutdownTimeoutMs = Integer.getInteger("com.ibm.tools.attach.shutdown_timeout", 10000).longValue(); //$NON-NLS-1$


I'm not sure of the point of this change.

It avoids a warning about auto-unboxing.

pshipton · 2017-11-09T19:33:53Z

test/Java9andUp/src/org/openj9/test/java9AttachAPI/SelfAttacher.java

@@ -0,0 +1,67 @@
+/*******************************************************************************
+ * Copyright (c) 2015, 2017 IBM Corp. and others


As this is new code the start date should be 2017

pshipton · 2017-11-09T19:34:07Z

test/Java9andUp/src/org/openj9/test/java9AttachAPI/TestAttachAPI.java

@@ -0,0 +1,103 @@
+/*******************************************************************************
+ * Copyright (c) 2001, 2017 IBM Corp. and others


As this is new code the start date should be 2017.

pshipton · 2017-11-09T19:34:44Z

test/Java9andUp/src_current/org/openj9/test/java9AttachAPI/VmIdGetter.java

@@ -0,0 +1,30 @@
+/* Copyright (c) 2015, 2017 IBM Corp. and others


As this is new code the start date should 2017.

pshipton · 2017-11-09T19:35:29Z

test/Java9andUp/src_latest/org/openj9/test/java9AttachAPI/VmIdGetter.java

@@ -0,0 +1,30 @@
+/* Copyright (c) 2015, 2017 IBM Corp. and others


As this is new code the start date should 2017.

pshipton · 2017-11-09T19:37:52Z

test/Java9andUp/src/org/openj9/test/java9AttachAPI/TargetManager.java

@@ -0,0 +1,412 @@
+/*******************************************************************************
+ * Copyright (c) 2001, 2017 IBM Corp. and others


As this is new code the start date should be 2017.

pshipton · 2017-11-09T19:46:45Z

test/Java9andUp/src/org/openj9/test/java9AttachAPI/SelfAttacher.java

+ * or the Apache License, Version 2.0 which accompanies this distribution and
+ * is available at https://www.apache.org/licenses/LICENSE-2.0.
+ *
+ * This Source Code is also Distributed under one or more Secondary Licenses,


These new files all have the incorrect license.

Fixed. Let me know if I missed any.

pshipton · 2017-11-09T19:54:04Z

test/Java9andUp/src/org/openj9/test/java9AttachAPI/TargetManager.java

+		logger.debug("javaExec="+javaExec);
+
+		/* These are the VM options to the parent process, which we wish to pass on to the child. */
+		String sideCar = System.getProperty("java.sidecar"); 


"java.sidecar" is obsolete.

Note that this is set by the playlist to indicate VM settings required by child processes (see test/Java8andUp/playlist.xml). Should I use a different system property?

Ok, I though it was a reference to something old and obsolete.

pshipton · 2017-11-09T20:00:24Z

jcl/src/java.base/share/classes/com/ibm/tools/attach/target/AttachHandler.java

@@ -102,10 +110,20 @@
 	private static String nameProperty;
 	private static String pidProperty;
 	private static int numberOfTargets;
+	/*[IF Sidecar19-SE]*/


Might be useful to add this code to Java 8 as well, but making the default true instead of false.

Good idea. Done

pshipton · 2017-11-10T19:56:40Z

jcl/src/java.base/share/classes/com/ibm/oti/util/ExternalMessages-MasterIndex.properties

@@ -1262,3 +1262,4 @@ K0644="Caller-sensitive method called StackWalker.getCallerClass()"
 #java.lang.ClassLoader
 K0645="The given class loader name can't be empty."

+K0646="Late attach connection to self disabled. Set jdk.attach.allowAttachSelf=true"


Please update the preprocessor definition to match.

pshipton · 2017-11-10T19:57:05Z

test/Java9andUp/src/org/openj9/test/java9AttachAPI/SelfAttacher.java

+/*******************************************************************************
+ * Copyright (c) 2017 IBM Corp. and others
+ *
+ * This program and the accompanying materials are made available under


Please update all the licenses to be the lastest version.

It appears the middle part of the licenses was updated, but not the ending part.

pshipton · 2017-11-10T19:57:20Z

test/Java9andUp/src/org/openj9/test/java9AttachAPI/SelfAttacher.java

@@ -0,0 +1,68 @@
+/*******************************************************************************
+ * Copyright (c) 2017 IBM Corp. and others


Please use 2017, 2017 in order to pass the copyright check.

pshipton · 2017-11-10T21:03:36Z

@llxia any concerns with the test code?

pdbain-ibm · 2017-11-13T14:25:11Z

@llxia or @sophia-guo Further to @pshipton 's comment above, is there a way I can share utility classes between the Java8AndUp and Java9AndUp projects? That would simplify this PR considerably. Thanks

pdbain-ibm · 2017-11-13T16:45:37Z

Copyrights and NLS message updated.

pdbain-ibm · 2017-11-14T15:19:44Z

Further to discussion with @smlambert et al, I will create a test utilities directory (under s separate pull request) to hold common code. When that is complete, this pull request will be updated to use utilities in the common directory.

pdbain-ibm · 2017-11-17T13:41:32Z

Pull request updated:

removed the duplicate TargetManager and used the common copy in TestUtilities
Renamed the test class and changed its package for compatibility with the Java 8 test code.
Otherwise it's the same.
@llxia could you please review the test code?

Thanks

smlambert · 2017-11-17T13:56:38Z

test/Java9andUp/src/org/openj9/test/attachAPI/SelfAttacher.java

+	public static void main(String[] args) {
+		try {
+			String myId = VmIdGetter.getVmId();
+			System.err.println("myId="+myId);


Please avoid stray system.out / system.err calls in test code and use logger instead.

The SelfAttacher is actually a child process. It uses stderr to communicate its process ID (and hence VM ID) to the target manager. I will add a comment to that effect.

smlambert · 2017-11-17T14:03:03Z

test/Java9andUp/src/org/openj9/test/attachAPI/SelfAttacher.java

+			e.printStackTrace();
+			System.exit(ATTACH_ERROR_CODE);
+		}
+		System.exit(ATTACH_SELF_API_SUCCEEDED_CODE);


Do not think the System.exit calls are appropriate above, assert is likely what you should be doing.

I use the exit code to indicate the result of the late attach attempt to the parent test process, i.e.

int status = target.waitFor(); assertEquals(status, ATTACH_SELF_API_SUCCEEDED_CODE, "wrong exit status"); Again, will add a comment.

I should have stepped back and viewed the entire suite of attach API tests to see. Comments will be useful thanks.

smlambert · 2017-11-17T14:04:12Z

test/Java9andUp/src/org/openj9/test/attachAPI/TestJava9AttachAPI.java

+	}
+
+	@Test
+	public void testAllowAttachSelfDisnable() {


Disable vs Disnable in method name

Oops. Thanks for catching that.

smlambert · 2017-11-17T14:13:53Z

test/Java9andUp/src/org/openj9/test/attachAPI/TestJava9AttachAPI.java

+				Arrays.asList(new String[] {ATTACH_ENABLE_PROPERTY,  ATTACH_SELF_ENABLE_PROPERTY+"=false"}), null);
+		try {
+			int status = target.waitFor();
+			assertEquals(status, ATTACH_SELF_IOEXCEPTION_CODE, "wrong exit status");


An opportunity for a better error message that wrong exit status (include exit status in the message would aid triage).

assertEquals() already does that, e.g.
wrong exit status expected [2] but found [1]

smlambert · 2017-11-17T14:24:40Z

test/Java9andUp/src_current/org/openj9/test/attachAPI/VmIdGetter.java

+ *******************************************************************************/
+package org.openj9.test.attachAPI;
+/* This class is separated from the main test because we need two
+ * copies to accommodate old and new versions of ProcessHandle


@pshipton src_current is for supporting old class library version, which I thought we were trying to move from, instead of build up more test/support for. Any guidance on this, why we keep adding more test material into this effort?

This needs to work on the vmfarm builds, which are stuck on an old (incompatible) class library.

This is adding a new test to IBM internal Java 9 b148 builds, correct? I agree its not necessary to add new testing. Existing testing either needs to be fixed or removed to keep the builds green. The b148 builds are obsolete and will be deleted once the equivalent testing is running on OpenJ9.

As discussed with the FV team, the tooling doesn't allow us to add the test only for the "latest" (i.e. post b148) JCL, so we will need to maintain 2 copies of some classes (as is done with the StackWalker tests) until we can kill b148.

llxia · 2017-11-17T14:47:26Z

test/Java9andUp/src/org/openj9/test/attachAPI/SelfAttacher.java

+	public static void main(String[] args) {
+		try {
+			String myId = VmIdGetter.getVmId();
+			System.err.println("myId="+myId);


Should we should logger here?

Good question, but no, see above.

llxia · 2017-11-17T14:47:40Z

test/Java9andUp/src_latest/org/openj9/test/attachAPI/VmIdGetter.java

+		return myId;
+	}
+
+}


We should put this file in test/Java9andUp/src to avoid duplication in both ssrc_latset and src_current.

Sadly no. the ProcessHandle API changed from getPid() to pid();

pdbain-ibm · 2017-11-17T15:43:54Z

Testing changes now. Stay tuned...

This addresses openj9 issue eclipse-openj9#126 Prevent a JVM from late attaching to itself unless the system property "jdk.attach.allowAttachSelf" is set either to "true" or an empty string, per the reference implementation. Enforce this in Java 9 only. Update existing tests to enable this feature by default so they will continue to run. Add new Java 9 tests to exercise this feature. Signed-off-by: Peter Bain <[email protected]>

pdbain-ibm · 2017-11-20T14:28:32Z

Personal build passed. @smlambert any more concerns?
Thanks

smlambert · 2017-11-20T17:49:16Z

Jenkins test extended

pdbain · 2017-11-21T02:21:18Z

failed tests due to known unrelated issue: org.openj9.test.vmArguments.VmArgumentTests.testCrNocr
#479

Restore legacy AttachAPI methods

pdbain-ibm changed the title ~~Disable late attach to self by default~~ WIP Disable late attach to self by default Sep 27, 2017

pdbain-ibm force-pushed the attach_self branch from 83ecb72 to 47ed307 Compare September 27, 2017 14:58

pdbain-ibm changed the title ~~WIP Disable late attach to self by default~~ Disable late attach to self by default Sep 27, 2017

pshipton added comp:vm jdk9 labels Sep 27, 2017

pshipton requested a review from DanHeidinga September 27, 2017 15:24

DanHeidinga requested changes Sep 28, 2017

View reviewed changes

pdbain-ibm force-pushed the attach_self branch 3 times, most recently from 8920243 to f490329 Compare September 29, 2017 15:15

pdbain-ibm force-pushed the attach_self branch from f490329 to 45c2066 Compare October 5, 2017 19:41

pshipton reviewed Nov 9, 2017

View reviewed changes

pdbain-ibm force-pushed the attach_self branch 2 times, most recently from d59dfed to db3c6cb Compare November 10, 2017 14:10

pshipton reviewed Nov 10, 2017

View reviewed changes

pshipton added the comp:test label Nov 10, 2017

DanHeidinga approved these changes Nov 13, 2017

View reviewed changes

pdbain-ibm force-pushed the attach_self branch from db3c6cb to fe52e1c Compare November 13, 2017 14:34

pshipton approved these changes Nov 13, 2017

View reviewed changes

pdbain-ibm force-pushed the attach_self branch from fe52e1c to 9f927e2 Compare November 17, 2017 02:46

smlambert self-requested a review November 17, 2017 13:54

smlambert requested changes Nov 17, 2017

View reviewed changes

llxia reviewed Nov 17, 2017

View reviewed changes

pdbain-ibm force-pushed the attach_self branch 5 times, most recently from adbcb58 to 5f3b64e Compare November 17, 2017 20:52

pdbain-ibm force-pushed the attach_self branch from 5f3b64e to 609fe3e Compare November 20, 2017 13:15

smlambert approved these changes Nov 20, 2017

View reviewed changes

smlambert merged commit b77c5b3 into eclipse-openj9:master Nov 21, 2017

tajila pushed a commit to tajila/openj9 that referenced this pull request Jun 17, 2019

Merge pull request eclipse-openj9#158 from Peter-Bain/ibm_sdk

0167f2b

Restore legacy AttachAPI methods

		@@ -0,0 +1,67 @@
		/*******************************************************************************
		* Copyright (c) 2015, 2017 IBM Corp. and others

		@@ -0,0 +1,103 @@
		/*******************************************************************************
		* Copyright (c) 2001, 2017 IBM Corp. and others

		@@ -0,0 +1,30 @@
		/* Copyright (c) 2015, 2017 IBM Corp. and others

		@@ -0,0 +1,412 @@
		/*******************************************************************************
		* Copyright (c) 2001, 2017 IBM Corp. and others

		@@ -0,0 +1,68 @@
		/*******************************************************************************
		* Copyright (c) 2017 IBM Corp. and others

Disable late attach to self by default #158

Disable late attach to self by default #158

Conversation

pdbain-ibm commented Sep 27, 2017 • edited by pshipton Loading

pdbain-ibm commented Sep 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdbain-ibm commented Sep 29, 2017

pdbain-ibm commented Oct 4, 2017

DanHeidinga commented Oct 4, 2017

pdbain-ibm commented Oct 16, 2017

pdbain-ibm commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshipton Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshipton commented Nov 10, 2017

pdbain-ibm commented Nov 13, 2017

pdbain-ibm commented Nov 13, 2017

pdbain-ibm commented Nov 14, 2017

pdbain-ibm commented Nov 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdbain-ibm commented Nov 17, 2017

pdbain-ibm commented Nov 20, 2017

smlambert commented Nov 20, 2017

pdbain commented Nov 21, 2017 • edited Loading

pdbain-ibm commented Sep 27, 2017 •

edited by pshipton

Loading

pshipton Nov 9, 2017 •

edited

Loading

pdbain commented Nov 21, 2017 •

edited

Loading