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

Re: [PATCH] SOCK_CLOEXEC for opening sockets

[Martin Buchholz]

I agree with this approach - it parallels the efforts made with O_CLOEXEC
in past years.

According to
https://www.freebsd.org/cgi/man.cgi?query=socket&sektion=2
SOCK_CLOEXEC is also available on freebsd.

On Tue, Jul 10, 2018 at 1:36 AM, Andrew Luo <
andrewluotechnolog...@outlook.com> wrote:

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

Unable to use custom SSLEngine with default TrustManagerFactory after updating to ea20 (and later)

[Norman Maurer]

Hi all,

I just tried to run netty[1] testsuite with the latest jdk11 EA release (21) 
and saw some class-cast-exception with our custom SSLEngine implementation


Caused by: java.lang.ClassCastException: class 
io.netty.handler.ssl.OpenSslEngine cannot be cast to class 
sun.security.ssl.SSLEngineImpl (io.netty.handler.ssl.OpenSslEngine is in 
unnamed module of loader 'app'; sun.security.ssl.SSLEngineImpl is in module 
java.base of loader 'bootstrap')
at 
java.base/sun.security.ssl.SSLAlgorithmConstraints.(SSLAlgorithmConstraints.java:93)
at 
java.base/sun.security.ssl.X509TrustManagerImpl.checkTrusted(X509TrustManagerImpl.java:270)
at 
java.base/sun.security.ssl.X509TrustManagerImpl.checkServerTrusted(X509TrustManagerImpl.java:141)
at 
io.netty.handler.ssl.ReferenceCountedOpenSslClientContext$ExtendedTrustManagerVerifyCallback.verify(ReferenceCountedOpenSslClientContext.java:237)
at 
io.netty.handler.ssl.ReferenceCountedOpenSslContext$AbstractCertificateVerifier.verify(ReferenceCountedOpenSslContext.java:621)
... 27 more


This change seems to be related to:
http://hg.openjdk.java.net/jdk/jdk11/rev/68fa3d4026ea 


I think you miss an instanceof check here in SSLAlgorithmConstraints before try 
to cast to SSLEngineImpl, as otherwise it will be impossible to use custom 
implementations of SSLEngine (which we have in netty) with the default 
TrustManagerFactory.

Does this sound correct ? Should I open a bug-report ?

Bye
Norman

Re: [PATCH] SOCK_CLOEXEC for opening sockets

[Alan Bateman]

On 10/07/2018 17:40, Martin Buchholz wrote:
I agree with this approach - it parallels the efforts made with 
O_CLOEXEC in past years.


According to
https://www.freebsd.org/cgi/man.cgi?query=socket&sektion=2
SOCK_CLOEXEC is also available on freebsd.

This is something that doesn't come up too often, I assume because most 
developers using ProcessBuilder/Process rather than invoking fork from 
native code.


If we are going to tackle this issue then it will require changes in 
several places, changing PlainSocketImpl.c is just one of several.


-Alan

Re: [PATCH] SOCK_CLOEXEC for opening sockets

[Norman Maurer]

+1 I think this makes a lot of sense


> On 10. Jul 2018, at 17:54, Alan Bateman  wrote:
> 
> On 10/07/2018 17:40, Martin Buchholz wrote:
>> I agree with this approach - it parallels the efforts made with O_CLOEXEC in 
>> past years.
>> 
>> According to 
>> https://www.freebsd.org/cgi/man.cgi?query=socket&sektion=2 
>> 
>> SOCK_CLOEXEC is also available on freebsd.
>> 
> This is something that doesn't come up too often, I assume because most 
> developers using ProcessBuilder/Process rather than invoking fork from native 
> code.
> 
> If we are going to tackle this issue then it will require changes in several 
> places, changing PlainSocketImpl.c is just one of several.
> 
> -Alan

RE: [PATCH] SOCK_CLOEXEC for opening sockets

[Andrew Luo]

I agree, I wasn't aware of the other uses of ::socket in the libnet codebase.  
Thus, I've added a new common function, NET_SocketOpen that can be used by all 
the source files in libnet and revised my patch:

diff -r 95c0644a1c47 src/java.base/unix/native/libnet/Inet4AddressImpl.c
--- a/src/java.base/unix/native/libnet/Inet4AddressImpl.c Fri Jun 
15 17:34:01 2018 -0700
+++ b/src/java.base/unix/native/libnet/Inet4AddressImpl.c  Tue Jul 10 
23:32:21 2018 -0700
@@ -264,7 +264,7 @@
 int connect_rv = -1;
 // open a TCP socket
