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