RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v13]
Alan Bateman
alanb at openjdk.org
Thu Nov 24 12:17:06 UTC 2022
On Thu, 24 Nov 2022 11:42:52 GMT, Per Minborg <pminborg at openjdk.org> 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:
>
> Remove redundant method
src/java.base/share/classes/sun/nio/ch/DatagramChannelImpl.java line 790:
> 788: try {
> 789: int n = receive0(fd,
> 790: ((DirectBuffer)bb).address() + pos, rem,
Would it be possible to restore the original alignment, just to make it easier to read.
src/java.base/share/classes/sun/nio/ch/DirectBuffer.java line 39:
> 37: // silent unrelated memory mutation and JVM crashes.
> 38: //
> 39: // Guards are available in the JavaNioAccess class.
Did you mean to leave the word "Guards" in the comment? I guess I would say something "See JavaNioAccess for methods to acquire/release" or something like that.
src/java.base/share/classes/sun/nio/ch/IOUtil.java line 480:
> 478: static MemorySessionImpl acquireScope(ByteBuffer bb, boolean async) {
> 479: if (async && NIO_ACCESS.isThreadConfined(bb)) {
> 480: throw new IllegalStateException("Confined session not supported");
No sure about ISE here. There is no mutable state on the input that would allow the acquire to succeed. I don't think anyone has run into it yet but we will need to look at the AsynchronousXXXX read/write methods to decide which exception to specify.
src/java.base/share/classes/sun/nio/ch/IOUtil.java line 543:
> 541: @Override public void run() { releaseScope(bb, session); }
> 542: static Runnable of(ByteBuffer bb, MemorySessionImpl session) { return new Releaser(bb, session); }
> 543: static Runnable ofNullable(ByteBuffer bb, MemorySessionImpl session) {
Would it be possible to re-format this to make it more readable? There's just a bit more going on compared to the original and it's harder to read.
-------------
PR: https://git.openjdk.org/jdk/pull/11260
More information about the security-dev
mailing list