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
david.holmes at oracle.com
Wed Feb 12 13:58:39 UTC 2020
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.plummer at oracle.com
>>> <mailto:chris.plummer at 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.plummer at oracle.com <mailto:chris.plummer at 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.Plummer at oracle.com <mailto:Chris.Plummer at 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.plummer at oracle.com <mailto:chris.plummer at 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.plummer at oracle.com <mailto:chris.plummer at 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.plummer at oracle.com
>>>>>>>>>>>>> <mailto:chris.plummer at oracle.com><mailto:chris.plummer at 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.plummer at oracle.com
>>>>>>>>>>>>>>>>> <mailto:chris.plummer at oracle.com><mailto:chris.plummer at 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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the serviceability-dev
mailing list