Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v2]
On Fri, 9 Oct 2020 03:58:38 GMT, Valerie Peng wrote: >> Anthony Scarpino has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Xuelei comments > > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 621: > >> 619: null : ByteBuffer.wrap(ibuffer.toByteArray()), src, >> dst); >> 620: dst.reset(); >> 621: ghashAllToS.doLastBlock(dst, processed); > > Are we sure about using "processed" here? I'd expect the value is the number > of bytes written into dst in the > doLastBlock(...) call on line 618. Is the "processed" variable used > differently in ByteBuffer case? it should probably be 'len' - PR: https://git.openjdk.java.net/jdk/pull/411
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v2]
On Fri, 9 Oct 2020 00:48:42 GMT, Valerie Peng wrote:
>> Anthony Scarpino has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Xuelei comments
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
> line 550:
>
>> 548:
>> 549: processed += len;
>> 550: ghashAllToS.update(src, len);
>
> Isn't input to ghashAllToS always be the produced cipher text? Did I miss
> something?
method is removed
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
> line 617:
>
>> 615:
>> 616: processAAD();
>> 617: if (len > 0) {
>
> Even if (len == 0), we should still process the data stored into 'ibuffer'?
> It seems that both of the
> encrypt(ByteBuffer) and encryptFinal(ByteBuffer) are adapted from their
> counterpart with byte[] arguments. However, the
> byte[] methods have different entrant conditions due to the buffering in
> CipherCore. So the impl of the ByteBuffer ones
> may need additional logic to handle all possible calling sequence.
Yes for encryptFinal. encrypt is removed
-
PR: https://git.openjdk.java.net/jdk/pull/411
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v2]
On Fri, 9 Oct 2020 03:20:40 GMT, Valerie Peng wrote: >> Anthony Scarpino has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Xuelei comments > > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 635: > >> 633: dst.put(block, 0, tagLenBytes); >> 634: >> 635: return (processed + tagLenBytes); > > Is it supposed to return "all data processed + tag len"? Normally, we return > the output size produced by this > particular call instead of all accumulated. Yes, that should be len + tagLenBytes. We obviously don't check that in our tests - PR: https://git.openjdk.java.net/jdk/pull/411
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v2]
On Fri, 9 Oct 2020 01:01:47 GMT, Valerie Peng wrote: >> Anthony Scarpino has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Xuelei comments > > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 595: > >> 593: } >> 594: GCTR gctrForSToTag = new GCTR(embeddedCipher, >> this.preCounterBlock); >> 595: gctrForSToTag.doFinal(s, 0, s.length, block, 0); > > since GCTR output the same length of output as input, (e.g. 'sOut' is same > length as 's'), can't we just re-use 's' as > the output buffer instead of 'block'? Actually I can take it one step further.. I think I can remove 's' and use 'block' for everything. I'll have to make sure no unexpected overwriting happens. - PR: https://git.openjdk.java.net/jdk/pull/411
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v2]
On Fri, 9 Oct 2020 01:04:26 GMT, Valerie Peng wrote:
>> Anthony Scarpino has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Xuelei comments
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
> line 593:
>
>> 591: if (tagLenBytes > block.length) {
>> 592: block = new byte[tagLenBytes];
>> 593: }
>
> Is tagLenBytes ever larger than AES block size?
no, 16 octets
-
PR: https://git.openjdk.java.net/jdk/pull/411
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v2]
On Thu, 8 Oct 2020 06:51:08 GMT, Anthony Scarpino wrote:
>> 8253821: Improve ByteBuffer performance with GCM
>
> Anthony Scarpino has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Xuelei comments
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
550:
> 548:
> 549: processed += len;
> 550: ghashAllToS.update(src, len);
Isn't input to ghashAllToS always be the produced cipher text? Did I miss
something?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
595:
> 593: }
> 594: GCTR gctrForSToTag = new GCTR(embeddedCipher,
> this.preCounterBlock);
> 595: gctrForSToTag.doFinal(s, 0, s.length, block, 0);
since GCTR output the same length of output as input, (e.g. 'sOut' is same
length as 's'), can't we just re-use 's' as
the output buffer instead of 'block'?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
593:
> 591: if (tagLenBytes > block.length) {
> 592: block = new byte[tagLenBytes];
> 593: }
Is tagLenBytes ever larger than AES block size?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
743:
> 741: }
> 742: GCTR gctrForSToTag = new GCTR(embeddedCipher,
> this.preCounterBlock);
> 743: gctrForSToTag.doFinal(s, 0, tagLenBytes, block, 0);
Same comments as earlier.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
748:
> 746: int mismatch = 0;
> 747: for (int i = 0; i < tagLenBytes; i++) {
> 748: mismatch |= tag[i] ^ block[i];
block->s?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
605:
> 603: int len = src.remaining();
> 604: dst.mark();
> 605: if (len > MAX_BUF_SIZE - tagLenBytes) {
Missing the bytes in ibuffer for this check?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
610:
> 608: }
> 609:
> 610: if (dst.remaining() < len + tagLenBytes) {
Missing the bytes in ibuffer for this check?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
614:
> 612: }
> 613:
> 614: checkDataLength(processed, len);
It seems that both checks (line 605 and 614) can be combined into:
checkDataLength(processed, Math.addExact(len, tagLenBytes));
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
630:
> 628: if (tagLenBytes > block.length) {
> 629: block = new byte[tagLenBytes];
> 630: }
Again, will this ever happen? Can just use 's' instead of 'block' from here and
below?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
635:
> 633: dst.put(block, 0, tagLenBytes);
> 634:
> 635: return (processed + tagLenBytes);
Is it supposed to return "all data processed + tag len"? Normally, we return
the output size produced by this
particular call instead of all accumulated.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
617:
> 615:
> 616: processAAD();
> 617: if (len > 0) {
Even if (len == 0), we should still process the data stored into 'ibuffer'? It
seems that both of the
encrypt(ByteBuffer) and encryptFinal(ByteBuffer) are adapted from their
counterpart with byte[] arguments. However, the
byte[] methods have different entrant conditions due to the buffering in
CipherCore. So the impl of the ByteBuffer ones
may need additional logic to handle all possible calling sequence.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
621:
> 619: null : ByteBuffer.wrap(ibuffer.toByteArray()), src,
> dst);
> 620: dst.reset();
> 621: ghashAllToS.doLastBlock(dst, processed);
Are we sure about using "processed" here? I'd expect the value is the number of
bytes written into dst in the
doLastBlock(...) call on line 618. Is the "processed" variable used differently
in ByteBuffer case?
-
PR: https://git.openjdk.java.net/jdk/pull/411
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v2]
On Fri, 9 Oct 2020 00:42:57 GMT, Valerie Peng wrote: >> Anthony Scarpino has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Xuelei comments > > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 545: > >> 543: "Unable to add remaining input to the buffer", >> e); >> 544: } >> 545: } > > Existing usage for ibuffer is only for decryption. If you are storing into > ibuffer for encryption, the comment about it > should be updated. The earlier code in this method should also check ibuffer > and if non-empty, apply its content before > applying src? I updated the comment. I had redone this method before your comment. Take a look at that on the next webrev - PR: https://git.openjdk.java.net/jdk/pull/411
Re: RFR: 8153005: Upgrade the default PKCS12 encryption/MAC algorithms [v3]
On Fri, 9 Oct 2020 00:07:39 GMT, Weijun Wang wrote: >> I tried but cannot find a way to tell if a system is Windows Server 2016 or >> 2019. Their os.version is all 10.0. I've >> filed an enhancement at https://bugs.openjdk.java.net/browse/JDK-8254241 for >> it. That said, I did try running the test >> on a Windows Server 2019 using new algorithms and it succeeds. > > There are existing tests reading openssl generated pkcs12 files in > https://github.com/openjdk/jdk/tree/master/test/jdk/sun/security/pkcs12/params, > it already contains files using both > weak and strong algorithms. Update `params/README`, exclude it from the de-BASE64 list (don't know it succeeded) in the `ParamsTest.java` test. Also remove a useless call in the test. Thinking about adding a benchmark, but it will be in another commit. - PR: https://git.openjdk.java.net/jdk/pull/473
Re: RFR: 8153005: Upgrade the default PKCS12 encryption/MAC algorithms [v3]
> Default algorithms are bumped to be based on PBES2 with AES-256 and SHA-256. > Please also review the CSR at > https://bugs.openjdk.java.net/browse/JDK-8228481. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: update README and exclude README - Changes: - all: https://git.openjdk.java.net/jdk/pull/473/files - new: https://git.openjdk.java.net/jdk/pull/473/files/6b5c5b5e..41be78aa Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=473=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=473=01-02 Stats: 39 lines in 2 files changed: 7 ins; 6 del; 26 mod Patch: https://git.openjdk.java.net/jdk/pull/473.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/473/head:pull/473 PR: https://git.openjdk.java.net/jdk/pull/473
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v2]
On Thu, 8 Oct 2020 06:51:08 GMT, Anthony Scarpino wrote: >> 8253821: Improve ByteBuffer performance with GCM > > Anthony Scarpino has updated the pull request incrementally with one > additional commit since the last revision: > > Xuelei comments src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 545: > 543: "Unable to add remaining input to the buffer", > e); > 544: } > 545: } Existing usage for ibuffer is only for decryption. If you are storing into ibuffer for encryption, the comment about it should be updated. The earlier code in this method should also check ibuffer and if non-empty, apply its content before applying src? - PR: https://git.openjdk.java.net/jdk/pull/411
Re: RFR: 8153005: Upgrade the default PKCS12 encryption/MAC algorithms [v2]
On Fri, 9 Oct 2020 00:04:17 GMT, Weijun Wang wrote: >> Are you still planning, or is it possible to add a test for Windows 2019? >> Also, have you considered adding a test that >> checks if the JDK can read OpenSSL PKCS#12 files and vice versa? Maybe we >> can do that later as a follow-on issue. >> Otherwise, I will approve. > > I tried but cannot find a way to tell if a system is Windows Server 2016 or > 2019. Their os.version is all 10.0. I've > filed an enhancement at https://bugs.openjdk.java.net/browse/JDK-8254241 for > it. That said, I did try running the test > on a Windows Server 2019 using new algorithms and it succeeds. There are existing tests reading openssl generated pkcs12 files in https://github.com/openjdk/jdk/tree/master/test/jdk/sun/security/pkcs12/params, but I can add a new one using strong algorithms. - PR: https://git.openjdk.java.net/jdk/pull/473
Re: RFR: 8153005: Upgrade the default PKCS12 encryption/MAC algorithms [v2]
On Thu, 8 Oct 2020 16:34:59 GMT, Sean Mullan wrote: >> New commit updating ic to 1. I also created separate constants for >> DEFAULT_CERT_PBE_ITERATION_COUNT and >> DEFAULT_KEY_PBE_ITERATION_COUNT. I haven't made the change for >> LEGACY_PBE_ITERATION_COUNT since they will never change. > > Are you still planning, or is it possible to add a test for Windows 2019? > Also, have you considered adding a test that > checks if the JDK can read OpenSSL PKCS#12 files and vice versa? Maybe we can > do that later as a follow-on issue. > Otherwise, I will approve. I tried but cannot find a way to tell if a system is Windows Server 2016 or 2019. Their os.version is all 10.0. I've filed an enhancement for it. That said, I did try running the test using new algorithms and it succeeds. - PR: https://git.openjdk.java.net/jdk/pull/473
RFR: 8253563: Change sun.security.jca.Providers.threadLists to be ThreadLocal
Could someone help reviewing this one-line change? This changes the provider list used during jar verification from InheritableThreadLocal to ThreadLocal. Existing usage and handling uses this field as temporary thread-specific provider list for jar verification. There seems to be no reason for it to be InheritableThreadLocal. Existing regression tests pass with this change. Thanks, Valerie - Commit messages: - 8253563: Change sun.security.jca.Providers.threadLists to be ThreadLocal Changes: https://git.openjdk.java.net/jdk/pull/570/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=570=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253563 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/570.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/570/head:pull/570 PR: https://git.openjdk.java.net/jdk/pull/570
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v3]
On Thu, 8 Oct 2020 13:59:20 GMT, Maurizio Cimadamore wrote: >> This patch contains the changes associated with the third incubation round >> of the foreign memory access API incubation >> (see JEP 393 [1]). This iteration focus on improving the usability of the >> API in 3 main ways: >> * first, by providing a way to obtain truly *shared* segments, which can be >> accessed and closed concurrently from >> multiple threads >> * second, by providing a way to register a memory segment against a >> `Cleaner`, so as to have some (optional) guarantee >> that the memory will be deallocated, eventually >> * third, by not requiring users to dive deep into var handles when they >> first pick up the API; a new `MemoryAccess` class >> has been added, which defines several useful dereference routines; these >> are really just thin wrappers around memory >> access var handles, but they make the barrier of entry for using this API >> somewhat lower. >> >> A big conceptual shift that comes with this API refresh is that the role of >> `MemorySegment` and `MemoryAddress` is not >> the same as it used to be; it used to be the case that a memory address >> could (sometimes, not always) have a back link >> to the memory segment which originated it; additionally, memory access var >> handles used `MemoryAddress` as a basic unit >> of dereference. This has all changed as per this API refresh; now a >> `MemoryAddress` is just a dumb carrier which >> wraps a pair of object/long addressing coordinates; `MemorySegment` has >> become the star of the show, as far as >> dereferencing memory is concerned. You cannot dereference memory if you >> don't have a segment. This improves usability >> in a number of ways - first, it is a lot easier to wrap native addresses >> (`long`, essentially) into a `MemoryAddress`; >> secondly, it is crystal clear what a client has to do in order to >> dereference memory: if a client has a segment, it can >> use that; otherwise, if the client only has an address, it will have to >> create a segment *unsafely* (this can be done >> by calling `MemoryAddress::asSegmentRestricted`). A list of the API, >> implementation and test changes is provided >> below. If you have any questions, or need more detailed explanations, I >> (and the rest of the Panama team) will be >> happy to point at existing discussions, and/or to provide the feedback >> required. A big thank to Erik Osterlund, >> Vladimir Ivanov and David Holmes, without whom the work on shared memory >> segment would not have been possible; also I'd >> like to thank Paul Sandoz, whose insights on API design have been very >> helpful in this journey. Thanks Maurizio >> Javadoc: >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html >> Specdiff: >> >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html >> >> CSR: >> >> https://bugs.openjdk.java.net/browse/JDK-8254163 >> >> >> >> ### API Changes >> >> * `MemorySegment` >> * drop factory for restricted segment (this has been moved to >> `MemoryAddress`, see below) >> * added a no-arg factory for a native restricted segment representing >> entire native heap >> * rename `withOwnerThread` to `handoff` >> * add new `share` method, to create shared segments >> * add new `registerCleaner` method, to register a segment against a cleaner >> * add more helpers to create arrays from a segment e.g. `toIntArray` >> * add some `asSlice` overloads (to make up for the fact that now segments >> are more frequently used as cursors) >> * rename `baseAddress` to `address` (so that `MemorySegment` can implement >> `Addressable`) >> * `MemoryAddress` >> * drop `segment` accessor >> * drop `rebase` method and replace it with `segmentOffset` which returns >> the offset (a `long`) of this address relative >> to a given segment >> * `MemoryAccess` >> * New class supporting several static dereference helpers; the helpers are >> organized by carrier and access mode, where a >> carrier is one of the usual suspect (a Java primitive, minus `boolean`); >> the access mode can be simple (e.g. access >> base address of given segment), or indexed, in which case the accessor >> takes a segment and either a low-level byte >> offset,or a high level logical index. The classification is reflected in >> the naming scheme (e.g. `getByte` vs. >> `getByteAtOffset` vs `getByteAtIndex`). >> * `MemoryHandles` >> * drop `withOffset` combinator >> * drop `withStride` combinator >> * the basic memory access handle factory now returns a var handle which >> takes a `MemorySegment` and a `long` - from which >> it is easy to derive all the other handles using plain var handle >> combinators. >> * `Addressable` >> * This is a new interface which is attached to entities which can be >> projected to a `MemoryAddress`. For now, both >>
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v2]
On Thu, 8 Oct 2020 01:08:44 GMT, Anthony Scarpino wrote:
>> src/java.base/share/classes/com/sun/crypto/provider/AESCipher.java line 658:
>>
>>> 656: BadPaddingException {
>>> 657: return bufferCrypt(input, output, false);
>>> 658: }
>>
>> Is the override of this method for using a different bufferCrypt impl? There
>> is also engineUpdate(ByteBuffer,
>> ByteBuffer) in CipherSpi, is there a reason for not overriding that here?
>
> Yes. thanks.. The IDE covered that up by going to the one in CipherSpi and I
> thought it was calling the AES one. TLS
> doesn't use update() so the perf numbers won't change. I'll have to run the
> tests again.
I'm not going to do the update() method, leaving it as is. There is some
complications with the Encrypt.java test were
the update is done with a direct bytebuffer, but the doFinal() is an empty
buffer, which sends it to the byte[]
doFinal(). CipherCore mitigates this situation inefficiently and I'd rather
optimize that in future changeset that I'm
already planning for byte[] methods.
-
PR: https://git.openjdk.java.net/jdk/pull/411
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v3]
On Thu, 8 Oct 2020 13:59:20 GMT, Maurizio Cimadamore wrote: >> This patch contains the changes associated with the third incubation round >> of the foreign memory access API incubation >> (see JEP 393 [1]). This iteration focus on improving the usability of the >> API in 3 main ways: >> * first, by providing a way to obtain truly *shared* segments, which can be >> accessed and closed concurrently from >> multiple threads >> * second, by providing a way to register a memory segment against a >> `Cleaner`, so as to have some (optional) guarantee >> that the memory will be deallocated, eventually >> * third, by not requiring users to dive deep into var handles when they >> first pick up the API; a new `MemoryAccess` class >> has been added, which defines several useful dereference routines; these >> are really just thin wrappers around memory >> access var handles, but they make the barrier of entry for using this API >> somewhat lower. >> >> A big conceptual shift that comes with this API refresh is that the role of >> `MemorySegment` and `MemoryAddress` is not >> the same as it used to be; it used to be the case that a memory address >> could (sometimes, not always) have a back link >> to the memory segment which originated it; additionally, memory access var >> handles used `MemoryAddress` as a basic unit >> of dereference. This has all changed as per this API refresh; now a >> `MemoryAddress` is just a dumb carrier which >> wraps a pair of object/long addressing coordinates; `MemorySegment` has >> become the star of the show, as far as >> dereferencing memory is concerned. You cannot dereference memory if you >> don't have a segment. This improves usability >> in a number of ways - first, it is a lot easier to wrap native addresses >> (`long`, essentially) into a `MemoryAddress`; >> secondly, it is crystal clear what a client has to do in order to >> dereference memory: if a client has a segment, it can >> use that; otherwise, if the client only has an address, it will have to >> create a segment *unsafely* (this can be done >> by calling `MemoryAddress::asSegmentRestricted`). A list of the API, >> implementation and test changes is provided >> below. If you have any questions, or need more detailed explanations, I >> (and the rest of the Panama team) will be >> happy to point at existing discussions, and/or to provide the feedback >> required. A big thank to Erik Osterlund, >> Vladimir Ivanov and David Holmes, without whom the work on shared memory >> segment would not have been possible; also I'd >> like to thank Paul Sandoz, whose insights on API design have been very >> helpful in this journey. Thanks Maurizio >> Javadoc: >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html >> Specdiff: >> >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html >> >> CSR: >> >> https://bugs.openjdk.java.net/browse/JDK-8254163 >> >> >> >> ### API Changes >> >> * `MemorySegment` >> * drop factory for restricted segment (this has been moved to >> `MemoryAddress`, see below) >> * added a no-arg factory for a native restricted segment representing >> entire native heap >> * rename `withOwnerThread` to `handoff` >> * add new `share` method, to create shared segments >> * add new `registerCleaner` method, to register a segment against a cleaner >> * add more helpers to create arrays from a segment e.g. `toIntArray` >> * add some `asSlice` overloads (to make up for the fact that now segments >> are more frequently used as cursors) >> * rename `baseAddress` to `address` (so that `MemorySegment` can implement >> `Addressable`) >> * `MemoryAddress` >> * drop `segment` accessor >> * drop `rebase` method and replace it with `segmentOffset` which returns >> the offset (a `long`) of this address relative >> to a given segment >> * `MemoryAccess` >> * New class supporting several static dereference helpers; the helpers are >> organized by carrier and access mode, where a >> carrier is one of the usual suspect (a Java primitive, minus `boolean`); >> the access mode can be simple (e.g. access >> base address of given segment), or indexed, in which case the accessor >> takes a segment and either a low-level byte >> offset,or a high level logical index. The classification is reflected in >> the naming scheme (e.g. `getByte` vs. >> `getByteAtOffset` vs `getByteAtIndex`). >> * `MemoryHandles` >> * drop `withOffset` combinator >> * drop `withStride` combinator >> * the basic memory access handle factory now returns a var handle which >> takes a `MemorySegment` and a `long` - from which >> it is easy to derive all the other handles using plain var handle >> combinators. >> * `Addressable` >> * This is a new interface which is attached to entities which can be >> projected to a `MemoryAddress`. For now, both >>
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v2]
On Thu, 8 Oct 2020 06:51:08 GMT, Anthony Scarpino wrote:
>> 8253821: Improve ByteBuffer performance with GCM
>
> Anthony Scarpino has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Xuelei comments
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
432:
> 430: // For input it takes the ibuffer which is wrapped in 'buffer' and
> 'src'
> 431: // from doFinal.
> 432: void doLastBlock(ByteBuffer buffer, ByteBuffer src, ByteBuffer dst)
The ordering of these new ByteBuffer-arg methods seems random? Perhaps, either
group them altogether or move them so
that they are together with the byte[] counterpart methods?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
508:
> 506: }
> 507:
> 508: int decrypt(ByteBuffer src, ByteBuffer dst) {
Similar to above. It seems a bit strange to see decrypt(...) method in between
encrypt(...) methods.
-
PR: https://git.openjdk.java.net/jdk/pull/411
Re: RFR: 8229867: Re-examine synchronization usages in http and https protocol handlers
On Thu, 8 Oct 2020 17:14:24 GMT, Alan Bateman wrote: >> Hi, >> >> This is a fix that upgrades the old HTTP and HTTPS legacy stack to use >> virtual-thread friendly locking instead of >> synchronized monitors. >> Most of the changes are mechanical - but there are still a numbers of subtle >> non-mechanical differences that are >> outlined below: >> 1. src/java.base/share/classes/sun/net/www/MessageHeader.java: >>`MessageHeader::print(PrintStream)` => synchronization modified to not >> synchronize on this while printing >>( a snapshot of the data is taken before printing instead) >> >> 2. src/java.base/share/classes/sun/net/www/MeteredStream.java: >> `MeteredStream::close` was missing synchronization: it is now protected >> by the lock >> >> 3. src/java.base/share/classes/sun/net/www/http/ChunkedOutputStream.java: >> `ChunkedOutputStream` no longer extends `PrintStream` but extends >> `OutputStream` directly. >> Extending `PrintStream` is problematic for virtual thread. After careful >> analysis, it appeared that >> `ChunkedOutputStream` didn't really need to extend `PrintStream`. >> `ChunkedOutputStream` >> is already wrapping a `PrintStream`. `ChunkedOutputStream` is never >> returned directly to user >> code but is wrapped in another stream. `ChunkedOutputStream` completely >> reimplement and >> reverse the flush logic implemented by its old super class`PrintStream` >> which leads me to believe >> there was no real reason for it to extend `PrintStream` - except for >> being able to call its `checkError` >> method - which can be done by using `instanceof ChunkedOutputStream` in >> the caller instead of >> casting to PrintStream. >> >> 4. src/java.base/share/classes/sun/net/www/http/HttpClient.java: >> Synchronization removed from `HttpClient::privilegedOpenServer` and >> replaced by an `assert`. >> There is no need for a synchronized here as the method is only called >> from a method that already >> holds the lock. >> >> 5. src/java.base/share/classes/sun/net/www/http/KeepAliveCache.java: >> Synchronization removed from `KeepAliveCache::removeVector` and replaced >> by an `assert`. >> This method is only called from methods already protected by the lock. >> Also the method has been made private. >> >> 6. src/java.base/share/classes/sun/net/www/http/KeepAliveStream.java >> `queuedForCleanup` is made volatile as it is read and written directly >> from outside the class >> and without protection by the `KeepAliveCleanerEntry`. >> Lock protection is also added to `close()`, which was missing it. >> Some methods that have no lock protection and did not need it because >> only called from >> within code blocks protected by the lock have aquired an `assert >> isLockHeldByCurrentThread();` >> >> 7. Concrete subclasses of `AuthenticationInfo` that provide an >> implementation for >> `AuthenticationInfo::setHeaders(HttpURLConnection conn, HeaderParser p, >> String raw)` have >> acquired an `assert conn.isLockheldByCurrentThread();` as the method >> should only be called >> from within a lock-protected block in `s.n.w.p.h.HttpURLConnection` >> >> 8. >> src/java.base/share/classes/sun/net/www/protocol/http/HttpURLConnection.java >> Several methods in this class have a acquired an `assert >> isLockheldByCurrentThread();` >> to help track the fact that AuthenticationInfo::setHeaders is only >> called while >> holding the lock. >> Synchronization was also removed from some method that didn't need it >> because only >> called from within code blocks protected by the lock: >> `getOutputStream0()`: synchronization removed and replaced by an `assert >> isLockheldByCurrentThread();` >> `setCookieHeader()`: synchronization removed and replaced by an `assert >> isLockheldByCurrentThread();` >> `getInputStream0()`: synchronization removed and replaced by an `assert >> isLockheldByCurrentThread();` >> `StreamingOutputStream`: small change to accomodate point 3. above >> (changes in ChunkedOutputStream). >> >> 9. >> src/java.base/share/classes/sun/net/www/protocol/http/NegotiateAuthentication.java.: >> synchronization removed from `setHeaders` and replace by an assert. See >> point 7. above. >> >> 10. >> src/java.base/unix/classes/sun/net/www/protocol/http/ntlm/NTLMAuthentication.java: >> synchronization removed from `setHeaders` and replace by an assert. See >> point 7. above. >> >> 11. >> src/java.base/windows/classes/sun/net/www/protocol/http/ntlm/NTLMAuthentication.java: >> synchronization removed from `setHeaders` and replace by an assert. See >> point 7. above. > > Mostly looks good. > > HttpClient.openServer - is there a reason to do the SM permission check while > holding the lock? > > The "closed" field in MeteredStream and ChunkedInputStream needs to be > volatile if you really want to read it without > holding the
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v2]
On Thu, 8 Oct 2020 03:21:46 GMT, Valerie Peng wrote: >> Anthony Scarpino has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Xuelei comments > > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 528: > >> 526: } >> 527: >> 528: ArrayUtil.blockSizeCheck(src.remaining(), blockSize); > > Hmm, I am not sure if this check still applies in ByteBuffer case. You are > passing the ByteBuffer objs directly from > AESCipher->CipherCore->GaloisCounterMode. This is different from the byte[] > case where CipherCore would chop up the > data into blocks and pass the blocks to the underlying FeedbackCipher impl. > Perhaps no existing regression tests covers > ByteBuffer inputs w/ non-blocksize data? Otherwise, this should be caught? > BTW, why not just use 'len' again? Seems > unnecessary to keep calling src.remaining() in various places in this method. Yes the check is unnecessary I suspect not using len was simply a mistake - PR: https://git.openjdk.java.net/jdk/pull/411
Re: RFR: 8229867: Re-examine synchronization usages in http and https protocol handlers
On Thu, 8 Oct 2020 11:22:09 GMT, Daniel Fuchs wrote: > Hi, > > This is a fix that upgrades the old HTTP and HTTPS legacy stack to use > virtual-thread friendly locking instead of > synchronized monitors. > Most of the changes are mechanical - but there are still a numbers of subtle > non-mechanical differences that are > outlined below: > 1. src/java.base/share/classes/sun/net/www/MessageHeader.java: >`MessageHeader::print(PrintStream)` => synchronization modified to not > synchronize on this while printing >( a snapshot of the data is taken before printing instead) > > 2. src/java.base/share/classes/sun/net/www/MeteredStream.java: > `MeteredStream::close` was missing synchronization: it is now protected > by the lock > > 3. src/java.base/share/classes/sun/net/www/http/ChunkedOutputStream.java: > `ChunkedOutputStream` no longer extends `PrintStream` but extends > `OutputStream` directly. > Extending `PrintStream` is problematic for virtual thread. After careful > analysis, it appeared that > `ChunkedOutputStream` didn't really need to extend `PrintStream`. > `ChunkedOutputStream` > is already wrapping a `PrintStream`. `ChunkedOutputStream` is never > returned directly to user > code but is wrapped in another stream. `ChunkedOutputStream` completely > reimplement and > reverse the flush logic implemented by its old super class`PrintStream` > which leads me to believe > there was no real reason for it to extend `PrintStream` - except for > being able to call its `checkError` > method - which can be done by using `instanceof ChunkedOutputStream` in > the caller instead of > casting to PrintStream. > > 4. src/java.base/share/classes/sun/net/www/http/HttpClient.java: > Synchronization removed from `HttpClient::privilegedOpenServer` and > replaced by an `assert`. > There is no need for a synchronized here as the method is only called > from a method that already > holds the lock. > > 5. src/java.base/share/classes/sun/net/www/http/KeepAliveCache.java: > Synchronization removed from `KeepAliveCache::removeVector` and replaced > by an `assert`. > This method is only called from methods already protected by the lock. > Also the method has been made private. > > 6. src/java.base/share/classes/sun/net/www/http/KeepAliveStream.java > `queuedForCleanup` is made volatile as it is read and written directly > from outside the class > and without protection by the `KeepAliveCleanerEntry`. > Lock protection is also added to `close()`, which was missing it. > Some methods that have no lock protection and did not need it because > only called from > within code blocks protected by the lock have aquired an `assert > isLockHeldByCurrentThread();` > > 7. Concrete subclasses of `AuthenticationInfo` that provide an implementation > for > `AuthenticationInfo::setHeaders(HttpURLConnection conn, HeaderParser p, > String raw)` have > acquired an `assert conn.isLockheldByCurrentThread();` as the method > should only be called > from within a lock-protected block in `s.n.w.p.h.HttpURLConnection` > > 8. > src/java.base/share/classes/sun/net/www/protocol/http/HttpURLConnection.java > Several methods in this class have a acquired an `assert > isLockheldByCurrentThread();` > to help track the fact that AuthenticationInfo::setHeaders is only called > while > holding the lock. > Synchronization was also removed from some method that didn't need it > because only > called from within code blocks protected by the lock: > `getOutputStream0()`: synchronization removed and replaced by an `assert > isLockheldByCurrentThread();` > `setCookieHeader()`: synchronization removed and replaced by an `assert > isLockheldByCurrentThread();` > `getInputStream0()`: synchronization removed and replaced by an `assert > isLockheldByCurrentThread();` > `StreamingOutputStream`: small change to accomodate point 3. above > (changes in ChunkedOutputStream). > > 9. > src/java.base/share/classes/sun/net/www/protocol/http/NegotiateAuthentication.java.: > synchronization removed from `setHeaders` and replace by an assert. See > point 7. above. > > 10. > src/java.base/unix/classes/sun/net/www/protocol/http/ntlm/NTLMAuthentication.java: > synchronization removed from `setHeaders` and replace by an assert. See > point 7. above. > > 11. > src/java.base/windows/classes/sun/net/www/protocol/http/ntlm/NTLMAuthentication.java: > synchronization removed from `setHeaders` and replace by an assert. See > point 7. above. Mostly looks good. HttpClient.openServer - is there a reason to do the SM permission check while holding the lock? The "closed" field in MeteredStream and ChunkedInputStream needs to be volatile if you really want to read it without holding the lock. There are a couple of places with comments that I assume you added for yourself when auditing this code.
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v2]
On Wed, 7 Oct 2020 20:56:28 GMT, Valerie Peng wrote:
>> Anthony Scarpino has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Xuelei comments
>
> src/java.base/share/classes/com/sun/crypto/provider/CipherCore.java line 1258:
>
>> 1256: } else {
>> 1257: if (buffered > 0) {
>> 1258: cipher.encrypt(buffer, 0, buffered, new byte[0], 0);
>
> Same comment as above?
So there is a problem here, but not a short buffer. I hadn't realized that
encrypt(byte[]..) didn't put data in the
GaloisCounterMode.ibuffer because I did that with encrypt(ByteBuffer..). I
added a GCM only method that will cover
this and place the data in the ibuffer for doFinal(ByteBuffer..) can complete
the op.
-
PR: https://git.openjdk.java.net/jdk/pull/411
Re: RFR: 8153005: Upgrade the default PKCS12 encryption/MAC algorithms [v2]
On Thu, 8 Oct 2020 14:21:09 GMT, Weijun Wang wrote: >> CSR updated. More description, and iteration counts lowered to 1. Will >> update code soon. > > New commit updating ic to 1. I also created separate constants for > DEFAULT_CERT_PBE_ITERATION_COUNT and > DEFAULT_KEY_PBE_ITERATION_COUNT. I haven't made the change for > LEGACY_PBE_ITERATION_COUNT since they will never change. Are you still planning, or is it possible to add a test for Windows 2019? Also, have you considered adding a test that checks if the JDK can read OpenSSL PKCS#12 files and vice versa? Maybe we can do that later as a follow-on issue. Otherwise, I will approve. - PR: https://git.openjdk.java.net/jdk/pull/473
GREASE'd ALPN values - a RFC 8701 / RFC 7301 / JEP 244 discussion
\o Hi all,
I saw that ALPN support from JEP 244 was backported to JDK8 and I've
recently had the time to take a closer look at it. For context, I'm
one of the maintainers of JSS, a NSS wrapper for Java. I've been
discussing this with another contributor, Fraser (cc'd).
One of the concerns we have with the implementation (and its exposure
in the corresponding SSLEngine/SSLSocket/SSLParameters interface) is
that protocols are passed in as Strings. However, RFC 7301 says in
section 6:
>o Identification Sequence: The precise set of octet values that
> identifies the protocol. This could be the UTF-8 encoding
> [RFC3629] of the protocol name.
When applied with GREASE'd values from RFC 8701, Strings don't work
well. In particular, most of the registered values [0] are non-UTF-8,
which can't be easily round-tripped in Java. This means that while
precise octet values are specified by IANA, they cannot be properly
specified in Java.
In particular:
byte[] desired = new byte[]{ (byte) 0xFA, (byte) 0xFA };
String encoded = new String(desired, StandardCharsets.UTF_8);
byte[] wire= encoded.getBytes(StandardCharsets.UTF_8);
String round = new String(wire, StandardCharsets.UTF_8);
fails, as does choosing US_ASCII for the encoding:
byte[] desired = new byte[]{ (byte) 0xFA, (byte) 0xFA };
String encoded = new String(desired, StandardCharsets.US_ASCII);
byte[] wire= encoded.getBytes(StandardCharsets.UTF_8);
String round = new String(wire, StandardCharsets.UTF_8);
Note that we (at the application level) can't control the final (wire
/ round-tripped) encoding to UTF_8 as this is done within the SunJSSE
implementation:
https://github.com/openjdk/jdk11u-dev/blob/master/src/java.base/share/classes/sun/security/ssl/AlpnExtension.java#L100
https://github.com/openjdk/jdk11u-dev/blob/master/src/java.base/share/classes/sun/security/ssl/AlpnExtension.java#L170
https://github.com/openjdk/jdk11u-dev/blob/master/src/java.base/share/classes/sun/security/ssl/AlpnExtension.java#L223
https://github.com/openjdk/jdk11u-dev/blob/master/src/java.base/share/classes/sun/security/ssl/AlpnExtension.java#L425
and perhaps other files I'm missing.
This decreases interoperability with other TLS implementations.
OpenSSL [1], NSS [2], and GnuTLS [3] support setting opaque blobs as
the ALPN protocol list, meaning the caller is free to supply GREASE'd
values. Go on the other hand still uses its string [4], but that
string class supports round-tripping non-UTF8 values correctly [5].
Additionally, it means that GREASE'd values sent by Java applications
aren't compliant with the RFC 8701/IANA wire values.
Is there some workaround I'm missing?
I believe that setting US_ASCII internally in SunJSSE isn't sufficient
to ensure the right wire encoding gets used. I'm thinking the only
real fix is to deprecate the String methods and provide byte[] methods
for all identifiers.
Thanks in advanced,
Alex
[0]:
https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#alpn-protocol-ids
[1]: https://www.openssl.org/docs/man1.1.0/man3/SSL_CTX_set_alpn_protos.html
[2]: https://github.com/nss-dev/nss/blob/master/lib/ssl/ssl.h#L392-L409
[3]:
https://gnutls.org/manual/html_node/Application-Layer-Protocol-Negotiation-_0028ALPN_0029.html
[4]: https://golang.org/pkg/crypto/tls/#ClientHelloInfo
[5]: https://play.golang.org/p/PjyZ-NZmKQe
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v2]
On Wed, 7 Oct 2020 20:56:06 GMT, Valerie Peng wrote:
>> Anthony Scarpino has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Xuelei comments
>
> src/java.base/share/classes/com/sun/crypto/provider/CipherCore.java line 1253:
>
>> 1251: if (decrypting) {
>> 1252: if (buffered > 0) {
>> 1253: cipher.decrypt(buffer, 0, buffered, new byte[0], 0);
>
> This looks a bit strange? The output buffer is 0-length which would lead to
> ShortBufferException when the buffered
> bytes is enough to produce some output.
This is right. decrypt() puts the data into GaloisCounterMode.ibuffer and
never uses the output
-
PR: https://git.openjdk.java.net/jdk/pull/411
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v3]
On Thu, 8 Oct 2020 13:59:20 GMT, Maurizio Cimadamore wrote: >> This patch contains the changes associated with the third incubation round >> of the foreign memory access API incubation >> (see JEP 393 [1]). This iteration focus on improving the usability of the >> API in 3 main ways: >> * first, by providing a way to obtain truly *shared* segments, which can be >> accessed and closed concurrently from >> multiple threads >> * second, by providing a way to register a memory segment against a >> `Cleaner`, so as to have some (optional) guarantee >> that the memory will be deallocated, eventually >> * third, by not requiring users to dive deep into var handles when they >> first pick up the API; a new `MemoryAccess` class >> has been added, which defines several useful dereference routines; these >> are really just thin wrappers around memory >> access var handles, but they make the barrier of entry for using this API >> somewhat lower. >> >> A big conceptual shift that comes with this API refresh is that the role of >> `MemorySegment` and `MemoryAddress` is not >> the same as it used to be; it used to be the case that a memory address >> could (sometimes, not always) have a back link >> to the memory segment which originated it; additionally, memory access var >> handles used `MemoryAddress` as a basic unit >> of dereference. This has all changed as per this API refresh; now a >> `MemoryAddress` is just a dumb carrier which >> wraps a pair of object/long addressing coordinates; `MemorySegment` has >> become the star of the show, as far as >> dereferencing memory is concerned. You cannot dereference memory if you >> don't have a segment. This improves usability >> in a number of ways - first, it is a lot easier to wrap native addresses >> (`long`, essentially) into a `MemoryAddress`; >> secondly, it is crystal clear what a client has to do in order to >> dereference memory: if a client has a segment, it can >> use that; otherwise, if the client only has an address, it will have to >> create a segment *unsafely* (this can be done >> by calling `MemoryAddress::asSegmentRestricted`). A list of the API, >> implementation and test changes is provided >> below. If you have any questions, or need more detailed explanations, I >> (and the rest of the Panama team) will be >> happy to point at existing discussions, and/or to provide the feedback >> required. A big thank to Erik Osterlund, >> Vladimir Ivanov and David Holmes, without whom the work on shared memory >> segment would not have been possible; also I'd >> like to thank Paul Sandoz, whose insights on API design have been very >> helpful in this journey. Thanks Maurizio >> Javadoc: >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html >> Specdiff: >> >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html >> >> CSR: >> >> https://bugs.openjdk.java.net/browse/JDK-8254163 >> >> >> >> ### API Changes >> >> * `MemorySegment` >> * drop factory for restricted segment (this has been moved to >> `MemoryAddress`, see below) >> * added a no-arg factory for a native restricted segment representing >> entire native heap >> * rename `withOwnerThread` to `handoff` >> * add new `share` method, to create shared segments >> * add new `registerCleaner` method, to register a segment against a cleaner >> * add more helpers to create arrays from a segment e.g. `toIntArray` >> * add some `asSlice` overloads (to make up for the fact that now segments >> are more frequently used as cursors) >> * rename `baseAddress` to `address` (so that `MemorySegment` can implement >> `Addressable`) >> * `MemoryAddress` >> * drop `segment` accessor >> * drop `rebase` method and replace it with `segmentOffset` which returns >> the offset (a `long`) of this address relative >> to a given segment >> * `MemoryAccess` >> * New class supporting several static dereference helpers; the helpers are >> organized by carrier and access mode, where a >> carrier is one of the usual suspect (a Java primitive, minus `boolean`); >> the access mode can be simple (e.g. access >> base address of given segment), or indexed, in which case the accessor >> takes a segment and either a low-level byte >> offset,or a high level logical index. The classification is reflected in >> the naming scheme (e.g. `getByte` vs. >> `getByteAtOffset` vs `getByteAtIndex`). >> * `MemoryHandles` >> * drop `withOffset` combinator >> * drop `withStride` combinator >> * the basic memory access handle factory now returns a var handle which >> takes a `MemorySegment` and a `long` - from which >> it is easy to derive all the other handles using plain var handle >> combinators. >> * `Addressable` >> * This is a new interface which is attached to entities which can be >> projected to a `MemoryAddress`. For now, both >>
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v2]
On Wed, 7 Oct 2020 20:34:21 GMT, Valerie Peng wrote:
>> Anthony Scarpino has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Xuelei comments
>
> src/java.base/share/classes/com/sun/crypto/provider/CipherCore.java line 816:
>
>> 814: }
>> 815:
>> 816: int update(ByteBuffer src, ByteBuffer dst) throws
>> ShortBufferException {
>
> Is this also one of the "GCM only" methods? If so, same comment as
> doFinal(ByteBuffer, ByteBuffer)?
> Maybe the name should be more specific to avoid misuse.
Ok.. I see what you mean by renaming the method. Yeah, I suppose it's better
since it's not truely generic
> src/java.base/share/classes/com/sun/crypto/provider/FeedbackCipher.java line
> 261:
>
>> 259:
>> 260: int encryptFinal(ByteBuffer src, ByteBuffer dst)
>> 261: throws IllegalBlockSizeException, AEADBadTagException,
>
> Tag is generated during encryption, this can't possibly throw
> AEADBadTagException, copy-n-paste error maybe?
Yep. copied decryptFinal.
-
PR: https://git.openjdk.java.net/jdk/pull/411
RFR: 8229867: Re-examine synchronization usages in http and https protocol handlers
Hi, This is a fix that upgrades the old HTTP and HTTPS legacy stack to use virtual-thread friendly locking instead of synchronized monitors. Most of the changes are mechanical - but there are still a numbers of subtle non-mechanical differences that are outlined below: 1. src/java.base/share/classes/sun/net/www/MessageHeader.java: `MessageHeader::print(PrintStream)` => synchronization modified to not synchronize on this while printing ( a snapshot of the data is taken before printing instead) 2. src/java.base/share/classes/sun/net/www/MeteredStream.java: `MeteredStream::close` was missing synchronization: it is now protected by the lock 3. src/java.base/share/classes/sun/net/www/http/ChunkedOutputStream.java: `ChunkedOutputStream` no longer extends `PrintStream` but extends `OutputStream` directly. Extending `PrintStream` is problematic for virtual thread. After careful analysis, it appeared that `ChunkedOutputStream` didn't really need to extend `PrintStream`. `ChunkedOutputStream` is already wrapping a `PrintStream`. `ChunkedOutputStream` is never returned directly to user code but is wrapped in another stream. `ChunkedOutputStream` completely reimplement and reverse the flush logic implemented by its old super class`PrintStream` which leads me to believe there was no real reason for it to extend `PrintStream` - except for being able to call its `checkError` method - which can be done by using `instanceof ChunkedOutputStream` in the caller instead of casting to PrintStream. 4. src/java.base/share/classes/sun/net/www/http/HttpClient.java: Synchronization removed from `HttpClient::privilegedOpenServer` and replaced by an `assert`. There is no need for a synchronized here as the method is only called from a method that already holds the lock. 5. src/java.base/share/classes/sun/net/www/http/KeepAliveCache.java: Synchronization removed from `KeepAliveCache::removeVector` and replaced by an `assert`. This method is only called from methods already protected by the lock. Also the method has been made private. 6. src/java.base/share/classes/sun/net/www/http/KeepAliveStream.java `queuedForCleanup` is made volatile as it is read and written directly from outside the class and without protection by the `KeepAliveCleanerEntry`. Lock protection is also added to `close()`, which was missing it. Some methods that have no lock protection and did not need it because only called from within code blocks protected by the lock have aquired an `assert isLockHeldByCurrentThread();` 7. Concrete subclasses of `AuthenticationInfo` that provide an implementation for `AuthenticationInfo::setHeaders(HttpURLConnection conn, HeaderParser p, String raw)` have acquired an `assert conn.isLockheldByCurrentThread();` as the method should only be called from within a lock-protected block in `s.n.w.p.h.HttpURLConnection` 8. src/java.base/share/classes/sun/net/www/protocol/http/HttpURLConnection.java Several methods in this class have a acquired an `assert isLockheldByCurrentThread();` to help track the fact that AuthenticationInfo::setHeaders is only called while holding the lock. Synchronization was also removed from some method that didn't need it because only called from within code blocks protected by the lock: `getOutputStream0()`: synchronization removed and replaced by an `assert isLockheldByCurrentThread();` `setCookieHeader()`: synchronization removed and replaced by an `assert isLockheldByCurrentThread();` `getInputStream0()`: synchronization removed and replaced by an `assert isLockheldByCurrentThread();` `StreamingOutputStream`: small change to accomodate point 3. above (changes in ChunkedOutputStream). 9. src/java.base/share/classes/sun/net/www/protocol/http/NegotiateAuthentication.java.: synchronization removed from `setHeaders` and replace by an assert. See point 7. above. 10. src/java.base/unix/classes/sun/net/www/protocol/http/ntlm/NTLMAuthentication.java: synchronization removed from `setHeaders` and replace by an assert. See point 7. above. 11. src/java.base/windows/classes/sun/net/www/protocol/http/ntlm/NTLMAuthentication.java: synchronization removed from `setHeaders` and replace by an assert. See point 7. above. - Commit messages: - 8229867: Re-examine synchronization usages in http and https protocol handlers Changes: https://git.openjdk.java.net/jdk/pull/558/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=558=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8229867 Stats: 1116 lines in 18 files changed: 551 ins; 149 del; 416 mod Patch: https://git.openjdk.java.net/jdk/pull/558.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/558/head:pull/558 PR: https://git.openjdk.java.net/jdk/pull/558
Re: RFR: 8153005: Upgrade the default PKCS12 encryption/MAC algorithms [v2]
On Wed, 7 Oct 2020 22:49:09 GMT, Weijun Wang wrote: >> CSR looks good. In "Sepcification" section: a typo in 'Thr iteration counts >> used by'. At the end, it describes the new >> system property will override the security properties and use the older and >> weaker algorithms, so suggest we could also >> add text about setting the iteration counts to the default legacy values. > > CSR updated. More description, and iteration counts lowered to 1. Will > update code soon. New commit updating ic to 1. I also created separate constants for DEFAULT_CERT_PBE_ITERATION_COUNT and DEFAULT_KEY_PBE_ITERATION_COUNT. I haven't made the change for LEGACY_PBE_ITERATION_COUNT since they will never change. - PR: https://git.openjdk.java.net/jdk/pull/473
Re: RFR: 8153005: Upgrade the default PKCS12 encryption/MAC algorithms [v2]
> Default algorithms are bumped to be based on PBES2 with AES-256 and SHA-256. > Please also review the CSR at > https://bugs.openjdk.java.net/browse/JDK-8228481. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: change ic to 1 - Changes: - all: https://git.openjdk.java.net/jdk/pull/473/files - new: https://git.openjdk.java.net/jdk/pull/473/files/b99611b3..6b5c5b5e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=473=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=473=00-01 Stats: 52 lines in 5 files changed: 1 ins; 1 del; 50 mod Patch: https://git.openjdk.java.net/jdk/pull/473.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/473/head:pull/473 PR: https://git.openjdk.java.net/jdk/pull/473
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v3]
> This patch contains the changes associated with the third incubation round of > the foreign memory access API incubation > (see JEP 393 [1]). This iteration focus on improving the usability of the API > in 3 main ways: > * first, by providing a way to obtain truly *shared* segments, which can be > accessed and closed concurrently from > multiple threads > * second, by providing a way to register a memory segment against a > `Cleaner`, so as to have some (optional) guarantee > that the memory will be deallocated, eventually > * third, by not requiring users to dive deep into var handles when they first > pick up the API; a new `MemoryAccess` class > has been added, which defines several useful dereference routines; these > are really just thin wrappers around memory > access var handles, but they make the barrier of entry for using this API > somewhat lower. > > A big conceptual shift that comes with this API refresh is that the role of > `MemorySegment` and `MemoryAddress` is not > the same as it used to be; it used to be the case that a memory address could > (sometimes, not always) have a back link > to the memory segment which originated it; additionally, memory access var > handles used `MemoryAddress` as a basic unit > of dereference. This has all changed as per this API refresh; now a > `MemoryAddress` is just a dumb carrier which > wraps a pair of object/long addressing coordinates; `MemorySegment` has > become the star of the show, as far as > dereferencing memory is concerned. You cannot dereference memory if you don't > have a segment. This improves usability > in a number of ways - first, it is a lot easier to wrap native addresses > (`long`, essentially) into a `MemoryAddress`; > secondly, it is crystal clear what a client has to do in order to dereference > memory: if a client has a segment, it can > use that; otherwise, if the client only has an address, it will have to > create a segment *unsafely* (this can be done > by calling `MemoryAddress::asSegmentRestricted`). A list of the API, > implementation and test changes is provided > below. If you have any questions, or need more detailed explanations, I (and > the rest of the Panama team) will be > happy to point at existing discussions, and/or to provide the feedback > required. A big thank to Erik Osterlund, > Vladimir Ivanov and David Holmes, without whom the work on shared memory > segment would not have been possible; also I'd > like to thank Paul Sandoz, whose insights on API design have been very > helpful in this journey. Thanks Maurizio > Javadoc: > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html > Specdiff: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254163 > > > > ### API Changes > > * `MemorySegment` > * drop factory for restricted segment (this has been moved to > `MemoryAddress`, see below) > * added a no-arg factory for a native restricted segment representing > entire native heap > * rename `withOwnerThread` to `handoff` > * add new `share` method, to create shared segments > * add new `registerCleaner` method, to register a segment against a cleaner > * add more helpers to create arrays from a segment e.g. `toIntArray` > * add some `asSlice` overloads (to make up for the fact that now segments > are more frequently used as cursors) > * rename `baseAddress` to `address` (so that `MemorySegment` can implement > `Addressable`) > * `MemoryAddress` > * drop `segment` accessor > * drop `rebase` method and replace it with `segmentOffset` which returns > the offset (a `long`) of this address relative > to a given segment > * `MemoryAccess` > * New class supporting several static dereference helpers; the helpers are > organized by carrier and access mode, where a > carrier is one of the usual suspect (a Java primitive, minus `boolean`); > the access mode can be simple (e.g. access > base address of given segment), or indexed, in which case the accessor > takes a segment and either a low-level byte > offset,or a high level logical index. The classification is reflected in > the naming scheme (e.g. `getByte` vs. > `getByteAtOffset` vs `getByteAtIndex`). > * `MemoryHandles` > * drop `withOffset` combinator > * drop `withStride` combinator > * the basic memory access handle factory now returns a var handle which > takes a `MemorySegment` and a `long` - from which > it is easy to derive all the other handles using plain var handle > combinators. > * `Addressable` > * This is a new interface which is attached to entities which can be > projected to a `MemoryAddress`. For now, both > `MemoryAddress` and `MemorySegment` implement it; we have plans, with JEP > 389 [2] to add more implementations. Clients > can largely ignore this interface, which
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v2]
On Thu, 8 Oct 2020 12:54:12 GMT, Erik Joelsson wrote: >> Maurizio Cimadamore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address review comments > > make/modules/java.base/gensrc/GensrcScopedMemoryAccess.gmk line 145: > >> 143: SCOPE_MEMORY_ACCESS_TYPES := Byte Short Char Int Long Float Double >> 144: $(foreach t, $(SCOPE_MEMORY_ACCESS_TYPES), \ >> 145: $(eval $(call GenerateScopedOp,BIN_$t,$t))) > > This indent was fine at 2 spaces. I meant the one below inside the recipe. Gotcha - I fixed the wrong foreach... - PR: https://git.openjdk.java.net/jdk/pull/548
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v2]
On Thu, 8 Oct 2020 10:29:24 GMT, Maurizio Cimadamore wrote: >> This patch contains the changes associated with the third incubation round >> of the foreign memory access API incubation >> (see JEP 393 [1]). This iteration focus on improving the usability of the >> API in 3 main ways: >> * first, by providing a way to obtain truly *shared* segments, which can be >> accessed and closed concurrently from >> multiple threads >> * second, by providing a way to register a memory segment against a >> `Cleaner`, so as to have some (optional) guarantee >> that the memory will be deallocated, eventually >> * third, by not requiring users to dive deep into var handles when they >> first pick up the API; a new `MemoryAccess` class >> has been added, which defines several useful dereference routines; these >> are really just thin wrappers around memory >> access var handles, but they make the barrier of entry for using this API >> somewhat lower. >> >> A big conceptual shift that comes with this API refresh is that the role of >> `MemorySegment` and `MemoryAddress` is not >> the same as it used to be; it used to be the case that a memory address >> could (sometimes, not always) have a back link >> to the memory segment which originated it; additionally, memory access var >> handles used `MemoryAddress` as a basic unit >> of dereference. This has all changed as per this API refresh; now a >> `MemoryAddress` is just a dumb carrier which >> wraps a pair of object/long addressing coordinates; `MemorySegment` has >> become the star of the show, as far as >> dereferencing memory is concerned. You cannot dereference memory if you >> don't have a segment. This improves usability >> in a number of ways - first, it is a lot easier to wrap native addresses >> (`long`, essentially) into a `MemoryAddress`; >> secondly, it is crystal clear what a client has to do in order to >> dereference memory: if a client has a segment, it can >> use that; otherwise, if the client only has an address, it will have to >> create a segment *unsafely* (this can be done >> by calling `MemoryAddress::asSegmentRestricted`). A list of the API, >> implementation and test changes is provided >> below. If you have any questions, or need more detailed explanations, I >> (and the rest of the Panama team) will be >> happy to point at existing discussions, and/or to provide the feedback >> required. A big thank to Erik Osterlund, >> Vladimir Ivanov and David Holmes, without whom the work on shared memory >> segment would not have been possible; also I'd >> like to thank Paul Sandoz, whose insights on API design have been very >> helpful in this journey. Thanks Maurizio >> Javadoc: >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html >> Specdiff: >> >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html >> >> CSR: >> >> https://bugs.openjdk.java.net/browse/JDK-8254163 >> >> >> >> ### API Changes >> >> * `MemorySegment` >> * drop factory for restricted segment (this has been moved to >> `MemoryAddress`, see below) >> * added a no-arg factory for a native restricted segment representing >> entire native heap >> * rename `withOwnerThread` to `handoff` >> * add new `share` method, to create shared segments >> * add new `registerCleaner` method, to register a segment against a cleaner >> * add more helpers to create arrays from a segment e.g. `toIntArray` >> * add some `asSlice` overloads (to make up for the fact that now segments >> are more frequently used as cursors) >> * rename `baseAddress` to `address` (so that `MemorySegment` can implement >> `Addressable`) >> * `MemoryAddress` >> * drop `segment` accessor >> * drop `rebase` method and replace it with `segmentOffset` which returns >> the offset (a `long`) of this address relative >> to a given segment >> * `MemoryAccess` >> * New class supporting several static dereference helpers; the helpers are >> organized by carrier and access mode, where a >> carrier is one of the usual suspect (a Java primitive, minus `boolean`); >> the access mode can be simple (e.g. access >> base address of given segment), or indexed, in which case the accessor >> takes a segment and either a low-level byte >> offset,or a high level logical index. The classification is reflected in >> the naming scheme (e.g. `getByte` vs. >> `getByteAtOffset` vs `getByteAtIndex`). >> * `MemoryHandles` >> * drop `withOffset` combinator >> * drop `withStride` combinator >> * the basic memory access handle factory now returns a var handle which >> takes a `MemorySegment` and a `long` - from which >> it is easy to derive all the other handles using plain var handle >> combinators. >> * `Addressable` >> * This is a new interface which is attached to entities which can be >> projected to a `MemoryAddress`. For now, both >>
Integrated: 8242882: opening jar file with large manifest might throw NegativeArraySizeException
On Wed, 23 Sep 2020 15:06:55 GMT, Jaikiran Pai wrote: > Can I please get a review and a sponsor for a fix for > https://bugs.openjdk.java.net/browse/JDK-8242882? > > As noted in that JBS issue, if the size of the Manifest entry in the jar > happens to be very large (such that it exceeds > the `Integer.MAX_VALUE`), then the current code in `JarFile#getBytes` can > lead to a `NegativeArraySizeException`. This > is due to the: if (len != -1 && len <= 65535) block which evaluates to > `true` when the size of the manifest entry is > larger than `Integer.MAX_VALUE`. As a result, this then ends up calling the > code which can lead to the > `NegativeArraySizeException`. The commit in this PR fixes that issue by > changing those `if/else` blocks to prevent > this issue and instead use a code path that leads to the > `InputStream#readAllBytes()` which internally has the > necessary checks to throw the expected `OutOfMemoryError`. This commit also > includes a jtreg test case which > reproduces the issue and verifies the fix. This pull request has now been integrated. Changeset: 782d45bd Author:Jaikiran Pai Committer: Lance Andersen URL: https://git.openjdk.java.net/jdk/commit/782d45bd Stats: 85 lines in 2 files changed: 84 ins; 0 del; 1 mod 8242882: opening jar file with large manifest might throw NegativeArraySizeException Reviewed-by: bchristi, lancea - PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException [v3]
On Thu, 1 Oct 2020 14:42:21 GMT, Jaikiran Pai wrote: >> Can I please get a review and a sponsor for a fix for >> https://bugs.openjdk.java.net/browse/JDK-8242882? >> >> As noted in that JBS issue, if the size of the Manifest entry in the jar >> happens to be very large (such that it exceeds >> the `Integer.MAX_VALUE`), then the current code in `JarFile#getBytes` can >> lead to a `NegativeArraySizeException`. This >> is due to the: if (len != -1 && len <= 65535) block which evaluates to >> `true` when the size of the manifest entry is >> larger than `Integer.MAX_VALUE`. As a result, this then ends up calling the >> code which can lead to the >> `NegativeArraySizeException`. The commit in this PR fixes that issue by >> changing those `if/else` blocks to prevent >> this issue and instead use a code path that leads to the >> `InputStream#readAllBytes()` which internally has the >> necessary checks to throw the expected `OutOfMemoryError`. This commit also >> includes a jtreg test case which >> reproduces the issue and verifies the fix. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Second round of review comments addressed Hi Jaikiran, Yes I think you are OK to move forward with the integration, - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v2]
On Thu, 8 Oct 2020 10:29:24 GMT, Maurizio Cimadamore wrote: >> This patch contains the changes associated with the third incubation round >> of the foreign memory access API incubation >> (see JEP 393 [1]). This iteration focus on improving the usability of the >> API in 3 main ways: >> * first, by providing a way to obtain truly *shared* segments, which can be >> accessed and closed concurrently from >> multiple threads >> * second, by providing a way to register a memory segment against a >> `Cleaner`, so as to have some (optional) guarantee >> that the memory will be deallocated, eventually >> * third, by not requiring users to dive deep into var handles when they >> first pick up the API; a new `MemoryAccess` class >> has been added, which defines several useful dereference routines; these >> are really just thin wrappers around memory >> access var handles, but they make the barrier of entry for using this API >> somewhat lower. >> >> A big conceptual shift that comes with this API refresh is that the role of >> `MemorySegment` and `MemoryAddress` is not >> the same as it used to be; it used to be the case that a memory address >> could (sometimes, not always) have a back link >> to the memory segment which originated it; additionally, memory access var >> handles used `MemoryAddress` as a basic unit >> of dereference. This has all changed as per this API refresh; now a >> `MemoryAddress` is just a dumb carrier which >> wraps a pair of object/long addressing coordinates; `MemorySegment` has >> become the star of the show, as far as >> dereferencing memory is concerned. You cannot dereference memory if you >> don't have a segment. This improves usability >> in a number of ways - first, it is a lot easier to wrap native addresses >> (`long`, essentially) into a `MemoryAddress`; >> secondly, it is crystal clear what a client has to do in order to >> dereference memory: if a client has a segment, it can >> use that; otherwise, if the client only has an address, it will have to >> create a segment *unsafely* (this can be done >> by calling `MemoryAddress::asSegmentRestricted`). A list of the API, >> implementation and test changes is provided >> below. If you have any questions, or need more detailed explanations, I >> (and the rest of the Panama team) will be >> happy to point at existing discussions, and/or to provide the feedback >> required. A big thank to Erik Osterlund, >> Vladimir Ivanov and David Holmes, without whom the work on shared memory >> segment would not have been possible; also I'd >> like to thank Paul Sandoz, whose insights on API design have been very >> helpful in this journey. Thanks Maurizio >> Javadoc: >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html >> Specdiff: >> >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html >> >> CSR: >> >> https://bugs.openjdk.java.net/browse/JDK-8254163 >> >> >> >> ### API Changes >> >> * `MemorySegment` >> * drop factory for restricted segment (this has been moved to >> `MemoryAddress`, see below) >> * added a no-arg factory for a native restricted segment representing >> entire native heap >> * rename `withOwnerThread` to `handoff` >> * add new `share` method, to create shared segments >> * add new `registerCleaner` method, to register a segment against a cleaner >> * add more helpers to create arrays from a segment e.g. `toIntArray` >> * add some `asSlice` overloads (to make up for the fact that now segments >> are more frequently used as cursors) >> * rename `baseAddress` to `address` (so that `MemorySegment` can implement >> `Addressable`) >> * `MemoryAddress` >> * drop `segment` accessor >> * drop `rebase` method and replace it with `segmentOffset` which returns >> the offset (a `long`) of this address relative >> to a given segment >> * `MemoryAccess` >> * New class supporting several static dereference helpers; the helpers are >> organized by carrier and access mode, where a >> carrier is one of the usual suspect (a Java primitive, minus `boolean`); >> the access mode can be simple (e.g. access >> base address of given segment), or indexed, in which case the accessor >> takes a segment and either a low-level byte >> offset,or a high level logical index. The classification is reflected in >> the naming scheme (e.g. `getByte` vs. >> `getByteAtOffset` vs `getByteAtIndex`). >> * `MemoryHandles` >> * drop `withOffset` combinator >> * drop `withStride` combinator >> * the basic memory access handle factory now returns a var handle which >> takes a `MemorySegment` and a `long` - from which >> it is easy to derive all the other handles using plain var handle >> combinators. >> * `Addressable` >> * This is a new interface which is attached to entities which can be >> projected to a `MemoryAddress`. For now, both >>
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v2]
> This patch contains the changes associated with the third incubation round of > the foreign memory access API incubation > (see JEP 393 [1]). This iteration focus on improving the usability of the API > in 3 main ways: > * first, by providing a way to obtain truly *shared* segments, which can be > accessed and closed concurrently from > multiple threads > * second, by providing a way to register a memory segment against a > `Cleaner`, so as to have some (optional) guarantee > that the memory will be deallocated, eventually > * third, by not requiring users to dive deep into var handles when they first > pick up the API; a new `MemoryAccess` class > has been added, which defines several useful dereference routines; these > are really just thin wrappers around memory > access var handles, but they make the barrier of entry for using this API > somewhat lower. > > A big conceptual shift that comes with this API refresh is that the role of > `MemorySegment` and `MemoryAddress` is not > the same as it used to be; it used to be the case that a memory address could > (sometimes, not always) have a back link > to the memory segment which originated it; additionally, memory access var > handles used `MemoryAddress` as a basic unit > of dereference. This has all changed as per this API refresh; now a > `MemoryAddress` is just a dumb carrier which > wraps a pair of object/long addressing coordinates; `MemorySegment` has > become the star of the show, as far as > dereferencing memory is concerned. You cannot dereference memory if you don't > have a segment. This improves usability > in a number of ways - first, it is a lot easier to wrap native addresses > (`long`, essentially) into a `MemoryAddress`; > secondly, it is crystal clear what a client has to do in order to dereference > memory: if a client has a segment, it can > use that; otherwise, if the client only has an address, it will have to > create a segment *unsafely* (this can be done > by calling `MemoryAddress::asSegmentRestricted`). A list of the API, > implementation and test changes is provided > below. If you have any questions, or need more detailed explanations, I (and > the rest of the Panama team) will be > happy to point at existing discussions, and/or to provide the feedback > required. A big thank to Erik Osterlund, > Vladimir Ivanov and David Holmes, without whom the work on shared memory > segment would not have been possible; also I'd > like to thank Paul Sandoz, whose insights on API design have been very > helpful in this journey. Thanks Maurizio > Javadoc: > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html > Specdiff: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254163 > > > > ### API Changes > > * `MemorySegment` > * drop factory for restricted segment (this has been moved to > `MemoryAddress`, see below) > * added a no-arg factory for a native restricted segment representing > entire native heap > * rename `withOwnerThread` to `handoff` > * add new `share` method, to create shared segments > * add new `registerCleaner` method, to register a segment against a cleaner > * add more helpers to create arrays from a segment e.g. `toIntArray` > * add some `asSlice` overloads (to make up for the fact that now segments > are more frequently used as cursors) > * rename `baseAddress` to `address` (so that `MemorySegment` can implement > `Addressable`) > * `MemoryAddress` > * drop `segment` accessor > * drop `rebase` method and replace it with `segmentOffset` which returns > the offset (a `long`) of this address relative > to a given segment > * `MemoryAccess` > * New class supporting several static dereference helpers; the helpers are > organized by carrier and access mode, where a > carrier is one of the usual suspect (a Java primitive, minus `boolean`); > the access mode can be simple (e.g. access > base address of given segment), or indexed, in which case the accessor > takes a segment and either a low-level byte > offset,or a high level logical index. The classification is reflected in > the naming scheme (e.g. `getByte` vs. > `getByteAtOffset` vs `getByteAtIndex`). > * `MemoryHandles` > * drop `withOffset` combinator > * drop `withStride` combinator > * the basic memory access handle factory now returns a var handle which > takes a `MemorySegment` and a `long` - from which > it is easy to derive all the other handles using plain var handle > combinators. > * `Addressable` > * This is a new interface which is attached to entities which can be > projected to a `MemoryAddress`. For now, both > `MemoryAddress` and `MemorySegment` implement it; we have plans, with JEP > 389 [2] to add more implementations. Clients > can largely ignore this interface, which
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v2]
On Thu, 8 Oct 2020 06:53:41 GMT, Aleksey Shipilev wrote: >> Maurizio Cimadamore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address review comments > > test/jdk/java/foreign/TestMismatch.java line 26: > >> 24: /* >> 25: * @test >> 26: * @run testng/othervm -XX:MaxDirectMemorySize=50 TestMismatch > > Whoa, allocating 5 GB? That might fail on 32-bit platforms... Anyhow, this > flag accepts suffixes, so > `-XX:MaxDirectMemorySize=5g`. I've done two things here: * the limit isn't really doing much in this test, so I've removed * I moved the limit in TestSegments; the limit is set to much lower threshold (2M) which should work regardless of 32/64 * For TestMismatch, which needs to allocate a segment bigger than 2^32 in one of the tests, I've added a guard in the offending test which verifies that we're indeed on a 64-bit platform - PR: https://git.openjdk.java.net/jdk/pull/548
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator)
On Wed, 7 Oct 2020 17:13:22 GMT, Maurizio Cimadamore wrote: > This patch contains the changes associated with the third incubation round of > the foreign memory access API incubation > (see JEP 393 [1]). This iteration focus on improving the usability of the API > in 3 main ways: > * first, by providing a way to obtain truly *shared* segments, which can be > accessed and closed concurrently from > multiple threads > * second, by providing a way to register a memory segment against a > `Cleaner`, so as to have some (optional) guarantee > that the memory will be deallocated, eventually > * third, by not requiring users to dive deep into var handles when they first > pick up the API; a new `MemoryAccess` class > has been added, which defines several useful dereference routines; these > are really just thin wrappers around memory > access var handles, but they make the barrier of entry for using this API > somewhat lower. > > A big conceptual shift that comes with this API refresh is that the role of > `MemorySegment` and `MemoryAddress` is not > the same as it used to be; it used to be the case that a memory address could > (sometimes, not always) have a back link > to the memory segment which originated it; additionally, memory access var > handles used `MemoryAddress` as a basic unit > of dereference. This has all changed as per this API refresh; now a > `MemoryAddress` is just a dumb carrier which > wraps a pair of object/long addressing coordinates; `MemorySegment` has > become the star of the show, as far as > dereferencing memory is concerned. You cannot dereference memory if you don't > have a segment. This improves usability > in a number of ways - first, it is a lot easier to wrap native addresses > (`long`, essentially) into a `MemoryAddress`; > secondly, it is crystal clear what a client has to do in order to dereference > memory: if a client has a segment, it can > use that; otherwise, if the client only has an address, it will have to > create a segment *unsafely* (this can be done > by calling `MemoryAddress::asSegmentRestricted`). A list of the API, > implementation and test changes is provided > below. If you have any questions, or need more detailed explanations, I (and > the rest of the Panama team) will be > happy to point at existing discussions, and/or to provide the feedback > required. A big thank to Erik Osterlund, > Vladimir Ivanov and David Holmes, without whom the work on shared memory > segment would not have been possible; also I'd > like to thank Paul Sandoz, whose insights on API design have been very > helpful in this journey. Thanks Maurizio > Javadoc: > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html > Specdiff: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254163 > > > > ### API Changes > > * `MemorySegment` > * drop factory for restricted segment (this has been moved to > `MemoryAddress`, see below) > * added a no-arg factory for a native restricted segment representing > entire native heap > * rename `withOwnerThread` to `handoff` > * add new `share` method, to create shared segments > * add new `registerCleaner` method, to register a segment against a cleaner > * add more helpers to create arrays from a segment e.g. `toIntArray` > * add some `asSlice` overloads (to make up for the fact that now segments > are more frequently used as cursors) > * rename `baseAddress` to `address` (so that `MemorySegment` can implement > `Addressable`) > * `MemoryAddress` > * drop `segment` accessor > * drop `rebase` method and replace it with `segmentOffset` which returns > the offset (a `long`) of this address relative > to a given segment > * `MemoryAccess` > * New class supporting several static dereference helpers; the helpers are > organized by carrier and access mode, where a > carrier is one of the usual suspect (a Java primitive, minus `boolean`); > the access mode can be simple (e.g. access > base address of given segment), or indexed, in which case the accessor > takes a segment and either a low-level byte > offset,or a high level logical index. The classification is reflected in > the naming scheme (e.g. `getByte` vs. > `getByteAtOffset` vs `getByteAtIndex`). > * `MemoryHandles` > * drop `withOffset` combinator > * drop `withStride` combinator > * the basic memory access handle factory now returns a var handle which > takes a `MemorySegment` and a `long` - from which > it is easy to derive all the other handles using plain var handle > combinators. > * `Addressable` > * This is a new interface which is attached to entities which can be > projected to a `MemoryAddress`. For now, both > `MemoryAddress` and `MemorySegment` implement it; we have plans, with JEP > 389 [2] to add more
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v2]
On Wed, 7 Oct 2020 19:42:11 GMT, Valerie Peng wrote: >> src/java.base/share/classes/com/sun/crypto/provider/AESCipher.java line 664: >> >>> 662: * engineUpdate() and engineDoFinal(). >>> 663: */ >>> 664: private int bufferCrypt(ByteBuffer input, ByteBuffer output, >> >> It looks like this method is copied from the CipherSpi. For maintenance, it >> would be nice to add an extra comment to >> state the copy and update. For example, "this method and implementation is >> copied from javax.crypto.CipherSpi, with an >> improvement for GCM mode." > > Agree w/ Xuelei, it'd be nice to mention CipherSpi.bufferCrypt() method. In > case a bug is found, it'd be checked and > fixed in both places. Yeah.. good point. - PR: https://git.openjdk.java.net/jdk/pull/411
Re: RFR: 8153005: Upgrade the default PKCS12 encryption/MAC algorithms
On Wed, 7 Oct 2020 22:08:19 GMT, Hai-May Chao wrote: >> Default algorithms are bumped to be based on PBES2 with AES-256 and SHA-256. >> Please also review the CSR at >> https://bugs.openjdk.java.net/browse/JDK-8228481. > > Looks good. Only minor comments. CSR looks good. In "Sepcification" section: a typo in 'Thr iteration counts used by'. At the end, it describes the new system property will override the security properties and use the older and weaker algorithms, so suggest we could also add text about setting the iteration counts to the default legacy values. - PR: https://git.openjdk.java.net/jdk/pull/473
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v2]
On Thu, 8 Oct 2020 03:21:46 GMT, Anthony Scarpino wrote:
>> 8253821: Improve ByteBuffer performance with GCM
>
> Anthony Scarpino has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Xuelei comments
src/java.base/share/classes/com/sun/crypto/provider/AESCipher.java line 658:
> 656: BadPaddingException {
> 657: return bufferCrypt(input, output, false);
> 658: }
Is the override of this method for using a different bufferCrypt impl? There is
also engineUpdate(ByteBuffer,
ByteBuffer) in CipherSpi, is there a reason for not overriding that here?
src/java.base/share/classes/com/sun/crypto/provider/AESCipher.java line 744:
> 742: } else {
> 743: return core.doFinal(input, output);
> 744: }
It seems this block is the only difference between this method and
CipherSpi.bufferCrypt(). Have you considered moving
this special handling to the overridden engineDoFinal(...) method and not
duplicating the whole CipherSpi.bufferCrypt()
method here? BTW, instead of using the generic update/doFinal name and then
commenting them for GCM usage only, perhaps
it's more enticing to name them as gcmUpdate/gcmDoFinal?
src/java.base/share/classes/com/sun/crypto/provider/CipherCore.java line 820:
> 818: return cipher.decrypt(src, dst);
> 819: }
> 820: return cipher.encrypt(src, dst);
How about return (decrypting? cipher.decrypt(src, dst) : cipher.encrypt(src,
dst));
src/java.base/share/classes/com/sun/crypto/provider/CipherCore.java line 816:
> 814: }
> 815:
> 816: int update(ByteBuffer src, ByteBuffer dst) throws
> ShortBufferException {
Is this also one of the "GCM only" methods? If so, same comment as
doFinal(ByteBuffer, ByteBuffer)?
Maybe the name should be more specific to avoid misuse.
src/java.base/share/classes/com/sun/crypto/provider/FeedbackCipher.java line
261:
> 259:
> 260: int encryptFinal(ByteBuffer src, ByteBuffer dst)
> 261: throws IllegalBlockSizeException, AEADBadTagException,
Tag is generated during encryption, this can't possibly throw
AEADBadTagException, copy-n-paste error maybe?
src/java.base/share/classes/com/sun/crypto/provider/CipherCore.java line 1253:
> 1251: if (decrypting) {
> 1252: if (buffered > 0) {
> 1253: cipher.decrypt(buffer, 0, buffered, new byte[0], 0);
This looks a bit strange? The output buffer is 0-length which would lead to
ShortBufferException when the buffered
bytes is enough to produce some output.
src/java.base/share/classes/com/sun/crypto/provider/CipherCore.java line 1258:
> 1256: } else {
> 1257: if (buffered > 0) {
> 1258: cipher.encrypt(buffer, 0, buffered, new byte[0], 0);
Same comment as above?
src/java.base/share/classes/com/sun/crypto/provider/CipherCore.java line 939:
> 937: // if the size of specified output buffer is less than
> 938: // the length of the cipher text, then the current
> 939: // content of cipher has to be preserved in order for
This change is somewhat dangerous. When using the supplied output buffer
directly, you may corrupt its content w/
padding bytes. This optimization should only be applied when no padding is
involved. In addition, input and output can
point to the same buffer with all sorts of index combinations, i.e. inOfs ==
outOfs, inOfs < outOfs, inOfs > outOfs.
With "outWithPadding" approach, no need to check. However, for
"internalOutput", data corruption may happen. This kind
of problem can be very hard to diagnose. Have to be very very careful here as
this code may impact all ciphers...
src/java.base/share/classes/com/sun/crypto/provider/GCTR.java line 240:
> 238: }
> 239: }
> 240: }
See the above red icon? It warns about missing newline at end of this file.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
528:
> 526: }
> 527:
> 528: ArrayUtil.blockSizeCheck(src.remaining(), blockSize);
Hmm, I am not sure if this check still applies in ByteBuffer case. You are
passing the ByteBuffer objs directly from
AESCipher->CipherCore->GaloisCounterMode. This is different from the byte[]
case where CipherCore would chop up the
data into blocks and pass the blocks to the underlying FeedbackCipher impl.
Perhaps no existing regression tests covers
ByteBuffer inputs w/ non-blocksize data? Otherwise, this should be caught? BTW,
why not just use 'len' again? Seems
unnecessary to keep calling src.remaining() in various places in this method.
-
PR: https://git.openjdk.java.net/jdk/pull/411
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v2]
On Wed, 7 Oct 2020 16:29:32 GMT, Xue-Lei Andrew Fan wrote:
>> Anthony Scarpino has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Xuelei comments
>
> src/java.base/share/classes/com/sun/crypto/provider/CipherCore.java line 947:
>
>> 945: // create temporary output buffer if the estimated size is
>> larger
>> 946: // than the user-provided buffer.
>> 947: if (output.length - outputOffset < estOutSize) {
>
> "outputCapacity" could be used to replace "output.length - outputOffset", and
> join the clause with the if-clause above.
interesting.. That makes this and the if condition below it able to join again.
> src/java.base/share/classes/com/sun/crypto/provider/CounterMode.java line 28:
>
>> 26: package com.sun.crypto.provider;
>> 27:
>> 28: import java.nio.ByteBuffer;
>
> There is no more update except this line. The new import ByteBuffer may not
> be used.
Thanks. That is another new thing to get used to with git and multiple
commits. When cleaning up code it's easy to
not see changes like this.
-
PR: https://git.openjdk.java.net/jdk/pull/411
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException [v2]
On Wed, 7 Oct 2020 21:40:43 GMT, Brent Christian wrote: >> I decided to slightly change the way this large manifest file was being >> created. I borrowed the idea from >> `Zip64SizeTest`[1] to create the file and set its length to a large value. I >> hope that is OK. If not, let me know, I >> will change this part. [1] >> https://github.com/openjdk/jdk/blob/master/test/jdk/java/util/zip/ZipFile/Zip64SizeTest.java#L121 > > I did some automated test runs, and the duration of this test is sufficiently > improved, IMO. > > While not representative of a real MANIFEST.MF file, I think it works well > enough for this specific test. Thank you for the review Brent. - PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v2]
On Wed, 7 Oct 2020 16:17:38 GMT, Xue-Lei Andrew Fan wrote: >> Anthony Scarpino has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Xuelei comments > > src/java.base/share/classes/com/sun/crypto/provider/AESCipher.java line 664: > >> 662: * engineUpdate() and engineDoFinal(). >> 663: */ >> 664: private int bufferCrypt(ByteBuffer input, ByteBuffer output, > > It looks like this method is copied from the CipherSpi. For maintenance, it > would be nice to add an extra comment to > state the copy and update. For example, "this method and implementation is > copied from javax.crypto.CipherSpi, with an > improvement for GCM mode." Agree w/ Xuelei, it'd be nice to mention CipherSpi.bufferCrypt() method. In case a bug is found, it'd be checked and fixed in both places. - PR: https://git.openjdk.java.net/jdk/pull/411
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v2]
On Wed, 7 Oct 2020 22:38:21 GMT, Valerie Peng wrote:
>> Anthony Scarpino has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Xuelei comments
>
> src/java.base/share/classes/com/sun/crypto/provider/CipherCore.java line 939:
>
>> 937: // if the size of specified output buffer is less than
>> 938: // the length of the cipher text, then the current
>> 939: // content of cipher has to be preserved in order for
>
> This change is somewhat dangerous. When using the supplied output buffer
> directly, you may corrupt its content w/
> padding bytes. This optimization should only be applied when no padding is
> involved. In addition, input and output can
> point to the same buffer with all sorts of index combinations, i.e. inOfs ==
> outOfs, inOfs < outOfs, inOfs > outOfs.
> With "outWithPadding" approach, no need to check. However, for
> "internalOutput", data corruption may happen. This kind
> of problem can be very hard to diagnose. Have to be very very careful here as
> this code may impact all ciphers...
- I do not understand where the corruption comes from. The user provides a
buffer that output is suppose to be placed
into, what could it be corrupting? The existing tests (SameBuffer, in
particular) works fine with this and the
ByteBuffer calls. I spent a lot of time trying to get those same buffer
tests to pass.
- It was my expectation that checkOutputCapacity() is making sure all the inOfs
==<> outOfs are going to work. Does that
not catch all cases?
- outWithPadding" is excessive because it doubles the allocation for every
operation followed by a copy to the original
buffer, even if the original buffer was adequate. I'm ok with doing the
extra alloc & copy in certain situations, but
not everytime. Can you be more specific what things may fail that we don't
already check for in the regression tests?
> src/java.base/share/classes/com/sun/crypto/provider/AESCipher.java line 658:
>
>> 656: BadPaddingException {
>> 657: return bufferCrypt(input, output, false);
>> 658: }
>
> Is the override of this method for using a different bufferCrypt impl? There
> is also engineUpdate(ByteBuffer,
> ByteBuffer) in CipherSpi, is there a reason for not overriding that here?
Yes. thanks.. The IDE covered that up by going to the one in CipherSpi and I
thought it was calling the AES one. TLS
doesn't use update() so the perf numbers won't change. I'll have to run the
tests again.
> src/java.base/share/classes/com/sun/crypto/provider/AESCipher.java line 744:
>
>> 742: } else {
>> 743: return core.doFinal(input, output);
>> 744: }
>
> It seems this block is the only difference between this method and
> CipherSpi.bufferCrypt(). Have you considered moving
> this special handling to the overridden engineDoFinal(...) method and not
> duplicating the whole CipherSpi.bufferCrypt()
> method here? BTW, instead of using the generic update/doFinal name and then
> commenting them for GCM usage only, perhaps
> it's more enticing to name them as gcmUpdate/gcmDoFinal?
I didn't see a way to override this because CipherSpi is a public class, any
methods I added would become a new API.
Also bufferCrypt is private, so I had to copy it. CipherSpi does not know
which mode is being used, but AESCipher
does. Maybe I'm missing something, I'd rather it not be a copy, but I
couldn't see a better way. If you have a
specific idea, please give me details.
-
PR: https://git.openjdk.java.net/jdk/pull/411
Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v2]
> 8253821: Improve ByteBuffer performance with GCM Anthony Scarpino has updated the pull request incrementally with one additional commit since the last revision: Xuelei comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/411/files - new: https://git.openjdk.java.net/jdk/pull/411/files/3a3a0296..b29c1ed5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=411=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=411=00-01 Stats: 17 lines in 6 files changed: 6 ins; 7 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/411.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/411/head:pull/411 PR: https://git.openjdk.java.net/jdk/pull/411
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException [v3]
On Wed, 7 Oct 2020 21:40:57 GMT, Brent Christian wrote: >> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Second round of review comments addressed > > Marked as reviewed by bchristi (Reviewer). Hello Lance, does the latest state of this PR look fine to you? If so, shall I trigger a integrate? - PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException [v2]
On Thu, 1 Oct 2020 14:39:50 GMT, Jaikiran Pai wrote:
>> test/jdk/java/util/jar/JarFile/LargeManifestOOMTest.java line 78:
>>
>>> 76: bw.write("OOM-Test: ");
>>> 77: for (long i = 0; i < 2147483648L; i++) {
>>> 78: bw.write("a");
>>
>> As you probably noticed, this test takes a little while to run. One way to
>> speed it up a little would be to write more
>> characters at a time. While we're at it, we may as well make the Manifest
>> well-formed by breaking it into 72-byte
>> lines. See "Line length" under:
>> https://docs.oracle.com/en/java/javase/15/docs/specs/jar/jar.html#notes-on-manifest-and-signature-files
>> Just write
>> enough lines to exceed Integer.MAX_VALUE bytes.
>
> I decided to slightly change the way this large manifest file was being
> created. I borrowed the idea from
> `Zip64SizeTest`[1] to create the file and set its length to a large value. I
> hope that is OK. If not, let me know, I
> will change this part. [1]
> https://github.com/openjdk/jdk/blob/master/test/jdk/java/util/zip/ZipFile/Zip64SizeTest.java#L121
I did some automated test runs, and the duration of this test is sufficiently
improved, IMO.
While not representative of a real MANIFEST.MF file, I think it works well
enough for this specific test.
-
PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException [v3]
On Thu, 1 Oct 2020 14:42:21 GMT, Jaikiran Pai wrote: >> Can I please get a review and a sponsor for a fix for >> https://bugs.openjdk.java.net/browse/JDK-8242882? >> >> As noted in that JBS issue, if the size of the Manifest entry in the jar >> happens to be very large (such that it exceeds >> the `Integer.MAX_VALUE`), then the current code in `JarFile#getBytes` can >> lead to a `NegativeArraySizeException`. This >> is due to the: if (len != -1 && len <= 65535) block which evaluates to >> `true` when the size of the manifest entry is >> larger than `Integer.MAX_VALUE`. As a result, this then ends up calling the >> code which can lead to the >> `NegativeArraySizeException`. The commit in this PR fixes that issue by >> changing those `if/else` blocks to prevent >> this issue and instead use a code path that leads to the >> `InputStream#readAllBytes()` which internally has the >> necessary checks to throw the expected `OutOfMemoryError`. This commit also >> includes a jtreg test case which >> reproduces the issue and verifies the fix. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Second round of review comments addressed Marked as reviewed by bchristi (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR: 8153005: Upgrade the default PKCS12 encryption/MAC algorithms
On Wed, 7 Oct 2020 22:20:07 GMT, Hai-May Chao wrote: >> Looks good. Only minor comments. > > CSR looks good. In "Sepcification" section: a typo in 'Thr iteration counts > used by'. At the end, it describes the new > system property will override the security properties and use the older and > weaker algorithms, so suggest we could also > add text about setting the iteration counts to the default legacy values. CSR updated. More description, and iteration counts lowered to 1. Will update code soon. - PR: https://git.openjdk.java.net/jdk/pull/473
Re: RFR: 8153005: Upgrade the default PKCS12 encryption/MAC algorithms
On Thu, 1 Oct 2020 20:02:34 GMT, Weijun Wang wrote:
> Default algorithms are bumped to be based on PBES2 with AES-256 and SHA-256.
> Please also review the CSR at
> https://bugs.openjdk.java.net/browse/JDK-8228481.
Looks good. Only minor comments.
src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 103:
> 101: = "PBEWithHmacSHA256AndAES_256";
> 102: private static final String DEFAULT_MAC_ALGORITHM = "HmacPBESHA256";
> 103: private static final int DEFAULT_PBE_ITERATION_COUNT = 5;
As we have keystore.pkcs12.certPbeIterationCount and
keystore.pkcs12.keyPbeIterationCount, I would like to suggest that
we can define DEFAULT_CERT_PBE_ITERATION_COUNT and
DEFAULT_KEY_PBE_ITERATION_COUNT, specifying each of the values for
finer granularity. Same for LEGACY_PBE_ITERATION_COUNT.
test/jdk/sun/security/mscapi/VeryLongAlias.java line 48:
> 46:
> 47: static String alias = String.format("%0512d", new
> Random().nextInt(10));
> 48:
Add bug number to @bug.
-
PR: https://git.openjdk.java.net/jdk/pull/473
Re: RFR: 8153005: Upgrade the default PKCS12 encryption/MAC algorithms
On Wed, 7 Oct 2020 22:06:28 GMT, Hai-May Chao wrote:
>> Default algorithms are bumped to be based on PBES2 with AES-256 and SHA-256.
>> Please also review the CSR at
>> https://bugs.openjdk.java.net/browse/JDK-8228481.
>
> test/jdk/sun/security/mscapi/VeryLongAlias.java line 48:
>
>> 46:
>> 47: static String alias = String.format("%0512d", new
>> Random().nextInt(10));
>> 48:
>
> Add bug number to @bug.
OK.
-
PR: https://git.openjdk.java.net/jdk/pull/473
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator)
On Wed, 7 Oct 2020 17:13:22 GMT, Maurizio Cimadamore wrote: > This patch contains the changes associated with the third incubation round of > the foreign memory access API incubation > (see JEP 393 [1]). This iteration focus on improving the usability of the API > in 3 main ways: > * first, by providing a way to obtain truly *shared* segments, which can be > accessed and closed concurrently from > multiple threads > * second, by providing a way to register a memory segment against a > `Cleaner`, so as to have some (optional) guarantee > that the memory will be deallocated, eventually > * third, by not requiring users to dive deep into var handles when they first > pick up the API; a new `MemoryAccess` class > has been added, which defines several useful dereference routines; these > are really just thin wrappers around memory > access var handles, but they make the barrier of entry for using this API > somewhat lower. > > A big conceptual shift that comes with this API refresh is that the role of > `MemorySegment` and `MemoryAddress` is not > the same as it used to be; it used to be the case that a memory address could > (sometimes, not always) have a back link > to the memory segment which originated it; additionally, memory access var > handles used `MemoryAddress` as a basic unit > of dereference. This has all changed as per this API refresh; now a > `MemoryAddress` is just a dumb carrier which > wraps a pair of object/long addressing coordinates; `MemorySegment` has > become the star of the show, as far as > dereferencing memory is concerned. You cannot dereference memory if you don't > have a segment. This improves usability > in a number of ways - first, it is a lot easier to wrap native addresses > (`long`, essentially) into a `MemoryAddress`; > secondly, it is crystal clear what a client has to do in order to dereference > memory: if a client has a segment, it can > use that; otherwise, if the client only has an address, it will have to > create a segment *unsafely* (this can be done > by calling `MemoryAddress::asSegmentRestricted`). A list of the API, > implementation and test changes is provided > below. If you have any questions, or need more detailed explanations, I (and > the rest of the Panama team) will be > happy to point at existing discussions, and/or to provide the feedback > required. A big thank to Erik Osterlund, > Vladimir Ivanov and David Holmes, without whom the work on shared memory > segment would not have been possible; also I'd > like to thank Paul Sandoz, whose insights on API design have been very > helpful in this journey. Thanks Maurizio > Javadoc: > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html > Specdiff: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254163 > > > > ### API Changes > > * `MemorySegment` > * drop factory for restricted segment (this has been moved to > `MemoryAddress`, see below) > * added a no-arg factory for a native restricted segment representing > entire native heap > * rename `withOwnerThread` to `handoff` > * add new `share` method, to create shared segments > * add new `registerCleaner` method, to register a segment against a cleaner > * add more helpers to create arrays from a segment e.g. `toIntArray` > * add some `asSlice` overloads (to make up for the fact that now segments > are more frequently used as cursors) > * rename `baseAddress` to `address` (so that `MemorySegment` can implement > `Addressable`) > * `MemoryAddress` > * drop `segment` accessor > * drop `rebase` method and replace it with `segmentOffset` which returns > the offset (a `long`) of this address relative > to a given segment > * `MemoryAccess` > * New class supporting several static dereference helpers; the helpers are > organized by carrier and access mode, where a > carrier is one of the usual suspect (a Java primitive, minus `boolean`); > the access mode can be simple (e.g. access > base address of given segment), or indexed, in which case the accessor > takes a segment and either a low-level byte > offset,or a high level logical index. The classification is reflected in > the naming scheme (e.g. `getByte` vs. > `getByteAtOffset` vs `getByteAtIndex`). > * `MemoryHandles` > * drop `withOffset` combinator > * drop `withStride` combinator > * the basic memory access handle factory now returns a var handle which > takes a `MemorySegment` and a `long` - from which > it is easy to derive all the other handles using plain var handle > combinators. > * `Addressable` > * This is a new interface which is attached to entities which can be > projected to a `MemoryAddress`. For now, both > `MemoryAddress` and `MemorySegment` implement it; we have plans, with JEP > 389 [2] to add more
