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

[Igor Ignatyev]

yes, I'm fine w/ that.
-- Igor 

> On Feb 12, 2020, at 3:48 PM, Chris Plummer  wrote:
> 
> On 2/12/20 2:15 PM, David Holmes wrote:
>> 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.
> Ok. We're in agreement here. Igor, are you ok with this?
> 
> try {
> ...
> } catch (InterruptedException e) {
>throw new RuntimeException(e);
> }
> 
> thanks,
> 
> Chris
>> 
>> 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 

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

[Chris Plummer]

On 2/12/20 2:15 PM, David Holmes wrote:

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.

Ok. We're in agreement here. Igor, are you ok with this?

try {
    ...
} catch (InterruptedException e) {
   throw new RuntimeException(e);
}

thanks,

Chris


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 

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

[David Holmes]

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 

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

[Chris Plummer]

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.


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 

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

[David Holmes]

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 

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

[Chris Plummer]

  
  
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)
  

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

[Igor Ignatyev]

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

-- Igor

> On Feb 11, 2020, at 10:19 PM, Chris Plummer  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 >> > 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  > 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 >> > 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  > 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 

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

[Chris Plummer]

  
  
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
  
  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 
  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 
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:
  
  
 

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

[Igor Ignatyev]

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  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 >> > 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  > 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 >> > 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   >> 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,

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

[Chris Plummer]

  
  
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 
  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 
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 
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
  

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

[Igor Ignatev]

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  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 >> > 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  > 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 >> >> >> 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Â 
>>  
>> seems to be a webrev from another issue, should it have been 
>> http://cr.openjdk.java.net/~cjplummer/8238196/webrev.00/Â 
>>  ?
>> 
>> -- Igor
>> 
>>> On Jan 30, 2020, at 10:10 PM, Chris Plummer 

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

[Chris Plummer]

  
  
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
  
  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 
  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

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

[serguei . spitsyn]

Hi Chris,

This looks okay to me.

Thanks,
Serguei


On 2/11/20 1:49 PM, Chris Plummer 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 
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 seems to 
be a webrev from another issue, should it have been 
http://cr.openjdk.java.net/~cjplummer/8238196/webrev.00/ ?


-- Igor

On Jan 30, 2020, at 10:10 PM, Chris Plummer 
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

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

[Igor Ignatyev]

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  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 >> > 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   >> 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Â 
  seems 
 to be a webrev from another issue, should it have been 
 http://cr.openjdk.java.net/~cjplummer/8238196/webrev.00/Â 
  ?
 
 -- Igor
 
> On Jan 30, 2020, at 10:10 PM, Chris Plummer   >> 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 

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

[Chris Plummer]

  
  
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
  
  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 >
  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 seems
to be a webrev from another issue, should it
have been http://cr.openjdk.java.net/~cjplummer/8238196/webrev.00/ ?

-- Igor

On Jan 30,
  2020, at 10:10 PM, Chris Plummer >
  wrote:
  
  Hello,
  
  

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

[Igor Ignatyev]

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  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 >> >> >> 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Â 
>>  seems to 
>> be a webrev from another issue, should it have been 
>> http://cr.openjdk.java.net/~cjplummer/8238196/webrev.00/Â 
>>  ?
>> 
>> -- Igor
>> 
>>> On Jan 30, 2020, at 10:10 PM, Chris Plummer >> >> >> 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.
>>> 

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

[Chris Plummer]

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 > 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 seems to 
be a webrev from another issue, should it have been 
http://cr.openjdk.java.net/~cjplummer/8238196/webrev.00/ ?


-- Igor

On Jan 30, 2020, at 10:10 PM, Chris Plummer 
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

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

[Igor Ignatyev]

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  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 
 Â seems to be a 
 webrev from another issue, should it have been 
 http://cr.openjdk.java.net/~cjplummer/8238196/webrev.00/ 
 Â ?
 
 -- Igor
 
> On Jan 30, 2020, at 10:10 PM, Chris Plummer  > 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 
> 

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

[Chris Plummer]

  
  
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 seems
to be a webrev from another issue, should it have been http://cr.openjdk.java.net/~cjplummer/8238196/webrev.00/ ?
  
  
  -- Igor

  
On Jan 30, 2020, at 10:10 PM, Chris
  Plummer 
  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

  

  


  


  
  


  

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

[Chris Plummer]

  
  
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 seems
  to be a webrev from another issue, should it have been http://cr.openjdk.java.net/~cjplummer/8238196/webrev.00/ ?


-- Igor
  

  On Jan 30, 2020, at 10:10 PM, Chris Plummer

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