RFR(M) : 8210039 : move OSInfo to top level testlibrary
http://cr.openjdk.java.net/~iignatyev//8210039/webrev.00/index.html
698 lines changed: 114 ins; 240 del; 344 mod
Hi all, could you please review this clean up of jdk testlibrary? the patch updates the tests to use jdk.test.lib.Platform instead of jdk.testlibrary.OSInfo.OSType, cleans up OSInfo and renames it to jdk.test.lib.OSVersion. testing: changed tests + :jdk_desktop test group webrev: http://cr.openjdk.java.net/~iignatyev//8210039/webrev.00/index.html JBS: https://bugs.openjdk.java.net/browse/JDK-8210039 Thanks, -- Igor
Looks fine. On 28/08/2018 09:50, Igor Ignatyev wrote:
http://cr.openjdk.java.net/~iignatyev//8210039/webrev.00/index.html
698 lines changed: 114 ins; 240 del; 344 mod
Hi all,
could you please review this clean up of jdk testlibrary? the patch updates the tests to use jdk.test.lib.Platform instead of jdk.testlibrary.OSInfo.OSType, cleans up OSInfo and renames it to jdk.test.lib.OSVersion.
testing: changed tests + :jdk_desktop test group webrev: http://cr.openjdk.java.net/~iignatyev//8210039/webrev.00/index.html JBS: https://bugs.openjdk.java.net/browse/JDK-8210039
Thanks, -- Igor
-- Best regards, Sergey.
On 28/08/2018 17:50, Igor Ignatyev wrote:
http://cr.openjdk.java.net/~iignatyev//8210039/webrev.00/index.html
698 lines changed: 114 ins; 240 del; 344 mod Hi all,
could you please review this clean up of jdk testlibrary? the patch updates the tests to use jdk.test.lib.Platform instead of jdk.testlibrary.OSInfo.OSType, cleans up OSInfo and renames it to jdk.test.lib.OSVersion.
testing: changed tests + :jdk_desktop test group webrev: http://cr.openjdk.java.net/~iignatyev//8210039/webrev.00/index.html JBS: https://bugs.openjdk.java.net/browse/JDK-8210039
The updates to the Sockets policy file suggests using this jdk.test.lib.Platform/OSVersion requires permissions that the test infrastructure needs, not the test. It's not wrong but it's always a concern when tests running with a security manager are granted non-obvious permissions. I would be tempted to change this test to drop the usage of Platform/OSVersion so that the test can be granted only the permissions that it needs. Alternatively maybe we can just drop the OS check from this test if we can establish that there aren't test systems running Solaris 11.2 or older (the 11.3 update was 2015). -Alan
On 30 Aug 2018, at 08:51, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 28/08/2018 17:50, Igor Ignatyev wrote:
http://cr.openjdk.java.net/~iignatyev//8210039/webrev.00/index.html
698 lines changed: 114 ins; 240 del; 344 mod Hi all,
could you please review this clean up of jdk testlibrary? the patch updates the tests to use jdk.test.lib.Platform instead of jdk.testlibrary.OSInfo.OSType, cleans up OSInfo and renames it to jdk.test.lib.OSVersion.
testing: changed tests + :jdk_desktop test group webrev: http://cr.openjdk.java.net/~iignatyev//8210039/webrev.00/index.html JBS: https://bugs.openjdk.java.net/browse/JDK-8210039
The updates to the Sockets policy file suggests using this jdk.test.lib.Platform/OSVersion requires permissions that the test infrastructure needs, not the test. It's not wrong but it's always a concern when tests running with a security manager are granted non-obvious permissions.
The uses of test libraries with security manager is a little cumbersome, and usually ends up with the test code being granted more permissions than is necessary. I share Alan’s concern. Another alternative, that we used in other areas, is to grant the test library only minimal permissions, separate to the actual test code. For example: http://hg.openjdk.java.net/jdk/jdk/file/9183040e34d8/test/jdk/java/net/httpc... -Chris.
Alan, Chris, thanks for looking at it, I went w/ the alternative suggested by Chris. that required a sprinkle of doPrivileged in the testlibrary, but now Sockets/policy.* files grant the minimal required permissions to the test code. the incremental webrev can found here[1], please let me know if you need the whole webrev. [1] http://cr.openjdk.java.net/~iignatyev//8210039/webrev.0-1/index.html Thanks, -- Igor
On Aug 30, 2018, at 3:28 AM, Chris Hegarty <chris.hegarty@oracle.com> wrote:
On 30 Aug 2018, at 08:51, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 28/08/2018 17:50, Igor Ignatyev wrote:
http://cr.openjdk.java.net/~iignatyev//8210039/webrev.00/index.html
698 lines changed: 114 ins; 240 del; 344 mod Hi all,
could you please review this clean up of jdk testlibrary? the patch updates the tests to use jdk.test.lib.Platform instead of jdk.testlibrary.OSInfo.OSType, cleans up OSInfo and renames it to jdk.test.lib.OSVersion.
testing: changed tests + :jdk_desktop test group webrev: http://cr.openjdk.java.net/~iignatyev//8210039/webrev.00/index.html JBS: https://bugs.openjdk.java.net/browse/JDK-8210039
The updates to the Sockets policy file suggests using this jdk.test.lib.Platform/OSVersion requires permissions that the test infrastructure needs, not the test. It's not wrong but it's always a concern when tests running with a security manager are granted non-obvious permissions.
The uses of test libraries with security manager is a little cumbersome, and usually ends up with the test code being granted more permissions than is necessary. I share Alan’s concern.
Another alternative, that we used in other areas, is to grant the test library only minimal permissions, separate to the actual test code. For example:
http://hg.openjdk.java.net/jdk/jdk/file/9183040e34d8/test/jdk/java/net/httpc... <http://hg.openjdk.java.net/jdk/jdk/file/9183040e34d8/test/jdk/java/net/httpclient/AsFileDownloadTest.policy#l24>
-Chris.
On 31/08/2018 19:42, Igor Ignatyev wrote:
Alan, Chris,
thanks for looking at it, I went w/ the alternative suggested by Chris. that required a sprinkle of doPrivileged in the testlibrary, but now Sockets/policy.* files grant the minimal required permissions to the test code. the incremental webrev can found here[1], please let me know if you need the whole webrev.
[1] http://cr.openjdk.java.net/~iignatyev//8210039/webrev.0-1/index.html <http://cr.openjdk.java.net/%7Eiignatyev//8210039/webrev.0-1/index.html>
I can't quickly verify that the code source "file:${test.classes}/../../../../test/lib/-" results in granting the permission to the right library but I assume it will fail if it's not correct. So I think this approach looks good to me. -Alan
Igor,
On 31 Aug 2018, at 19:42, Igor Ignatyev <igor.ignatyev@oracle.com> wrote:
Alan, Chris,
thanks for looking at it, I went w/ the alternative suggested by Chris. that required a sprinkle of doPrivileged in the testlibrary, but now Sockets/policy.* files grant the minimal required permissions to the test code.
It’s bit annoying to have retrofit the test library classes with doPriv, but I think that it is the right approach, if we intend them to be usable with tests running with a security manager ( which we do ).
the incremental webrev can found here[1], please let me know if you need the whole webrev.
[1] http://cr.openjdk.java.net/~iignatyev//8210039/webrev.0-1/index.html <http://cr.openjdk.java.net/~iignatyev//8210039/webrev.0-1/index.html> I think this is fine.
Thanks, -Chris.
Hi Igor, Nit: You could have introduced a private static String getProperty(String name) { return AccessController.doP.... } in Platform.java to avoid the long lines. Otherwise looks good to me too! best regards, -- daniel On 31/08/2018 19:42, Igor Ignatyev wrote:
Alan, Chris,
thanks for looking at it, I went w/ the alternative suggested by Chris. that required a sprinkle of doPrivileged in the testlibrary, but now Sockets/policy.* files grant the minimal required permissions to the test code. the incremental webrev can found here[1], please let me know if you need the whole webrev.
[1] http://cr.openjdk.java.net/~iignatyev//8210039/webrev.0-1/index.html
Thanks, -- Igor
On Aug 30, 2018, at 3:28 AM, Chris Hegarty <chris.hegarty@oracle.com> wrote:
On 30 Aug 2018, at 08:51, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 28/08/2018 17:50, Igor Ignatyev wrote:
http://cr.openjdk.java.net/~iignatyev//8210039/webrev.00/index.html
698 lines changed: 114 ins; 240 del; 344 mod Hi all,
could you please review this clean up of jdk testlibrary? the patch updates the tests to use jdk.test.lib.Platform instead of jdk.testlibrary.OSInfo.OSType, cleans up OSInfo and renames it to jdk.test.lib.OSVersion.
testing: changed tests + :jdk_desktop test group webrev: http://cr.openjdk.java.net/~iignatyev//8210039/webrev.00/index.html JBS: https://bugs.openjdk.java.net/browse/JDK-8210039
The updates to the Sockets policy file suggests using this jdk.test.lib.Platform/OSVersion requires permissions that the test infrastructure needs, not the test. It's not wrong but it's always a concern when tests running with a security manager are granted non-obvious permissions.
The uses of test libraries with security manager is a little cumbersome, and usually ends up with the test code being granted more permissions than is necessary. I share Alan’s concern.
Another alternative, that we used in other areas, is to grant the test library only minimal permissions, separate to the actual test code. For example:
http://hg.openjdk.java.net/jdk/jdk/file/9183040e34d8/test/jdk/java/net/httpc... <http://hg.openjdk.java.net/jdk/jdk/file/9183040e34d8/test/jdk/java/net/httpclient/AsFileDownloadTest.policy#l24>
-Chris.
Daniel, Chris, Alan, thank you for your review. I've pushed it w/ Platfform::privilegedGetProperty method added to shorten lines in Platform.java Cheers, -- Igor
On Sep 4, 2018, at 6:08 AM, Daniel Fuchs <daniel.fuchs@oracle.com> wrote:
Hi Igor,
Nit: You could have introduced a private static String getProperty(String name) { return AccessController.doP.... } in Platform.java to avoid the long lines.
Otherwise looks good to me too!
best regards,
-- daniel
On 31/08/2018 19:42, Igor Ignatyev wrote:
Alan, Chris, thanks for looking at it, I went w/ the alternative suggested by Chris. that required a sprinkle of doPrivileged in the testlibrary, but now Sockets/policy.* files grant the minimal required permissions to the test code. the incremental webrev can found here[1], please let me know if you need the whole webrev. [1] http://cr.openjdk.java.net/~iignatyev//8210039/webrev.0-1/index.html Thanks, -- Igor
On Aug 30, 2018, at 3:28 AM, Chris Hegarty <chris.hegarty@oracle.com> wrote:
On 30 Aug 2018, at 08:51, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 28/08/2018 17:50, Igor Ignatyev wrote:
http://cr.openjdk.java.net/~iignatyev//8210039/webrev.00/index.html
698 lines changed: 114 ins; 240 del; 344 mod Hi all,
could you please review this clean up of jdk testlibrary? the patch updates the tests to use jdk.test.lib.Platform instead of jdk.testlibrary.OSInfo.OSType, cleans up OSInfo and renames it to jdk.test.lib.OSVersion.
testing: changed tests + :jdk_desktop test group webrev: http://cr.openjdk.java.net/~iignatyev//8210039/webrev.00/index.html JBS: https://bugs.openjdk.java.net/browse/JDK-8210039
The updates to the Sockets policy file suggests using this jdk.test.lib.Platform/OSVersion requires permissions that the test infrastructure needs, not the test. It's not wrong but it's always a concern when tests running with a security manager are granted non-obvious permissions.
The uses of test libraries with security manager is a little cumbersome, and usually ends up with the test code being granted more permissions than is necessary. I share Alan’s concern.
Another alternative, that we used in other areas, is to grant the test library only minimal permissions, separate to the actual test code. For example:
http://hg.openjdk.java.net/jdk/jdk/file/9183040e34d8/test/jdk/java/net/httpc... <http://hg.openjdk.java.net/jdk/jdk/file/9183040e34d8/test/jdk/java/net/httpclient/AsFileDownloadTest.policy#l24>
-Chris.
participants (5)
-
Alan Bateman
-
Chris Hegarty
-
Daniel Fuchs
-
Igor Ignatyev
-
Sergey Bylokhov