[PATCH] SOCK_CLOEXEC for opening sockets

[Andrew Luo]

Hi,

I want to propose to use SOCK_CLOEXEC when opening sockets in the OpenJDK.  I ran into some issues when forking processes (in JNI/C/C++/native code) on Linux where OpenJDK had opened a socket (in Java code).  The child process ends up inheriting a file descriptor to the same socket, which is not ideal in certain circumstances (for example if the Java process restarts and tries to open the socket again while the child process is still running).  Of course, after forking the child process can close all those file descriptors as a workaround, but we use O_CLOEXEC when opening files, so I think it would be ideal to use the same for sockets.

Just some info about the patch:


1.       This is only for Linux (I don't believe SOCK_CLOEXEC exists on other platforms, I use a preprocessor guard for SOCK_CLOEXEC)

2.       I try twice if the first time attempting to open the socket fails with EINVAL because it is possible that the OpenJDK was compiled on a newer kernel/with newer headers that supports SOCK_CLOEXEC but runs on a lower version kernel (not sure if this is supported by the OpenJDK project)

Patch is attached below.  Let me know if you want me to make some changes.

(I did not add a unit test - it would probably need to be a functional test, one that involves a child process and forking, etc.  Let me know if you believe this is necessary to add)

Thanks,

-Andrew

diff -r 95c0644a1c47 src/java.base/unix/native/libnet/PlainSocketImpl.c
--- a/src/java.base/unix/native/libnet/PlainSocketImpl.c Fri Jun 15 17:34:01 2018 -0700
+++ b/src/java.base/unix/native/libnet/PlainSocketImpl.c             Tue Jul 10 01:30:08 2018 -0700
@@ -104,6 +104,34 @@
}
 /*
+ * Opens a socket
+ * On systems where supported, uses SOCK_CLOEXEC where possible
+ */
+static int openSocket(int domain, int type, int protocol) {
+#if defined(SOCK_CLOEXEC)
+    int typeToUse = type | SOCK_CLOEXEC;
+#else
+    int typeToUse = type;
+#endif
+
+    int socketFileDescriptor = socket(domain, typeToUse, protocol);
+#if defined(SOCK_CLOEXEC)
+    if ((socketFileDescriptor == -1) && (errno = EINVAL)) {
+        // Attempt to open the socket without SOCK_CLOEXEC
+        // May have been compiled on an OS with SOCK_CLOEXEC supported
+        // But runtime system might not have SOCK_CLOEXEC support
+        socketFileDescriptor = socket(domain, type, protocol);
+    }
+#else
+    // Best effort
+    // Return value is intentionally ignored since socket was successfully opened anyways
+    fcntl(socketFileDescriptor, F_SETFD, FD_CLOEXEC);
+#endif
+
+    return socketFileDescriptor;
+}
+
+/*
  * The initroto function is called whenever PlainSocketImpl is
  * loaded, to cache field IDs for efficiency. This is called every time
  * the Java class is loaded.
@@ -178,7 +206,8 @@
         return;
     }
-    if ((fd = socket(domain, type, 0)) == -1) {
+
+    if ((fd = openSocket(domain, type, 0)) == -1) {
         /* note: if you run out of fds, you may not be able to load
          * the exception class, and get a NoClassDefFoundError
          * instead.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/net-dev/attachments/20180710/34fe4ec2/attachment.html>