Re: RFR: 8238196: tests that use SA Attach should not be allowed to run against signed binaries on Mac OS X 10.14.5 and later

Wed, 12 Feb 2020 14:16:07 -0800 

On 13/02/2020 5:02 am, Chris Plummer wrote:

Hi David,
What you have below is a mix of #2 and #3 - you convert to a generic exception but also re-assert the interrupt state. That's a little unusual.
That what I also thought which is why I was suggesting not doing the interrupt() call and only throwing the RuntimeException. I agree that doing both does not make sense, and in general doing (3) does not make sense if the caller is not setup to properly check the interrupt state.
From a library writer perspective you should have zero knowledge of the caller and doing (3) makes perfect sense. Remember that you will only re-assert the interrupt() if you get the InterruptedException in the first place, which means that some other code already issued the initial interrupt().
Whether this code shoud be treated as general purpose library code is another matter.
Personally, in this case I think I'd use (2) and make interruption a failure mode. 

David


Chris


On 2/12/20 5:58 AM, David Holmes wrote:

Hi Chris,

I think you are overthinking this. :)
What you have observed is that the code that actually uses this method does not utilise interrupts, or expect them, so if you artifically inject one in this library method then you see things failing in unexpected ways. That also means that if the thread was interrupted by some other piece of logic then it would also fail in unexpected ways. That doesn't negate your choice to re-assert the interrupt state.
From a library writing perspective if you have a method that performs a blocking call that can throw InterruptedException then you generally have three choices: 

1. Throw InterruptedException yourself and pass the buck to your callers.
2. Convert the InterruptedException to a more general failure exception - typically an unchecked RuntimeException - for which interruption is but one possible cause; or 3. Catch the InterruptedException and allow the method to complete normally (i.e. not by throwing an exception) but re-assert the interrupt state so that a caller checking for interruption will still see that it occurred.
What you have below is a mix of #2 and #3 - you convert to a generic exception but also re-assert the interrupt state. That's a little unusual. 

David


On 12/02/2020 6:16 pm, Chris Plummer wrote:

Hi Igor,
I think it might be best to the interrupt() call out. I wanted to see what would happen if we ever got an InterruptedException, so I added the following to the start of Platform.shouldSAAttach(): 

         try {
             throw new InterruptedException();
         } catch (InterruptedException e) {
             Thread.currentThread().interrupt();
             throw new RuntimeException(e);
         }
At the start of the test run, before any tests are actually run, I see the following: 

failed to get value for vm.hasSAandCanAttach
java.lang.RuntimeException: java.lang.InterruptedException
     at jdk.test.lib.Platform.shouldSAAttach(Platform.java:300)
     at requires.VMProps.vmHasSAandCanAttach(VMProps.java:327)
     at requires.VMProps$SafeMap.put(VMProps.java:69)
     at requires.VMProps.call(VMProps.java:101)
     at requires.VMProps.call(VMProps.java:57)
     at com.sun.javatest.regtest.agent.GetJDKProperties.run(GetJDKProperties.java:80)      at com.sun.javatest.regtest.agent.GetJDKProperties.main(GetJDKProperties.java:54) 
Caused by: java.lang.InterruptedException
     at jdk.test.lib.Platform.shouldSAAttach(Platform.java:297)
     ... 6 more

This seems reasonable.

For each test that checks vm.hasSAandCanAttach I also see.
TEST RESULT: Error. Error evaluating expression: vm.hasSAandCanAttach: java.lang.RuntimeException: java.lang.InterruptedException 

This too seems reasonable.
For tests that don't check vm.hasSAandCanAttach, but instead make a runtime check that calls Platform.shouldSAAttach(), the test fails with: 

java.lang.IllegalThreadStateException: process hasn't exited
     at java.base/java.lang.ProcessImpl.exitValue(ProcessImpl.java:500)
     at jdk.test.lib.apps.LingeredApp.stopApp(LingeredApp.java:380)
     at jdk.test.lib.apps.LingeredApp.stopApp(LingeredApp.java:433)
     at ClhsdbAttach.main(ClhsdbAttach.java:77)
     at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)      at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)      at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) 
     at java.base/java.lang.reflect.Method.invoke(Method.java:564)
     at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127) 
     at java.base/java.lang.Thread.run(Thread.java:832)
This is a confusing way to fail. The reason it fails this way is because stopApp() first calls waitAppTerminiate(), which does the following: 

     public void waitAppTerminate() {
         // This code is modeled after tail end of ProcessTools.getOutput(). 
         try {
             appProcess.waitFor();
             outPumperThread.join();
             errPumperThread.join();
         } catch (InterruptedException e) {
             Thread.currentThread().interrupt();
             // pass
         }
     }

I added an e.printStackTrace() call and see the following:

