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.

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

Don't leak temporary file during test and inital loading of lib. #141

Merged
merged 3 commits into from
Feb 1, 2022
Merged
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -26,8 +26,10 @@
import io.netty.util.internal.logging.InternalLogger;
import io.netty.util.internal.logging.InternalLoggerFactory;

import java.io.File;
import java.io.IOException;
import java.nio.channels.Selector;
import java.nio.file.Path;
import java.util.Arrays;
import java.util.Locale;

@@ -59,14 +61,17 @@ final class Native {
PeerCredentials.class, java.io.FileDescriptor.class
);

File tmpDir = PlatformDependent.tmpdir();
Path tmpFile = tmpDir.toPath().resolve("netty_io_uring.tmp");
try {
// First, try calling a side-effect free JNI method to see if the library was already
// loaded by the application.
Native.createFile();
Native.createFile(tmpFile.toString());
Copy link
Collaborator

Choose a reason for hiding this comment

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

The comment says "side-effect free" but creating a file is not. How did we get here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, this returns a file descriptor that isn't closed.

Copy link
Member Author

Choose a reason for hiding this comment

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

side-effect free in terms of JNI.

} catch (UnsatisfiedLinkError ignore) {
// The library was not previously loaded, load it now.
loadNativeLibrary();
} finally {
tmpFile.toFile().delete();
try {
if (selector != null) {
selector.close();
@@ -256,23 +261,23 @@ private static boolean checkKernelVersion0(String kernelVersion) {
private static native boolean ioUringProbe(int ringFd, int[] ios);
private static native long[][] ioUringSetup(int entries);

public static native int ioUringEnter(int ringFd, int toSubmit, int minComplete, int flags);
static native int ioUringEnter(int ringFd, int toSubmit, int minComplete, int flags);

public static native void eventFdWrite(int fd, long value);
static native void eventFdWrite(int fd, long value);

public static FileDescriptor newBlockingEventFd() {
static FileDescriptor newBlockingEventFd() {
return new FileDescriptor(blockingEventFd());
}

public static native void ioUringExit(long submissionQueueArrayAddress, int submissionQueueRingEntries,
static native void ioUringExit(long submissionQueueArrayAddress, int submissionQueueRingEntries,
long submissionQueueRingAddress, int submissionQueueRingSize,
long completionQueueRingAddress, int completionQueueRingSize,
int ringFd);

private static native int blockingEventFd();

// for testing(it is only temporary)
public static native int createFile();
// for testing only!
static native int createFile(String name);

private static native int registerUnix();

10 changes: 7 additions & 3 deletions transport-native-io_uring/src/main/c/netty_io_uring_native.c
Original file line number Diff line number Diff line change
@@ -296,8 +296,12 @@ static jobjectArray netty_io_uring_setup(JNIEnv *env, jclass clazz, jint entries
return array;
}

static jint netty_create_file(JNIEnv *env, jclass class) {
return open("io-uring-test.txt", O_RDWR | O_TRUNC | O_CREAT, 0644);
static jint netty_create_file(JNIEnv *env, jclass class, jstring filename) {
const char *file = (*env)->GetStringUTFChars(env, filename, 0);

int fd = open(file, O_RDWR | O_TRUNC | O_CREAT, 0644);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we're Linux specific (because io_uring) we can use O_TMPFILE and have the "file" refer to the parent directory. That way the file will never have a name, and will vanish once the fd is closed.

Copy link
Member Author

Choose a reason for hiding this comment

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

good point ... let me do this

Copy link
Contributor

@franz1981 franz1981 Feb 2, 2022

Choose a reason for hiding this comment

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

It is a good idea but seems requires some checks to be sure it works

_TMPFILE requires support by the underlying filesystem;
              only a subset of Linux filesystems provide that support.
              In the initial implementation, support was provided in the
              ext2, ext3, ext4, UDF, Minix, and tmpfs filesystems.
              Support for other filesystems has subsequently been added
              as follows: XFS (Linux 3.15); Btrfs (Linux 3.16); F2FS
              (Linux 3.16); and ubifs (Linux 4.9)

but it seems vastly supported TBH

Copy link
Member Author

Choose a reason for hiding this comment

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

@franz1981 thanks a lot! Because of this I just did not change anything but added a explicit delete to the test.

(*env)->ReleaseStringUTFChars(env, filename, file);
return fd;
}


@@ -592,7 +596,7 @@ static const JNINativeMethod method_table[] = {
{"ioUringSetup", "(I)[[J", (void *) netty_io_uring_setup},
{"ioUringProbe", "(I[I)Z", (void *) netty_io_uring_probe},
{"ioUringExit", "(JIJIJII)V", (void *) netty_io_uring_ring_buffer_exit},
{"createFile", "()I", (void *) netty_create_file},
{"createFile", "(Ljava/lang/String;)I", (void *) netty_create_file},
{"ioUringEnter", "(IIII)I", (void *) netty_io_uring_enter},
{"blockingEventFd", "()I", (void *) netty_epoll_native_blocking_event_fd},
{"eventFdWrite", "(IJ)V", (void *) netty_io_uring_eventFdWrite },
Original file line number Diff line number Diff line change
@@ -17,7 +17,9 @@

import io.netty.channel.unix.FileDescriptor;

import java.io.File;
import java.nio.charset.Charset;
import java.nio.file.Path;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.atomic.AtomicReference;

@@ -37,6 +39,7 @@
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Timeout;
import org.junit.jupiter.api.io.TempDir;

public class NativeTest {

@@ -45,14 +48,18 @@ public static void loadJNI() {
assumeTrue(IOUring.isAvailable());
}




normanmaurer marked this conversation as resolved.
Show resolved Hide resolved
@Test
public void canWriteFile() throws Exception {
public void canWriteFile(@TempDir Path tmpDir) throws Exception {
ByteBufAllocator allocator = new UnpooledByteBufAllocator(true);
final ByteBuf writeEventByteBuf = allocator.directBuffer(100);
final String inputString = "Hello World!";
writeEventByteBuf.writeCharSequence(inputString, Charset.forName("UTF-8"));

int fd = Native.createFile();
Path file = tmpDir.resolve("io_uring.tmp");
int fd = Native.createFile(file.toString());

RingBuffer ringBuffer = Native.createRingBuffer(32);
IOUringSubmissionQueue submissionQueue = ringBuffer.ioUringSubmissionQueue();