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 <[email protected]> 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 <[email protected]
>>> <mailto:[email protected]>> 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 <[email protected]
>>>>> <mailto:[email protected]>> wrote:
>>>>>
>>>>> Hi Igor,
>>>>>
>>>>> Here's an updated webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~cjplummer/8238196/webrev.01/index.html
>>>>> <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 <[email protected]
>>>>>>> <mailto:[email protected]><mailto:[email protected]
>>>>>>> <mailto:[email protected]>>> 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
>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8238196>
>>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8238196/webrev.00
>>>>>>>>> <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
>>>>>>>>>>> <[email protected]
>>>>>>>>>>> <mailto:[email protected]><mailto:[email protected]
>>>>>>>>>>> <mailto:[email protected]>>> 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
>>>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8238196>
>>>>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00
>>>>>>>>>>> <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
>>>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8238268>
>>>
>>
>
