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 at oracle.com]

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