java.lang.InterruptedException
     at java.base/java.lang.Object.wait(Native Method)
     at java.base/java.lang.Object.wait(Object.java:321)
     at java.base/java.lang.ProcessImpl.waitFor(ProcessImpl.java:474)
     at jdk.test.lib.apps.LingeredApp.waitAppTerminate(LingeredApp.java:239) 
     at jdk.test.lib.apps.LingeredApp.stopApp(LingeredApp.java:380)
     at jdk.test.lib.apps.LingeredApp.stopApp(LingeredApp.java:434)
So the earlier call to interrupt() is resulting in waitAppTerminate() not actually waiting for exit. This then results in stopApp() getting IllegalThreadStateException when calling Process.exitValue().
If I comment out the call to interrupt() in Platform.shouldSAAttach(), I think the failure stack trace is much better:
java.lang.RuntimeException: Test ERROR java.lang.RuntimeException: java.lang.InterruptedException 
     at ClhsdbAttach.main(ClhsdbAttach.java:75)
     at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)      at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)      at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) 
     at java.base/java.lang.reflect.Method.invoke(Method.java:564)
     at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127) 
     at java.base/java.lang.Thread.run(Thread.java:832)
Caused by: java.lang.RuntimeException: java.lang.InterruptedException
     at jdk.test.lib.Platform.shouldSAAttach(Platform.java:300)
     at ClhsdbLauncher.run(ClhsdbLauncher.java:199)
     at ClhsdbAttach.main(ClhsdbAttach.java:71)
     ... 6 more
Caused by: java.lang.InterruptedException
     at jdk.test.lib.Platform.shouldSAAttach(Platform.java:297)
     ... 8 more
There's still a minor issue with rethrowing the RuntimeException encapsulated inside another RuntimeException. That the fault of the test which is catching all Exceptions and encapsulating them in a RuntimeException, even if the Exceptions itself is already a RuntimeException. It should add have a catch clause for RuntimeException, and just rethrow it without encapulating it. All the Clhsdb tests seem to do this, so that's about 20 places to fix. Probably not worth doing unless some other cleanup is being done at the same time. 

Chris

On 2/11/20 10:30 PM, Igor Ignatyev wrote:

I'd say yes, it's better to still call Thread::interrupt.

-- Igor
On Feb 11, 2020, at 10:19 PM, Chris Plummer <chris.plum...@oracle.com <mailto:chris.plum...@oracle.com>> wrote: 

Ok. Should I still call interrupt()?

Chris

On 2/11/20 10:07 PM, Igor Ignatyev wrote:

Hi Chris,
that's a common practice for any kind of library-ish code, if there are no explicit check of interrupt status, it will be checked a by next operation which might be interrupted. in this particular case, I agree rethrowing it as an unchecked exception might be a good alternative. 

-- Igor
On Feb 11, 2020, at 10:03 PM, Chris Plummer <chris.plum...@oracle.com <mailto:chris.plum...@oracle.com>> wrote: 

Hi Igor,
I guess I fail to see the benefit of this. Who is going to check the interrupt status of this thread and do something meaningful with it? It seems we would want to immediately propagate the failure by throwing a RuntimeException. This will work well when called from a test since this is a common way to fail a test. The other use of this code is by VMProps.vmHasSAandCanAttach(). It looks like if a RuntimeException is thrown the right thing will happen when SafeMap.put() catches the exception (it catches all Throwables). 

Chris

On 2/11/20 7:12 PM, Igor Ignatev wrote:

rather like this :


} catch (InterruptedException e) {
 Thread.currentThread().interrupt();
   return false; // assume not signed
}


— Igor
On Feb 11, 2020, at 6:15 PM, Chris Plummer <chris.plum...@oracle.com <mailto:chris.plum...@oracle.com>> wrote: 


Like this?

        } catch (InterruptedException e) {
Thread.currentThread().interrupt();
            throw new RuntimeException(e);
        }

Chris

On 2/11/20 2:23 PM, Igor Ignatyev wrote:
no, I meant to call Thread.currentThread().interrupt(), calling that will restore interrupted state of the thread, so an user of Platform class will be able to response to it appropriately, w/ your current code, the fact that the thread was interrupted will be missed, and in most cases it is not right thing to do. 

-- Igor
On Feb 11, 2020, at 2:02 PM, Chris Plummer <chris.plum...@oracle.com <mailto:chris.plum...@oracle.com>> wrote: 

Hi Igor,
I'm not sure what you mean by restore the interrupt state. Do you mean loop back to the waitFor() call? 

thanks,

Chris

On 2/11/20 1:55 PM, Igor Ignatyev wrote:

Hi Chris,
I don't insist on (3), so I'm fine if you don't want to change that part. one thing I'd change though is to restore thread interrupted state at L#266 of Platform.java (no need to publish new webrev) 

