Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

8296024: Usage of DirectBuffer::address should be guarded #11260

Closed
wants to merge 19 commits into from
Closed

8296024: Usage of DirectBuffer::address should be guarded #11260

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
23a6fd1
Guard usages of DirectBuffer::address
minborg Nov 21, 2022
9fafafa
Cleanup after comments
minborg Nov 21, 2022
9fcf2bb
Remove comments
minborg Nov 21, 2022
c081b4a
Rename methods
minborg Nov 21, 2022
eca7c38
Cleanup static declarations
minborg Nov 21, 2022
17a72e9
Update src/java.base/share/classes/sun/nio/ch/DirectBuffer.java
minborg Nov 21, 2022
a0de3bc
Update src/java.base/share/classes/jdk/internal/access/JavaNioAccess.…
minborg Nov 21, 2022
7097934
Remove comments
minborg Nov 21, 2022
88ed3aa
Merge branch 'guard-directbuffer-address' of https://github.com/minbo…
minborg Nov 21, 2022
0526e3d
Rework Acquisition
minborg Nov 22, 2022
06c764c
Remove redundant guard and fix comment permanently
minborg Nov 22, 2022
88ae68b
Refactor to try-finally handling
minborg Nov 23, 2022
682b6f5
Cleanup
minborg Nov 23, 2022
0546b23
Remove redundant method
minborg Nov 24, 2022
30aff11
Reformat and fix comment
minborg Nov 24, 2022
95c4058
Re-introduce address variables
minborg Nov 28, 2022
eedf41a
Re-introduce yet another address vairable
minborg Nov 28, 2022
619473e
Remove session exposure
minborg Nov 29, 2022
cd95bc8
Merge master
minborg Dec 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 43 additions & 32 deletions src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@

package com.sun.crypto.provider;

