Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
On Tue, 22 Nov 2022 13:49:45 GMT, Per Minborg wrote:
>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
>> line 914:
>>
>>> 912: * If so, make a copy to put the dst data in.
>>> 913: */
>>> 914: @SuppressWarnings("try")
>>
>> After looking at the implementation some more, I'm not sure this need
>> fixing? E.g. this method is just using the address to compute some overlap -
>> and return a buffer sliced accordingly. There's no access to the buffer data
>> (except for the last part which does a `put`). The access will fail if the
>> session is closed from underneath. I don't think this can crash the VM (in
>> fact this code did not have a reachability fence to begin with).
>
> Well spotted. I will remove the guarding here.
This `@SuppressWarnings` annotation is no longer needed:
Suggestion:
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
On Tue, 22 Nov 2022 09:23:40 GMT, Maurizio Cimadamore
wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Rework Acquisition
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
> line 914:
>
>> 912: * If so, make a copy to put the dst data in.
>> 913: */
>> 914: @SuppressWarnings("try")
>
> After looking at the implementation some more, I'm not sure this need fixing?
> E.g. this method is just using the address to compute some overlap - and
> return a buffer sliced accordingly. There's no access to the buffer data
> (except for the last part which does a `put`). The access will fail if the
> session is closed from underneath. I don't think this can crash the VM (in
> fact this code did not have a reachability fence to begin with).
Well spotted. I will remove the guarding here.
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
On Tue, 22 Nov 2022 09:29:14 GMT, Maurizio Cimadamore
wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Rework Acquisition
>
> src/java.base/share/classes/java/nio/Buffer.java line 827:
>
>> 825:
>> 826: @Override
>> 827: public Runnable acquireSessionOrNull(Buffer buffer,
>> boolean async) {
>
> If allocation/performance is an issue, a relatively straightforward way to
> adjust the code would be to let go of the TWR entirely. E.g. we have code
> that does this:
>
>
> Buffer b = ...
> try {
> // use buffer.address();
> } finally {
> Reference.reachabilityFence(b);
> }
>
>
> We could transform to this:
>
>
> Buffer b = ...
> acquire(b); // calls MemorySessionImpl.acquire0 if session is not null
> try {
> // use buffer.address();
> } finally {
> release(b); // calls MemorySessionImpl.release0 if session is not null
> (and maybe adds a reachability fence, just in case)
> }
>
> This leads to a simpler patch that is probably easier to validate. The
> remaining IOUtil code will be using a different idiom, and perhaps we can, at
> some point, go back and make that consistent too.
The AutoCloseable experiment was interesting to try but I think you are right,
acquire try { .. } finally release would be simpler and also remove the
concerns about the performance critical code paths.
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
On Tue, 22 Nov 2022 09:38:35 GMT, Maurizio Cimadamore
wrote:
>> src/jdk.sctp/unix/classes/sun/nio/ch/sctp/SctpMultiChannelImpl.java line 590:
>>
>>> 588: int pos)
>>> 589: throws IOException {
>>> 590: try (var guard = NIO_ACCESS.acquireScope(bb)) {
>>
>> Why was the old code not using reachability fences? Bug or feature?
>
> I see that there's a subsequent buffer call if `n > 0`, so that's probably
> why the fence was skipped? (I also assume that the code calling this method
> will access the buffer before/after, so reachability is never truly an issue
> - but for session-backed buffers this needs fixing).
>
> Also, stepping back, I note how, if `receive0` was a native call using
> Linker, perhaps we wouldn't need all this manual address computation - we'd
> just get a memory segment slice from the buffer and pass that to the handle
> (which will perform the correct liveness check). E.g. maybe a better long
> term solution would be to panama-ize this code?
Yes, once the memory/linker APIs are permanent then the SCTP implementation
would be a good candidate to redo.
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
On Tue, 22 Nov 2022 09:11:44 GMT, Per Minborg wrote:
>> This PR proposes the introduction of **guarding** of the use of
>> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
>> Function and Memory access, it is possible to derive Buffer instances that
>> are backed by native memory that, in turn, can be closed asynchronously by
>> the user (and not only via a `Cleaner` when there is no other reference to
>> the `Buffer` object). If another thread is invoking `MemorySession::close`
>> while a thread is doing work using raw addresses, the outcome is undefined.
>> This means the JVM might crash or even worse, silent modification of
>> unrelated memory might occur.
>>
>> Design choices in this PR:
>>
>> There is already a method `MemorySession::whileAlive` that takes a runnable
>> and that will perform the provided action while acquiring the underlying`
>> MemorySession` (if any). However, using this method entailed relatively
>> large changes while converting larger/nested code segments into lambdas.
>> This would also risk introducing lambda capturing. So, instead, a
>> try-with-resources friendly access method was added. This made is more easy
>> to add guarding and did not risk lambda capturing. Also, introducing lambdas
>> in certain fundamental JDK classes might incur bootstrap problems.
>>
>> The aforementioned TwR is using a "session acquisition" that is not used
>> explicitly in the try block itself. This raises a warning that is suppressed
>> using `@SuppressWarnings("try")`. In the future, we might be able to remove
>> these suppressions by using the reserved variable name `_`. Another
>> alternative was evaluated where a non-autocloseable resource was used.
>> However, it became relatively complicated to guarantee that the session was
>> always released and, at the same time, the existing 'final` block was always
>> executed properly (as they both might throw exceptions). In the end, the
>> proposed solution was much simpler and robust despite requiring a
>> non-referenced TwR variable.
>>
>> In some cases, where is is guaranteed that the backing memory session is
>> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
>> ~~These cases have been documented in the code.~~
>>
>> On some occasions, a plurality of acquisitions are made. This would never
>> lead to deadlocks as acquisitions are fundamentally concurrent counters and
>> not resources that only one thread can "own".
>>
>> I have added comments (and not javadocs) before the declaration of the
>> non-public-api `DirectBuffer::address` method, that the use of the returned
>> address needs to be guarded. It can be discussed if this is preferable or
>> not.
>>
>> This PR spawns several areas of responsibility and so, I expect more than
>> one reviewer before promoting the PR.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Rework Acquisition
src/java.base/share/classes/sun/nio/ch/DirectBuffer.java line 43:
> 41: // try (var guard = NIO_ACCESS.acquireSession(bb)) {
> 42: // performOperation(bb.address());
> 43: // }
Again, updating this comment was missed:
Suggestion:
// try (var guard = NIO_ACCESS.acquireScope(bb)) {
// performOperation(guard.address());
// }
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
On Tue, 22 Nov 2022 09:32:32 GMT, Maurizio Cimadamore
wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Rework Acquisition
>
> src/jdk.sctp/unix/classes/sun/nio/ch/sctp/SctpMultiChannelImpl.java line 590:
>
>> 588: int pos)
>> 589: throws IOException {
>> 590: try (var guard = NIO_ACCESS.acquireScope(bb)) {
>
> Why was the old code not using reachability fences? Bug or feature?
I see that there's a subsequent buffer call if `n > 0`, so that's probably why
the fence was skipped? (I also assume that the code calling this method will
access the buffer before/after, so reachability is never truly an issue - but
for session-backed buffers this needs fixing).
Also, stepping back, I note how, if `receive0` was a native call using Linker,
perhaps we wouldn't need all this manual address computation - we'd just get a
memory segment slice from the buffer and pass that to the handle (which will
perform the correct liveness check). E.g. maybe a better long term solution
would be to panama-ize this code?
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
On Tue, 22 Nov 2022 09:11:44 GMT, Per Minborg wrote:
>> This PR proposes the introduction of **guarding** of the use of
>> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
>> Function and Memory access, it is possible to derive Buffer instances that
>> are backed by native memory that, in turn, can be closed asynchronously by
>> the user (and not only via a `Cleaner` when there is no other reference to
>> the `Buffer` object). If another thread is invoking `MemorySession::close`
>> while a thread is doing work using raw addresses, the outcome is undefined.
>> This means the JVM might crash or even worse, silent modification of
>> unrelated memory might occur.
>>
>> Design choices in this PR:
>>
>> There is already a method `MemorySession::whileAlive` that takes a runnable
>> and that will perform the provided action while acquiring the underlying`
>> MemorySession` (if any). However, using this method entailed relatively
>> large changes while converting larger/nested code segments into lambdas.
>> This would also risk introducing lambda capturing. So, instead, a
>> try-with-resources friendly access method was added. This made is more easy
>> to add guarding and did not risk lambda capturing. Also, introducing lambdas
>> in certain fundamental JDK classes might incur bootstrap problems.
>>
>> The aforementioned TwR is using a "session acquisition" that is not used
>> explicitly in the try block itself. This raises a warning that is suppressed
>> using `@SuppressWarnings("try")`. In the future, we might be able to remove
>> these suppressions by using the reserved variable name `_`. Another
>> alternative was evaluated where a non-autocloseable resource was used.
>> However, it became relatively complicated to guarantee that the session was
>> always released and, at the same time, the existing 'final` block was always
>> executed properly (as they both might throw exceptions). In the end, the
>> proposed solution was much simpler and robust despite requiring a
>> non-referenced TwR variable.
>>
>> In some cases, where is is guaranteed that the backing memory session is
>> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
>> ~~These cases have been documented in the code.~~
>>
>> On some occasions, a plurality of acquisitions are made. This would never
>> lead to deadlocks as acquisitions are fundamentally concurrent counters and
>> not resources that only one thread can "own".
>>
>> I have added comments (and not javadocs) before the declaration of the
>> non-public-api `DirectBuffer::address` method, that the use of the returned
>> address needs to be guarded. It can be discussed if this is preferable or
>> not.
>>
>> This PR spawns several areas of responsibility and so, I expect more than
>> one reviewer before promoting the PR.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Rework Acquisition
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
914:
> 912: * If so, make a copy to put the dst data in.
> 913: */
> 914: @SuppressWarnings("try")
After looking at the implementation some more, I'm not sure this need fixing?
E.g. this method is just using the address to compute some overlap - and return
a buffer sliced accordingly. There's no access to the buffer data (except for
the last part which does a `put`). The access will fail if the session is
closed from underneath. I don't think this can crash the VM (in fact this code
did not have a reachability fence to begin with).
src/java.base/share/classes/java/nio/Buffer.java line 827:
> 825:
> 826: @Override
> 827: public Runnable acquireSessionOrNull(Buffer buffer,
> boolean async) {
If allocation/performance is an issue, a relatively straightforward way to
adjust the code would be to let go of the TWR entirely. E.g. we have code that
does this:
Buffer b = ...
try {
// use buffer.address();
} finally {
Reference.reachabilityFence(b);
}
We could transform to this:
Buffer b = ...
acquire(b); // calls MemorySessionImpl.acquire0 if session is not null
try {
// use buffer.address();
} finally {
release(b); // calls MemorySessionImpl.release0 if session is not null (and
maybe adds a reachability fence, just in case)
}
This leads to a simpler patch that is probably easier to validate. The
remaining IOUtil code will be using a different idiom, and perhaps we can, at
some point, go back and make that consistent too.
src/jdk.sctp/unix/classes/sun/nio/ch/sctp/SctpMultiChannelImpl.java line 590:
> 588: int pos)
> 589: throws IOException {
> 590: try (var guard = NIO_ACCESS.acquireScope(bb)) {
Why was the old code not using reachability fences? Bug or feature?
-
PR:
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
> This PR proposes the introduction of **guarding** of the use of
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
> Function and Memory access, it is possible to derive Buffer instances that
> are backed by native memory that, in turn, can be closed asynchronously by
> the user (and not only via a `Cleaner` when there is no other reference to
> the `Buffer` object). If another thread is invoking `MemorySession::close`
> while a thread is doing work using raw addresses, the outcome is undefined.
> This means the JVM might crash or even worse, silent modification of
> unrelated memory might occur.
>
> Design choices in this PR:
>
> There is already a method `MemorySession::whileAlive` that takes a runnable
> and that will perform the provided action while acquiring the underlying`
> MemorySession` (if any). However, using this method entailed relatively large
> changes while converting larger/nested code segments into lambdas. This would
> also risk introducing lambda capturing. So, instead, a try-with-resources
> friendly access method was added. This made is more easy to add guarding and
> did not risk lambda capturing. Also, introducing lambdas in certain
> fundamental JDK classes might incur bootstrap problems.
>
> The aforementioned TwR is using a "session acquisition" that is not used
> explicitly in the try block itself. This raises a warning that is suppressed
> using `@SuppressWarnings("try")`. In the future, we might be able to remove
> these suppressions by using the reserved variable name `_`. Another
> alternative was evaluated where a non-autocloseable resource was used.
> However, it became relatively complicated to guarantee that the session was
> always released and, at the same time, the existing 'final` block was always
> executed properly (as they both might throw exceptions). In the end, the
> proposed solution was much simpler and robust despite requiring a
> non-referenced TwR variable.
>
> In some cases, where is is guaranteed that the backing memory session is
> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
> ~~These cases have been documented in the code.~~
>
> On some occasions, a plurality of acquisitions are made. This would never
> lead to deadlocks as acquisitions are fundamentally concurrent counters and
> not resources that only one thread can "own".
>
> I have added comments (and not javadocs) before the declaration of the
> non-public-api `DirectBuffer::address` method, that the use of the returned
> address needs to be guarded. It can be discussed if this is preferable or not.
>
> This PR spawns several areas of responsibility and so, I expect more than one
> reviewer before promoting the PR.
Per Minborg has updated the pull request incrementally with one additional
commit since the last revision:
Rework Acquisition
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11260/files
- new: https://git.openjdk.org/jdk/pull/11260/files/88ed3aa2..0526e3dc
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=11260=08
- incr: https://webrevs.openjdk.org/?repo=jdk=11260=07-08
Stats: 249 lines in 19 files changed: 66 ins; 91 del; 92 mod
Patch: https://git.openjdk.org/jdk/pull/11260.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260
PR: https://git.openjdk.org/jdk/pull/11260