Thanks,
-- Igor
On Feb 11, 2020, at 1:49 PM, Chris Plummer <chris.plum...@oracle.com <mailto:chris.plum...@oracle.com>> wrote: 

Hi Igor,

Here's an updated webrev:
http://cr.openjdk.java.net/~cjplummer/8238196/webrev.01/index.html
I rebased to JDK 15 and made all the changes you suggested except for (3). I did not think it is necessary since the code is only executed on OSX. However, if you still feel allowing flexibility in the path separator is important, I can add that change too. 

thanks,

Chris

On 2/10/20 1:34 PM, Igor Ignatyev wrote:

Hi Chris,
in general it all looks good, I have a few comments (most of them are editorial): 
in Platform.java:
1. you have doubled spaced at line#238 (b/w boolean and isSignedOSX) 2. as FileNotFoundException is IOException, there is no need to declare the former in the signature of isSignedOSX 3. it's better to pass jdkPath, "bin" and "java" as separate arguments to Path.get, so the code won't depend on file separator 4. you are waiting for codesign to finish w/o reading its cout / cerr, which might lead to a deadlock (if codesign will exhaust IO buffer before exiting), so you need to either create two separate threads to read cout and cerr or  redirect these streams them to files and read these files afterwards or just ignore cout/cerr by using Redirect.DISCARD. I'd personally recommend the latter as the result of codesign can be reliably deduced from its exitcode (0 - signed, 1 - verification failed, 2 - wrong arguments, 3 - not all requirements from R are satisfied) and using cout/cerr is somewhat fragile as there is no guarantee output format won't be changed. 

the rest looks good to me.

-- Igor
On Feb 10, 2020, at 11:48 AM, Chris Plummer <chris.plum...@oracle.com <mailto:chris.plum...@oracle.com><mailto:chris.plum...@oracle.com>> wrote:
Ping #2. It's not that hard of a review. Most of it is the new Platform.isSignedOSX() method, which is well commented and pretty straight froward. 

thanks,

Chris

On 2/4/20 5:04 PM, Chris Plummer wrote:

Ping!
And I decided to push to 15 instead of 14. Will backport to 14 eventually. 

thanks,

Chris

On 1/30/20 10:20 PM, Chris Plummer wrote:

Yes, you are correct:

https://bugs.openjdk.java.net/browse/JDK-8238196
http://cr.openjdk.java.net/~cjplummer/8238196/webrev.00

thanks,

Chris

On 1/30/20 10:13 PM, Igor Ignatyev wrote:

Hi Chris,
http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00Â <http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00%C3%82> seems to be a webrev from another issue, should it have beenÂhttp://cr.openjdk.java.net/~cjplummer/8238196/webrev.00/Â <http://cr.openjdk.java.net/~cjplummer/8238196/webrev.00/%C3%82> ? 

-- Igor
On Jan 30, 2020, at 10:10 PM, Chris Plummer <chris.plum...@oracle.com <mailto:chris.plum...@oracle.com><mailto:chris.plum...@oracle.com>> wrote: 

Hello,
Please review the following fix for some SA tests that are failing on Mac OS X 10.14.5 and later: 

https://bugs.openjdk.java.net/browse/JDK-8238196
http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00
The issue is that SA can't attach to a signed binary starting with 10.14.5. There is no workaround for this, so these tests are being disabled when it is detected that the binary is signed and we are running on 10.14 or later (I chose all 10.14 releases to simplify the check).
Some background may help explain the fix. In order for SA to attach to a live process (not a core file) on OSX, either the attaching process (ie. the test) has to be run as root, or sudo needs to be supported. However, the only tests that make the sudo check are the 20 or so that use ClhsdbLauncher. The rest all rely on "@requires vm.hasSAandCanAttach" to filter out tests that use SA attach. vm.hasSAandCanAttach only checks if the test is being run as root. Thus all our non-ClhsdbLauncher tests that SA attach to a live process are currently not run unless they are run as root. 8238268 [1] has been filed to address this, making it so all the tests will attempt to use sudo if not run as root.
Because of the difference in how ClhsdbLauncher tests and "@requires vm.hasSAandCanAttach" tests check to see if they are runnable, this fix needs to address both types of checks. The common code for both these cases is Platform.shouldSAAttach(), which on OSX basically equates to check to see if we are running as root. I changed it to also return false if running on signed binary with 10.14 or later. However, this confused the ClhsdbLauncher use of Platform.shouldSAAttach() somewhat, since it assumed a false result only happens because you are not running as root (in which case it would then check if sudo will work). So ClhsdbLauncher now has double check that the false result was not because of running a signed binary. If it is signed, it won't do the sudo check. This will get cleaned up with 8238268 [1], which will move the sudo check into Platform.shouldSAAttach(). 

thanks,

Chris

[1] https://bugs.openjdk.java.net/browse/JDK-8238268

Reply via email to