import jdk.internal.access.JavaNioAccess;
import jdk.internal.access.SharedSecrets;
import jdk.internal.misc.Unsafe;
import sun.nio.ch.DirectBuffer;
import sun.security.jca.JCAUtil;
Expand Down Expand Up @@ -92,6 +94,8 @@ abstract class GaloisCounterMode extends CipherSpi {

static final byte[] EMPTY_BUF = new byte[0];

private static final JavaNioAccess NIO_ACCESS = SharedSecrets.getJavaNioAccess();

private boolean initialized = false;

SymmetricCipher blockCipher;
Expand Down Expand Up @@ -907,44 +911,49 @@ int doLastBlock(GCMOperation op, ByteBuffer buffer, ByteBuffer src,
* and if dst will overwrite src data before src can be processed.
* If so, make a copy to put the dst data in.
*/
@SuppressWarnings("try")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well spotted. I will remove the guarding here.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This @SuppressWarnings annotation is no longer needed:

Suggested change
@SuppressWarnings("try")

ByteBuffer overlapDetection(ByteBuffer src, ByteBuffer dst) {
if (src.isDirect() && dst.isDirect()) {
DirectBuffer dsrc = (DirectBuffer) src;
DirectBuffer ddst = (DirectBuffer) dst;

// Get the current memory address for the given ByteBuffers
long srcaddr = dsrc.address();
long dstaddr = ddst.address();
try (var srcGuard = NIO_ACCESS.acquireScope(src);
var dstGuard = NIO_ACCESS.acquireScope(dst)) {

// Find the lowest attachment that is the base memory address
// of the shared memory for the src object
while (dsrc.attachment() != null) {
srcaddr = ((DirectBuffer) dsrc.attachment()).address();
dsrc = (DirectBuffer) dsrc.attachment();
}
// Get the current memory address for the given ByteBuffers
long srcaddr = dsrc.address();
long dstaddr = ddst.address();

// Find the lowest attachment that is the base memory address
// of the shared memory for the dst object
while (ddst.attachment() != null) {
dstaddr = ((DirectBuffer) ddst.attachment()).address();
ddst = (DirectBuffer) ddst.attachment();
}
// Find the lowest attachment that is the base memory address
// of the shared memory for the src object
while (dsrc.attachment() != null) {
srcaddr = ((DirectBuffer) dsrc.attachment()).address();
dsrc = (DirectBuffer) dsrc.attachment();
}

// If the base addresses are not the same, there is no overlap
if (srcaddr != dstaddr) {
return dst;
}
// At this point we know these objects share the same memory.
// This checks the starting position of the src and dst address
// for overlap.
// It uses the base address minus the passed object's address to
// get the offset from the base address, then add the position()
// from the passed object. That gives up the true offset from
// the base address. As long as the src side is >= the dst
// side, we are not in overlap.
if (((DirectBuffer) src).address() - srcaddr + src.position() >=
((DirectBuffer) dst).address() - dstaddr + dst.position()) {
return dst;
// Find the lowest attachment that is the base memory address
// of the shared memory for the dst object
while (ddst.attachment() != null) {
dstaddr = ((DirectBuffer) ddst.attachment()).address();
ddst = (DirectBuffer) ddst.attachment();
}

// If the base addresses are not the same, there is no overlap
if (srcaddr != dstaddr) {
return dst;
}
// At this point we know these objects share the same memory.
// This checks the starting position of the src and dst address
// for overlap.
// It uses the base address minus the passed object's address to
// get the offset from the base address, then add the position()
// from the passed object. That gives up the true offset from
// the base address. As long as the src side is >= the dst
// side, we are not in overlap.
if (((DirectBuffer) src).address() - srcaddr + src.position() >=
((DirectBuffer) dst).address() - dstaddr + dst.position()) {
return dst;
}
}

} else if (!src.isDirect() && !dst.isDirect()) {
Expand Down Expand Up @@ -1585,8 +1594,10 @@ public int doFinal(ByteBuffer src, ByteBuffer dst)
int ofs = dst.arrayOffset() + dst.position();
Arrays.fill(dst.array(), ofs , ofs + len, (byte)0);
} else {
Unsafe.getUnsafe().setMemory(((DirectBuffer)dst).address(),
len + dst.position(), (byte)0);
try (var guard = NIO_ACCESS.acquireScope(dst)) {
Unsafe.getUnsafe().setMemory(guard.address(),
len + dst.position(), (byte) 0);
}
}
throw new AEADBadTagException("Tag mismatch");
}
Expand Down
13 changes: 12 additions & 1 deletion src/java.base/share/classes/java/nio/Buffer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,7 @@ final void checkSession() {
// setup access to this package in SharedSecrets
SharedSecrets.setJavaNioAccess(
new JavaNioAccess() {

@Override
public BufferPool getDirectBufferPool() {
return Bits.BUFFER_POOL;
Expand Down Expand Up @@ -823,7 +824,7 @@ public MemorySegment bufferSegment(Buffer buffer) {
}

@Override
public Runnable acquireSession(Buffer buffer, boolean async) {
public Runnable acquireSessionOrNull(Buffer buffer, boolean async) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var session = buffer.session();
if (session == null) {
return null;
Expand All @@ -835,6 +836,16 @@ public Runnable acquireSession(Buffer buffer, boolean async) {
return session::release0;
}

@Override
public ScopeAcquisition acquireScope(Buffer buffer) {
var scope = buffer.session();
if (scope == null) {
return ScopeAcquisition.create(buffer);
}
scope.acquire0();
return ScopeAcquisition.create(buffer, scope);
}

@Override
public void force(FileDescriptor fd, long address, boolean isSync, long offset, long size) {
MappedMemoryUtils.force(fd, address, isSync, offset, size);
Expand Down
11 changes: 5 additions & 6 deletions src/java.base/share/classes/java/util/zip/Adler32.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,13 @@

package java.util.zip;

import java.lang.ref.Reference;
import java.nio.ByteBuffer;
import sun.nio.ch.DirectBuffer;

import jdk.internal.util.Preconditions;
import jdk.internal.vm.annotation.IntrinsicCandidate;

import static java.util.zip.ZipUtils.NIO_ACCESS;

/**
* A class that can be used to compute the Adler-32 checksum of a data
* stream. An Adler-32 checksum is almost as reliable as a CRC-32 but
Expand Down Expand Up @@ -96,10 +97,8 @@ public void update(ByteBuffer buffer) {
if (rem <= 0)
return;
if (buffer.isDirect()) {
try {
adler = updateByteBuffer(adler, ((DirectBuffer)buffer).address(), pos, rem);
} finally {
Reference.reachabilityFence(buffer);
try (var guard = NIO_ACCESS.acquireScope(buffer)) {
adler = updateByteBuffer(adler, guard.address(), pos, rem);
}
} else if (buffer.hasArray()) {
adler = updateBytes(adler, buffer.array(), pos + buffer.arrayOffset(), rem);
Expand Down
10 changes: 4 additions & 6 deletions src/java.base/share/classes/java/util/zip/CRC32.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@

package java.util.zip;

import java.lang.ref.Reference;
import java.nio.ByteBuffer;
import java.util.Objects;

import sun.nio.ch.DirectBuffer;
import jdk.internal.util.Preconditions;
import jdk.internal.vm.annotation.IntrinsicCandidate;

import static java.util.zip.ZipUtils.NIO_ACCESS;

/**
* A class that can be used to compute the CRC-32 of a data stream.
*
Expand Down Expand Up @@ -96,10 +96,8 @@ public void update(ByteBuffer buffer) {
if (rem <= 0)
return;
if (buffer.isDirect()) {
try {
crc = updateByteBuffer(crc, ((DirectBuffer)buffer).address(), pos, rem);
} finally {
Reference.reachabilityFence(buffer);
try (var guard = NIO_ACCESS.acquireScope(buffer)) {
crc = updateByteBuffer(crc, guard.address(), pos, rem);
}
} else if (buffer.hasArray()) {
crc = updateBytes(crc, buffer.array(), pos + buffer.arrayOffset(), rem);
Expand Down
10 changes: 4 additions & 6 deletions src/java.base/share/classes/java/util/zip/CRC32C.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,14 @@
*/
package java.util.zip;

import java.lang.ref.Reference;
import java.nio.ByteBuffer;
import java.nio.ByteOrder;

import jdk.internal.misc.Unsafe;
import jdk.internal.util.Preconditions;
import jdk.internal.vm.annotation.IntrinsicCandidate;
import sun.nio.ch.DirectBuffer;

import static java.util.zip.ZipUtils.NIO_ACCESS;

/**
* A class that can be used to compute the CRC-32C of a data stream.
Expand Down Expand Up @@ -171,11 +171,9 @@ public void update(ByteBuffer buffer) {
}

if (buffer.isDirect()) {
try {
crc = updateDirectByteBuffer(crc, ((DirectBuffer) buffer).address(),
try (var guard = NIO_ACCESS.acquireScope(buffer)) {
crc = updateDirectByteBuffer(crc, guard.address(),
pos, limit);
} finally {
Reference.reachabilityFence(buffer);
}
} else if (buffer.hasArray()) {
crc = updateBytes(crc, buffer.array(), pos + buffer.arrayOffset(),
Expand Down
48 changes: 15 additions & 33 deletions src/java.base/share/classes/java/util/zip/Deflater.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,14 @@
package java.util.zip;

import java.lang.ref.Cleaner.Cleanable;
import java.lang.ref.Reference;
import java.nio.ByteBuffer;
import java.nio.ReadOnlyBufferException;
import java.util.Objects;

import jdk.internal.ref.CleanerFactory;
import jdk.internal.util.Preconditions;
import sun.nio.ch.DirectBuffer;

import static java.util.zip.ZipUtils.NIO_ACCESS;

/**
* This class provides support for general purpose compression using the
Expand Down Expand Up @@ -337,11 +337,8 @@ public void setDictionary(ByteBuffer dictionary) {
int remaining = Math.max(dictionary.limit() - position, 0);
ensureOpen();
if (dictionary.isDirect()) {
long address = ((DirectBuffer) dictionary).address();
try {
setDictionaryBuffer(zsRef.address(), address + position, remaining);
} finally {
Reference.reachabilityFence(dictionary);
try (var guard = NIO_ACCESS.acquireScope(dictionary)) {
setDictionaryBuffer(zsRef.address(), guard.address() + position, remaining);
}
} else {
byte[] array = ZipUtils.getBufferArray(dictionary);
Expand Down Expand Up @@ -587,14 +584,11 @@ public int deflate(byte[] output, int off, int len, int flush) {
inputPos = input.position();
int inputRem = Math.max(input.limit() - inputPos, 0);
if (input.isDirect()) {
try {
long inputAddress = ((DirectBuffer) input).address();
try (var guard = NIO_ACCESS.acquireScope(input)) {
result = deflateBufferBytes(zsRef.address(),
inputAddress + inputPos, inputRem,
guard.address() + inputPos, inputRem,
output, off, len,
flush, params);
} finally {
Reference.reachabilityFence(input);
}
} else {
byte[] inputArray = ZipUtils.getBufferArray(input);
Expand Down Expand Up @@ -709,14 +703,11 @@ public int deflate(ByteBuffer output, int flush) {
if (input == null) {
inputPos = this.inputPos;
if (output.isDirect()) {
long outputAddress = ((DirectBuffer) output).address();
try {
try (var guard = NIO_ACCESS.acquireScope(output)) {
result = deflateBytesBuffer(zsRef.address(),
inputArray, inputPos, inputLim - inputPos,
outputAddress + outputPos, outputRem,
guard.address() + outputPos, outputRem,
flush, params);
} finally {
Reference.reachabilityFence(output);
}
} else {
byte[] outputArray = ZipUtils.getBufferArray(output);
Expand All @@ -730,41 +721,32 @@ public int deflate(ByteBuffer output, int flush) {
inputPos = input.position();
int inputRem = Math.max(input.limit() - inputPos, 0);
if (input.isDirect()) {
long inputAddress = ((DirectBuffer) input).address();
try {
try (var inGuard = NIO_ACCESS.acquireScope(input)) {
if (output.isDirect()) {
long outputAddress = outputPos + ((DirectBuffer) output).address();
try {
try (var outGuard = NIO_ACCESS.acquireScope(output)) {
result = deflateBufferBuffer(zsRef.address(),
inputAddress + inputPos, inputRem,
outputAddress, outputRem,
inGuard.address() + inputPos, inputRem,
outGuard.address(), outputRem,
flush, params);
} finally {
Reference.reachabilityFence(output);
}
} else {
byte[] outputArray = ZipUtils.getBufferArray(output);
int outputOffset = ZipUtils.getBufferOffset(output);
result = deflateBufferBytes(zsRef.address(),
inputAddress + inputPos, inputRem,
inGuard.address() + inputPos, inputRem,
outputArray, outputOffset + outputPos, outputRem,
flush, params);
}
} finally {
Reference.reachabilityFence(input);
}
} else {
byte[] inputArray = ZipUtils.getBufferArray(input);
int inputOffset = ZipUtils.getBufferOffset(input);
if (output.isDirect()) {
long outputAddress = ((DirectBuffer) output).address();
try {
try (var guard = NIO_ACCESS.acquireScope(output)) {
result = deflateBytesBuffer(zsRef.address(),
inputArray, inputOffset + inputPos, inputRem,
outputAddress + outputPos, outputRem,
guard.address()+ outputPos, outputRem,
flush, params);
} finally {
Reference.reachabilityFence(output);
}
} else {
byte[] outputArray = ZipUtils.getBufferArray(output);
Expand Down
Loading