Re: [9] Review request for 8072515: Test Task: Develop new tests for JEP 219: Datagram Transport Layer Security (DTLS)
Looks fine to me. It's nice to keep each line not exceed 80 characters. For example - * @run main/othervm -Dtest.security.protocol=DTLS -Dtest.mode=norm DTLSBufferOverflowUnderflowTest + * @run main/othervm -Dtest.security.protocol=DTLS + * -Dtest.mode=norm DTLSBufferOverflowUnderflowTest Thanks, Xuelei On 6/2/2015 8:15 PM, Konstantin Shefov wrote: > Hello, > > Please review new tests fro DTLS feature for JDK 9: > > bug: https://bugs.openjdk.java.net/browse/JDK-8072515 > webrev: http://cr.openjdk.java.net/~kshefov/8072515/webrev.00/ > > > Thanks > -Konstantin >
Re: JDK 9 RFR of JDK-8083436: Doclint regression introduced by JDK-8043758
Looks fine to me.
Thanks for the update!
Xuelei
On 6/4/2015 7:06 AM, Joseph D. Darcy wrote:
> Hello,
>
> Please review the patch below to address a recently introduced doclint
> regression.
>
> Thanks,
>
> -Joe
>
> diff -r 5f952ade41ff
> src/java.base/share/classes/javax/net/ssl/SSLEngineResult.java
> --- a/src/java.base/share/classes/javax/net/ssl/SSLEngineResult.java Wed
> Jun 03 14:35:17 2015 -0700
> +++ b/src/java.base/share/classes/javax/net/ssl/SSLEngineResult.java Wed
> Jun 03 16:04:22 2015 -0700
> @@ -280,7 +280,7 @@
> * @apiNote Note that sequence number is an unsigned long and cannot
> * exceed {@code -1L}. It is desired to use the unsigned
> * long comparing mode for comparison of unsigned long
> values
> - * (see also {@link java.lang.Long#compareUnsigned()
> + * (see also {@link java.lang.Long#compareUnsigned(long,
> long)
> * Long.compareUnsigned()}).
> *
> * For DTLS protocols, the first 16 bits of the sequence
> @@ -300,7 +300,7 @@
> * record; or ${@code -1L} if no record is produced or
> consumed,
> * or this operation is not supported by the underlying
> provider
> *
> - * @see java.lang.Long#compareUnsigned(boolean, boolean)
> + * @see java.lang.Long#compareUnsigned(long, long)
> *
> * @since 1.9
> */
>
Re: JDK 9 RFR of JDK-8083436: Doclint regression introduced by JDK-8043758
Or I think you could also just remove the args, since there is only one
compareUnsigned currently. Probably safer to use long, long.
Brad
On 6/3/2015 4:06 PM, Joseph D. Darcy wrote:
Hello,
Please review the patch below to address a recently introduced doclint
regression.
Thanks,
-Joe
diff -r 5f952ade41ff
src/java.base/share/classes/javax/net/ssl/SSLEngineResult.java
--- a/src/java.base/share/classes/javax/net/ssl/SSLEngineResult.java Wed
Jun 03 14:35:17 2015 -0700
+++ b/src/java.base/share/classes/javax/net/ssl/SSLEngineResult.java Wed
Jun 03 16:04:22 2015 -0700
@@ -280,7 +280,7 @@
* @apiNote Note that sequence number is an unsigned long and cannot
* exceed {@code -1L}. It is desired to use the unsigned
* long comparing mode for comparison of unsigned long
values
- * (see also {@link java.lang.Long#compareUnsigned()
+ * (see also {@link java.lang.Long#compareUnsigned(long,
long)
* Long.compareUnsigned()}).
*
* For DTLS protocols, the first 16 bits of the sequence
@@ -300,7 +300,7 @@
* record; or ${@code -1L} if no record is produced or
consumed,
* or this operation is not supported by the underlying
provider
*
- * @see java.lang.Long#compareUnsigned(boolean, boolean)
+ * @see java.lang.Long#compareUnsigned(long, long)
*
* @since 1.9
*/
JDK 9 RFR of JDK-8083436: Doclint regression introduced by JDK-8043758
Hello,
Please review the patch below to address a recently introduced doclint
regression.
Thanks,
-Joe
diff -r 5f952ade41ff
src/java.base/share/classes/javax/net/ssl/SSLEngineResult.java
--- a/src/java.base/share/classes/javax/net/ssl/SSLEngineResult.java Wed
Jun 03 14:35:17 2015 -0700
+++ b/src/java.base/share/classes/javax/net/ssl/SSLEngineResult.java Wed
Jun 03 16:04:22 2015 -0700
@@ -280,7 +280,7 @@
* @apiNote Note that sequence number is an unsigned long and cannot
* exceed {@code -1L}. It is desired to use the unsigned
* long comparing mode for comparison of unsigned long
values
- * (see also {@link java.lang.Long#compareUnsigned()
+ * (see also {@link java.lang.Long#compareUnsigned(long,
long)
* Long.compareUnsigned()}).
*
* For DTLS protocols, the first 16 bits of the sequence
@@ -300,7 +300,7 @@
* record; or ${@code -1L} if no record is produced or
consumed,
* or this operation is not supported by the underlying
provider
*
- * @see java.lang.Long#compareUnsigned(boolean, boolean)
+ * @see java.lang.Long#compareUnsigned(long, long)
*
* @since 1.9
*/
JEP 232 RFR: JDK-8065942 and JDK-8056179
This is the third and fourth in a series of fixes for JEP 232 (Improve Secure Application Performance) [1]. webrev: http://cr.openjdk.java.net/~mullan/webrevs/8065942-8056179/webrev.00/ bugs: https://bugs.openjdk.java.net/browse/JDK-8065942 and https://bugs.openjdk.java.net/browse/JDK-8056179 This fix changes the Permissions and PermissionCollection subclasses to use concurrent collections, which significantly reduces contention when multiple threads are performing security checks. The bugs needed to be fixed together, because removing the synchronized blocks in Permissions revealed that several of the underlying PermissionCollection implementations were not thread-safe. Several new unit tests were also added to test basic functionality of these classes. With these fixes, the throughput of the Permissions.implies method improves from approximately 6x to 10x when more than one thread is running. Each of the bugs contains a performance chart with more details. Thanks, Sean [1] http://openjdk.java.net/jeps/232
Re: RFR 8031111: fix krb5 caddr (and 8079821: MSOID2.java test is not perfect)
I don't think it's worth doing. Overflow at nt[pos-1] means the size
is bigger than 65535 (or 32767, unsigned? Not sure at the momemnt)
which is impossible for a SPNEGO token. Furthermore, if we really want
to worry about it, we will need to expand the length octets from 2
bytes to 3 bytes and it will be much more complicated.
Ok, just add some comment to state this then.
No further comments.
Thanks,
Valerie
On 6/1/2015 6:24 PM, Weijun Wang wrote:
On 06/02/2015 04:36 AM, Valerie Peng wrote:
Some nit/questions for 803 webrev:
In the test, why not use "noaddresses" since it's the one documented in
the krb5 conf page?
I'll use noaddresses.
If "noaddresses" is true, then the extra_addresses has no effect, right?
I didn't see checking for the "noaddresses" in HostAddresses.java file.
Is that done somewhere else?
The getLocalAddresses() method is only called in KrbAsReq as
if (addresses == null && cfg.useAddresses()) {
addresses = HostAddresses.getLocalAddresses();
}
cfg.useAddress() checks the noaddresses setting.
As for 8079821 webrev, do u need to check nt[pos-1] for overflow as well
when adding 1 to it?
I don't think it's worth doing. Overflow at nt[pos-1] means the size
is bigger than 65535 (or 32767, unsigned? Not sure at the momemnt)
which is impossible for a SPNEGO token. Furthermore, if we really want
to worry about it, we will need to expand the length octets from 2
bytes to 3 bytes and it will be much more complicated.
Thanks
Max
Valerie
On 5/8/2015 8:00 AM, Weijun Wang wrote:
Hi Valerie
Please review the code change at
http://cr.openjdk.java.net/~weijun/803/webrev.00/
The codes to read local addresses are updated. We are also supporting
the extra_addresses krb5.conf setting.
This code change triggers a bug (MSOID2.java) in a test I've recently
added, please also review the change at
http://cr.openjdk.java.net/~weijun/8079821/webrev.00/
Thanks
Max
Re: RFR 7191662: JCE providers should be located via ServiceLoader
Correct, if the makefile related changes are removed then no need for build team to review 7191662 webrev anymore. There are other discussions ongoing and we should be able to reach a decision in a day or two. Will update the list again. Thanks, Valerie On 06/01/15 16:39, Magnus Ihse Bursie wrote: On 2015-05-29 00:10, Valerie Peng wrote: Please find comments in line... On 5/27/2015 3:42 PM, Mandy Chung wrote: Valerie, Did you see my comment yesterday? http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html Yes, we exchanged emails after this above one. I will follow up your latest one later today. Since you have reverted the java.security to keep the classname, to avoid causing merge conflict to jimage refresh, let’s remove the META-INF files in the first push and the build change. The security providers will be loaded via the fallback mechanism (i.e. ProviderLoader.legacyLoad method). You should keep the ProviderLoader.load method to take the provider name instead of classname. Sure, I can remove the META-INF files so the providers are loaded through the legacyLoad(). Hmm, the ProviderLoader.load() method is used by java.security file provider loading. Since the current list still uses class name, it should take class name when checking for matches while iterating through the list returned by ServiceLoader. This way, when changes are sync'ed into Jake, no extra change required and the providers will be loaded through ProviderLoader.load() automatically with the current list. I’ll file a bug to follow up to change java.security to list the provider name. This will wait after the jimage refresh goes into jdk9/dev Ok. Thanks, Valerie I'm not sure I followed completely here were this landed. Does this mean that there's currently no need for a build system code review on http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/, and that a new webrev will be posted instead? /Magnus . Mandy On May 27, 2015, at 3:29 PM, Valerie Peng wrote: Hi, build experts, Can you please review the make file related change, i.e. the new file make/gensrc/Gensrc-java.naming.gmk, in the following webrev: http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/ This is for merging the java.security.Provider file from various providers and use the (merged) result for the final image build. The rest of source code changes are reviewed by my team already. Thanks, Valerie (Java Security Team)
Re: Code Review Request, JDK-8081792, buffer size calculation issue in NativeGCMCipher
Change looks fine. Thanks, Valerie On 06/03/15 01:55, Xuelei Fan wrote: Hi Weijun or Valerie, Can I get a quick code review for this simple fix? DTLS implementation exposes the bug. There are a few new nightly testing failure. No new regression test. The existing test cases (the nightly testing failure) can be used as regression test instead. Webrev: http://cr.openjdk.java.net/~xuelei/8081792/webrev.00/ Thanks, Xuelei
Code Review Request, JDK-8081792, buffer size calculation issue in NativeGCMCipher
Hi Weijun or Valerie, Can I get a quick code review for this simple fix? DTLS implementation exposes the bug. There are a few new nightly testing failure. No new regression test. The existing test cases (the nightly testing failure) can be used as regression test instead. Webrev: http://cr.openjdk.java.net/~xuelei/8081792/webrev.00/ Thanks, Xuelei