-fd = socket(AF_INET, SOCK_STREAM, 0);
+fd = NET_SocketOpen(AF_INET, SOCK_STREAM, 0);
 if (fd == -1) {
 // note: if you run out of fds, you may not be able to load
 // the exception class, and get a NoClassDefFoundError instead.
@@ -503,7 +503,7 @@
 // Let's try to create a RAW socket to send ICMP packets.
 // This usually requires "root" privileges, so it's likely to fail.
-fd = socket(AF_INET, SOCK_RAW, IPPROTO_ICMP);
+fd = NET_SocketOpen(AF_INET, SOCK_RAW, IPPROTO_ICMP);
 if (fd == -1) {
 return tcp_ping4(env, &sa, netif, timeout, ttl);
 } else {
diff -r 95c0644a1c47 src/java.base/unix/native/libnet/Inet6AddressImpl.c
--- a/src/java.base/unix/native/libnet/Inet6AddressImpl.c Fri Jun 
15 17:34:01 2018 -0700
+++ b/src/java.base/unix/native/libnet/Inet6AddressImpl.c  Tue Jul 10 
23:32:21 2018 -0700
@@ -461,7 +461,7 @@
 int connect_rv = -1;
 // open a TCP socket
-fd = socket(AF_INET6, SOCK_STREAM, 0);
+fd = NET_SocketOpen(AF_INET6, SOCK_STREAM, 0);
 if (fd == -1) {
 // note: if you run out of fds, you may not be able to load
 // the exception class, and get a NoClassDefFoundError instead.
@@ -711,7 +711,7 @@
 // Let's try to create a RAW socket to send ICMP packets.
 // This usually requires "root" privileges, so it's likely to fail.
-fd = socket(AF_INET6, SOCK_RAW, IPPROTO_ICMPV6);
+fd = NET_SocketOpen(AF_INET6, SOCK_RAW, IPPROTO_ICMPV6);
 if (fd == -1) {
 return tcp_ping6(env, &sa, netif, timeout, ttl);
 } else {
diff -r 95c0644a1c47 src/java.base/unix/native/libnet/NetworkInterface.c
--- a/src/java.base/unix/native/libnet/NetworkInterface.c Fri Jun 
15 17:34:01 2018 -0700
+++ b/src/java.base/unix/native/libnet/NetworkInterface.c  Tue Jul 10 
23:32:21 2018 -0700
@@ -1055,7 +1055,7 @@
static int openSocket(JNIEnv *env, int proto) {
 int sock;
-if ((sock = socket(proto, SOCK_DGRAM, 0)) < 0) {
+if ((sock = NET_SocketOpen(proto, SOCK_DGRAM, 0)) < 0) {
 // If EPROTONOSUPPORT is returned it means we don't have
 // support for this proto so don't throw an exception.
 if (errno != EPROTONOSUPPORT) {
@@ -1078,9 +1078,9 @@
static int openSocketWithFallback(JNIEnv *env, const char *ifname) {
 int sock;
-if ((sock = socket(AF_INET, SOCK_DGRAM, 0)) < 0) {
+if ((sock = NET_SocketOpen(AF_INET, SOCK_DGRAM, 0)) < 0) {
 if (errno == EPROTONOSUPPORT) {
-if ((sock = socket(AF_INET6, SOCK_DGRAM, 0)) < 0) {
+if ((sock = NET_SocketOpen(AF_INET6, SOCK_DGRAM, 0)) < 0) {
 JNU_ThrowByNameWithMessageAndLastError
 (env, JNU_JAVANETPKG "SocketException", "IPV6 Socket 
creation failed");
 return -1;
@@ -1315,9 +1315,9 @@
static int openSocketWithFallback(JNIEnv *env, const char *ifname) {
 int sock;
-if ((sock = socket(AF_INET, SOCK_DGRAM, 0)) < 0) {
+if ((sock = NET_SocketOpen(AF_INET, SOCK_DGRAM, 0)) < 0) {
 if (errno == EPROTONOSUPPORT) {
-if ((sock = socket(AF_INET6, SOCK_DGRAM, 0)) < 0) {
+if ((sock = NET_SocketOpen(AF_INET6, SOCK_DGRAM, 0)) < 0) {
 JNU_ThrowByNameWithMessageAndLastError
 (env, JNU_JAVANETPKG "SocketException", "IPV6 Socket 
creation failed");
 return -1;
@@ -1590,9 +1590,9 @@
 int sock, alreadyV6 = 0;
 struct lifreq if2;
-if ((sock = socket(AF_INET, SOCK_DGRAM, 0)) < 0) {
+if ((sock = NET_SocketOpen(AF_INET, SOCK_DGRAM, 0)) < 0) {
 if (errno == EPROTONOSUPPORT) {
-if ((sock = socket(AF_INET6, SOCK_DGRAM, 0)) < 0) {
+if ((sock = NET_SocketOpen(AF_INET6, SOCK_DGRAM, 0)) < 0) {
 JNU_ThrowByNameWithMessageAndLastError
 (env, JNU_JAVANETPKG "SocketException", "IPV6 Socket 
creation failed");
 return -1;
@@ -1616,7 +1616,7 @@
 strncpy(if2.lifr_name, ifname, sizeof(if2.lifr_name) - 1);
 if (ioctl(sock, SIOCGLIFNETMASK, (char *)&if2) < 0) {
 close(sock);
-if ((sock = socket(AF_INET6, SOCK_DGRAM, 0)) < 0) {
+if ((sock = NET_SocketOpen(AF_INET6, SOCK_DGRAM, 0)) < 0) {
 JNU_ThrowByNameWithMessageAndLastError
 (env, JNU_JAVANETPKG "SocketException", "IPV6 Socket 
creation failed");
 return -