Hi Chris and Daniel,

Well spotted, Chris. Thanks for that!

Thanks too Daniel, that's a good idea. I've made those changes and included them in a new webrev, which you can find below.

http://cr.openjdk.java.net/~pconcannon/8240533/webrevs/webrev.02/


Kind regards,

Patrick

On 03/04/2020 17:16, Daniel Fuchs wrote:
Hi Patrick,

 120         { perms.add(new SocketPermission("127.0.0.1:0",
 121                 "connect,accept")); }
 122         { perms.add(new SocketPermission("0.0.0.0:0",
 123                 "connect,accept")); }

there in other tests - I think a single permission:

    { perms.add(new SocketPermission("*:0")); }

would be more robust as it would take care of both IPv6 and IPv4 in one
go. We should strive to avoid to hard-code 127.0.0.1 and 0.0.0.0
in tests.

best regards,

-- daniel

On 03/04/2020 14:47, Patrick Concannon wrote:
Hi,

Thanks for the feedback. 



Lance - I swapped out expectThrows for assertThrows, as requested.

Chris - I put in an extra check in the tests to ensure that the new code doesn’t interfere with the Security Manager checks already present in the source.

The new webrev can be found here: http://cr.openjdk.java.net/~pconcannon/8240533/webrevs/webrev.01/

 <http://cr.openjdk.java.net/~pconcannon/8240533/webrevs/webrev.01/>


Kind regards,

Patrick

On 31/03/2020 15:33, Chris Hegarty wrote:
Patrick,

On 31 Mar 2020, at 15:08, Daniel Fuchs<daniel.fu...@oracle.com>  wrote:

..
bug:https://bugs.openjdk.java.net/browse/JDK-8240533
webrev:http://cr.openjdk.java.net/~pconcannon/8240533/webrevs/webrev.00/
Look good Patrick.

The check is deliberately performed after the security manager checks, right? If so, it is worth asserting this in a test.

-Chris.


Reply via email to