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.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 >>> <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> 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/> ? >>>> >>>> -- Igor >>>> >>>>> On Jan 30, 2020, at 10:10 PM, Chris Plummer <chris.plum...@oracle.com >>>>> <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 >>>>> <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> >>>>> >>>> >>> >> >
