RE: [PATCH] SOCK_CLOEXEC for opening sockets

[Andrew Luo]

Just checking to see if anyone has any other comments/feedback on this.

Thanks,

-Andrew

-Original Message-
From: Andrew Luo 
Sent: Monday, July 30, 2018 9:44 PM
To: 'Andrew Luo' ; Ivan Gerasimov 
; Chris Hegarty 
Cc: net-dev@openjdk.java.net
Subject: RE: [PATCH] SOCK_CLOEXEC for opening sockets

Fixed a bug, updated (I was calling accept directly instead of a helper 
function that calls accept then fcntl):

diff --git a/make/lib/Lib-jdk.net.gmk b/make/lib/Lib-jdk.net.gmk
--- a/make/lib/Lib-jdk.net.gmk
+++ b/make/lib/Lib-jdk.net.gmk
@@ -33,9 +33,11 @@
   NAME := extnet, \
   OPTIMIZATION := LOW, \
   CFLAGS := $(CFLAGS_JDKLIB), \
+  EXTRA_HEADER_DIRS := \
+  java.base:libnet, \
   LDFLAGS := $(LDFLAGS_JDKLIB) \
   $(call SET_SHARED_LIBRARY_ORIGIN), \
-  LIBS := -ljava, \
+  LIBS := -ljava -lnet, \
   LIBS_solaris := -lsocket, \
   LIBS_linux := -ljvm, \
   ))
diff --git a/src/java.base/aix/native/libnio/ch/AixPollPort.c 
b/src/java.base/aix/native/libnio/ch/AixPollPort.c
--- a/src/java.base/aix/native/libnio/ch/AixPollPort.c
+++ b/src/java.base/aix/native/libnio/ch/AixPollPort.c
@@ -30,6 +30,7 @@
 #include "jlong.h"
 
 #include "sun_nio_ch_AixPollPort.h"
+#include "net_util_md.h"
 
 #include 
 #include 
@@ -137,7 +138,7 @@
 JNIEXPORT void JNICALL
 Java_sun_nio_ch_AixPollPort_socketpair(JNIEnv* env, jclass clazz, jintArray 
sv) {
 int sp[2];
-if (socketpair(PF_UNIX, SOCK_STREAM, 0, sp) == -1) {
+if (NET_SocketPair(PF_UNIX, SOCK_STREAM, 0, sp) == -1) {
 JNU_ThrowIOExceptionWithLastError(env, "socketpair failed");
 } else {
 jint res[2];
diff --git a/src/java.base/linux/native/libnet/linux_close.c 
b/src/java.base/linux/native/libnet/linux_close.c
--- a/src/java.base/linux/native/libnet/linux_close.c
+++ b/src/java.base/linux/native/libnet/linux_close.c
@@ -24,6 +24,7 @@
  */
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -395,8 +396,32 @@
 BLOCKING_IO_RETURN_INT( s, sendto(s, msg, len, flags, to, tolen) );  }
 
-int NET_Accept(int s, struct sockaddr *addr, socklen_t *addrlen) {
-BLOCKING_IO_RETURN_INT( s, accept(s, addr, addrlen) );
+static int acceptFcntlCloexec(int s, struct sockaddr *addr, socklen_t 
*addrlen) {
+int cs;
+cs = accept(s, addr, addrlen);
+if (cs != -1) {
+// Best effort - return value is intentionally ignored since the 
connected
+// socket was successfully created.
+fcntl(cs, F_SETFD, FD_CLOEXEC);
+}
+return cs;
+}
+
+JNIEXPORT int JNICALL
+NET_Accept(int s, struct sockaddr *addr, socklen_t *addrlen) {
+static int (*accept4functionpointer)(int sockfd, struct sockaddr *addr, 
socklen_t *addrlen, int flags) = NULL;
+static int accept4initialized = 0;
+int cs;
+
+if (!accept4initialized) {
+accept4functionpointer = dlsym(RTLD_DEFAULT, "accept4");
+accept4initialized = 1;
+}
+
+if (accept4functionpointer != NULL) {
+BLOCKING_IO_RETURN_INT( s, accept4functionpointer(s, addr, addrlen, 
SOCK_CLOEXEC) );
+}
+BLOCKING_IO_RETURN_INT( s, acceptFcntlCloexec(s, addr, addrlen) );
 }
 
 int NET_Connect(int s, struct sockaddr *addr, int addrlen) { diff --git 
a/src/java.base/unix/native/libnet/Inet4AddressImpl.c 
b/src/java.base/unix/native/libnet/Inet4AddressImpl.c
--- a/src/java.base/unix/native/libnet/Inet4AddressImpl.c
+++ b/src/java.base/unix/native/libnet/Inet4AddressImpl.c
@@ -264,7 +264,7 @@
 int connect_rv = -1;
 
 // open a TCP socket
-fd = socket(AF_INET, SOCK_STREAM, 0);
+fd = NET_Socket(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_Socket(AF_INET, SOCK_RAW, IPPROTO_ICMP);
 if (fd == -1) {
 return tcp_ping4(env, , netif, timeout, ttl);
 } else {
diff --git a/src/java.base/unix/native/libnet/Inet6AddressImpl.c 
b/src/java.base/unix/native/libnet/Inet6AddressImpl.c
--- a/src/java.base/unix/native/libnet/Inet6AddressImpl.c
+++ b/src/java.base/unix/native/libnet/Inet6AddressImpl.c
@@ -461,7 +461,7 @@
 int connect_rv = -1;
 
 // open a TCP socket
-fd = socket(AF_INET6, SOCK_STREAM, 0);
+fd = NET_Socket(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);
+

RE: [PATCH] SOCK_CLOEXEC for opening sockets

[Andrew Luo]

7 @@
 int type = (stream ? SOCK_STREAM : SOCK_DGRAM);
 int domain = (ipv6_available() && preferIPv6) ? AF_INET6 : AF_INET;
 
-fd = socket(domain, type, 0);
+fd = NET_Socket(domain, type, 0);
 if (fd < 0) {
 return handleSocketError(env, errno);
 }
diff --git a/src/java.base/unix/native/libnio/ch/ServerSocketChannelImpl.c 
b/src/java.base/unix/native/libnio/ch/ServerSocketChannelImpl.c
--- a/src/java.base/unix/native/libnio/ch/ServerSocketChannelImpl.c
+++ b/src/java.base/unix/native/libnio/ch/ServerSocketChannelImpl.c
@@ -92,7 +92,7 @@
  * accept() was called.
  */
 for (;;) {
-newfd = accept(ssfd, , _len);
+newfd = NET_Accept(ssfd, , _len);
 if (newfd >= 0) {
 break;
 }
diff --git a/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c 
b/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
--- a/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
+++ b/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
@@ -31,6 +31,7 @@
 #include 
 #include 
 #include "jni_util.h"
+#include "net_util_md.h"
 #include "jdk_net_LinuxSocketOptions.h"
 
 /*
@@ -86,7 +87,7 @@
 (JNIEnv *env, jobject unused) {
 int one = 1;
 int rv, s;
-s = socket(PF_INET, SOCK_STREAM, 0);
+s = NET_Socket(PF_INET, SOCK_STREAM, 0);
 if (s < 0) {
 return JNI_FALSE;
 }
@@ -103,7 +104,7 @@
 static jint socketOptionSupported(jint sockopt) {
 jint one = 1;
 jint rv, s;
-s = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
+s = NET_Socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
 if (s < 0) {
 return 0;
 }
diff --git a/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c 
b/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
--- a/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
+++ b/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
@@ -31,11 +31,12 @@
 #include 
 #include 
 #include "jni_util.h"
+#include "net_util_md.h"
 
 static jint socketOptionSupported(jint sockopt) {
 jint one = 1;
 jint rv, s;
-s = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
+s = NET_Socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
 if (s < 0) {
 return 0;
 }
diff --git a/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c 
b/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
--- a/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
+++ b/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
@@ -157,7 +157,7 @@
 sock_flow_props_t props;
 int rv, s;
 
-s = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
+s = NET_Socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
 if (s < 0) {
 return JNI_FALSE;
 }
diff --git a/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.h 
b/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.h
--- a/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.h
+++ b/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.h
@@ -36,6 +36,7 @@
 #include "jdk_net_SocketFlow.h"
 #include "SolarisSocketOptions.h"
 #include "jdk_net_SolarisSocketOptions.h"
+#include "net_util_md.h"
 
 #ifndef SO_FLOW_SLA
 #define SO_FLOW_SLA 0x1018
diff --git a/src/jdk.sctp/unix/native/libsctp/SctpNet.c 
b/src/jdk.sctp/unix/native/libsctp/SctpNet.c
--- a/src/jdk.sctp/unix/native/libsctp/SctpNet.c
+++ b/src/jdk.sctp/unix/native/libsctp/SctpNet.c
@@ -151,7 +151,7 @@
 Java_sun_nio_ch_sctp_SctpNet_init
   (JNIEnv *env, jclass cl) {
 int sp[2];
-if (socketpair(PF_UNIX, SOCK_STREAM, 0, sp) < 0) {
+if (NET_SocketPair(PF_UNIX, SOCK_STREAM, 0, sp) < 0) {
 JNU_ThrowIOExceptionWithLastError(env, "socketpair failed");
 return;
     }
@@ -180,7 +180,7 @@
     return 0;
 }
 
-fd = socket(domain, (oneToOne ? SOCK_STREAM : SOCK_SEQPACKET), 
IPPROTO_SCTP);
+fd = NET_Socket(domain, (oneToOne ? SOCK_STREAM : SOCK_SEQPACKET), 
IPPROTO_SCTP);
 
 if (fd < 0) {
 return handleSocketError(env, errno);

Thanks,

-Andrew

-Original Message-
From: Andrew Luo  
Sent: Monday, July 30, 2018 5:08 PM
To: Andrew Luo ; Ivan Gerasimov 
; Chris Hegarty 
Cc: net-dev@openjdk.java.net
Subject: RE: [PATCH] SOCK_CLOEXEC for opening sockets

I've updated my patch.  Let me know if this approach looks good to you.  I'm 
using dlsym to get the function pointer to accept4 at runtime so we can support 
compiling on newer Linux systems and using the binaries on older Linux systems 
(where the newer Linux system has accept4, and the older Linux system does 
not).  I know I still need to edit the other Unix (Solaris, AIX, BSD) files but 
I wanted to put this out there first to see if the approach looks good.

diff --git a/make/lib/Lib-jdk.net.gmk b/make/lib/Lib-jdk.net.gmk
--- a/make/lib/Lib-jdk.net.gmk
+++ b/make/lib/Lib-jdk.net.gmk
@@ -33,9 +33,11 @@
   NAME :=

RE: [PATCH] SOCK_CLOEXEC for opening sockets

[Andrew Luo]

Do we need to support compiling on a newer kernel with newer headers and then 
running the compiled binaries on an older platform?  If so, then I think we 
probably have to use dlsym to call accept4 at runtime rather than compile 
time...

Thanks,

-Andrew

-Original Message-
From: net-dev  On Behalf Of Ivan Gerasimov
Sent: Wednesday, July 25, 2018 10:04 PM
To: Chris Hegarty 
Cc: net-dev@openjdk.java.net
Subject: Re: [PATCH] SOCK_CLOEXEC for opening sockets

Hi Chris!

A couple of minor comments.

1)
if (s != -1 || (s == -1 && errno != EINVAL)) {

This can be simplified to

if (s != -1 || errno != EINVAL) {

2)
What about sockets created with accept():  Shouldn't NET_Accept be modified to 
set O_CLOEXEC as well?
On Linux accept4() can be used to pass SOCK_CLOEXEC flag.

With kind regards,
Ivan

On 7/25/18 5:49 AM, Chris Hegarty wrote:
> Alan,
>
>> On 25 Jul 2018, at 08:24, Alan Bateman  wrote:
>>
>> ...
>>
>> As I said previously, the patch isn't complete so native code calling 
>> fork/exec may still have to deal with other file descriptors that are 
>> inherited into the child. I don't object to doing this in phases of course 
>> but somehow we have managed to get by for 20 years without this being an 
>> issue.
> I added the following to the JIRA description to make this clear:
>
> "This JIRA issue, by itself, does not completely resolve the problem 
> of native code calling fork/exec, it may still have to deal with other 
> file descriptors that are inherited by the child. Instead this JIRA 
> issue is targeted at the socket and channels areas only, other areas 
> should be tackled on a phased approach though separate JIRA issues."
>
>> The updates to the various site to use the NET_* functions are fine. 
>> However, I think the new functions in net_util_md.c could be cleaner. I 
>> think it would be better to fallback to socket/socketpair + fcntl when the 
>> initial call fails with EINVAL.
> Agreed. How about this ( trying to reduce the ifdef blocks, and keep 
> them relatively clean ) :
>
> ---
> JNIEXPORT int JNICALL
> NET_Socket(int domain, int type, int protocol) {
>  int s;
> #if defined(SOCK_CLOEXEC)
>  s = socket(domain, type | SOCK_CLOEXEC, protocol);
>  if (s != -1 || (s == -1 && errno != EINVAL)) {
>  return s;
>  }
> #endif
>  s = socket(domain, type, protocol);
>  if (s != -1) {
>  // Best effort - return value is intentionally ignored since the 
> socket
>  // was successfully created.
>  fcntl(s, F_SETFD, FD_CLOEXEC);
>  }
>  return s;
> }
> ---
>
> Updated webrev:
>http://cr.openjdk.java.net/~chegar/8207335/webrev.01/
>
> -Chris.

--
With kind regards,
Ivan Gerasimov

Re: [PATCH] SOCK_CLOEXEC for opening sockets

[Ivan Gerasimov]

Hi Chris!

A couple of minor comments.

1)
if (s != -1 || (s == -1 && errno != EINVAL)) {

This can be simplified to

if (s != -1 || errno != EINVAL) {

2)
What about sockets created with accept():  Shouldn't NET_Accept be 
modified to set O_CLOEXEC as well?

On Linux accept4() can be used to pass SOCK_CLOEXEC flag.

With kind regards,
Ivan

On 7/25/18 5:49 AM, Chris Hegarty wrote:

Alan,


On 25 Jul 2018, at 08:24, Alan Bateman  wrote:

...

As I said previously, the patch isn't complete so native code calling fork/exec 
may still have to deal with other file descriptors that are inherited into the 
child. I don't object to doing this in phases of course but somehow we have 
managed to get by for 20 years without this being an issue.

I added the following to the JIRA description to make this clear:

"This JIRA issue, by itself, does not completely resolve the problem
of native code calling fork/exec, it may still have to deal with other
file descriptors that are inherited by the child. Instead this JIRA
issue is targeted at the socket and channels areas only, other areas
should be tackled on a phased approach though separate JIRA
issues."


The updates to the various site to use the NET_* functions are fine. However, I 
think the new functions in net_util_md.c could be cleaner. I think it would be 
better to fallback to socket/socketpair + fcntl when the initial call fails 
with EINVAL.

Agreed. How about this ( trying to reduce the ifdef blocks, and
keep them relatively clean ) :

---
JNIEXPORT int JNICALL
NET_Socket(int domain, int type, int protocol) {
 int s;
#if defined(SOCK_CLOEXEC)
 s = socket(domain, type | SOCK_CLOEXEC, protocol);
 if (s != -1 || (s == -1 && errno != EINVAL)) {
 return s;
 }
#endif
 s = socket(domain, type, protocol);
 if (s != -1) {
 // Best effort - return value is intentionally ignored since the socket
 // was successfully created.
 fcntl(s, F_SETFD, FD_CLOEXEC);
 }
 return s;
}
---

Updated webrev:
   http://cr.openjdk.java.net/~chegar/8207335/webrev.01/

-Chris.


--
With kind regards,
Ivan Gerasimov

Re: [PATCH] SOCK_CLOEXEC for opening sockets

[Alan Bateman]

On 25/07/2018 13:49, Chris Hegarty wrote:

:

The updates to the various site to use the NET_* functions are fine. However, I 
think the new functions in net_util_md.c could be cleaner. I think it would be 
better to fallback to socket/socketpair + fcntl when the initial call fails 
with EINVAL.

Agreed. How about this ( trying to reduce the ifdef blocks, and
keep them relatively clean ) :

---
JNIEXPORT int JNICALL
NET_Socket(int domain, int type, int protocol) {
 int s;
#if defined(SOCK_CLOEXEC)
 s = socket(domain, type | SOCK_CLOEXEC, protocol);
 if (s != -1 || (s == -1 && errno != EINVAL)) {
 return s;
 }
#endif
 s = socket(domain, type, protocol);
 if (s != -1) {
 // Best effort - return value is intentionally ignored since the socket
 // was successfully created.
 fcntl(s, F_SETFD, FD_CLOEXEC);
 }
 return s;
}
---

Updated webrev:
   http://cr.openjdk.java.net/~chegar/8207335/webrev.01/


This looks much better - thanks!

-Alan

Re: [PATCH] SOCK_CLOEXEC for opening sockets

[Chris Hegarty]

Alan,

> On 25 Jul 2018, at 08:24, Alan Bateman  wrote:
> 
> ...
> 
> As I said previously, the patch isn't complete so native code calling 
> fork/exec may still have to deal with other file descriptors that are 
> inherited into the child. I don't object to doing this in phases of course 
> but somehow we have managed to get by for 20 years without this being an 
> issue.

I added the following to the JIRA description to make this clear:

"This JIRA issue, by itself, does not completely resolve the problem
of native code calling fork/exec, it may still have to deal with other
file descriptors that are inherited by the child. Instead this JIRA
issue is targeted at the socket and channels areas only, other areas
should be tackled on a phased approach though separate JIRA
issues."

> The updates to the various site to use the NET_* functions are fine. However, 
> I think the new functions in net_util_md.c could be cleaner. I think it would 
> be better to fallback to socket/socketpair + fcntl when the initial call 
> fails with EINVAL.

Agreed. How about this ( trying to reduce the ifdef blocks, and
keep them relatively clean ) :

---
JNIEXPORT int JNICALL
NET_Socket(int domain, int type, int protocol) {
int s;
#if defined(SOCK_CLOEXEC)
s = socket(domain, type | SOCK_CLOEXEC, protocol);
if (s != -1 || (s == -1 && errno != EINVAL)) {
return s;
}
#endif
s = socket(domain, type, protocol);
if (s != -1) {
// Best effort - return value is intentionally ignored since the socket
// was successfully created.
fcntl(s, F_SETFD, FD_CLOEXEC);
}
return s;
}
---

Updated webrev:
  http://cr.openjdk.java.net/~chegar/8207335/webrev.01/

-Chris.

Re: [PATCH] SOCK_CLOEXEC for opening sockets

[Alan Bateman]

On 24/07/2018 21:34, Chris Hegarty wrote:

On 19 Jul 2018, at 18:41, Andrew Luo  wrote:

Just checking - is there any other changes that I should make to the patch, or 
anything else you guys need me to do?

A webrev genderated from Andrew’s patch along with:

1) some additional includes of “net_util_md.h” in several missing places
 in the jdk.net module’s source, as well as the appropriate make change:
EXTRA_HEADER_DIRS := \
   java.base:libnet,

2) simplified the ifdef structure for NET_Socket and NET_SocketPair
 in net_util_md.c, and some comment updates, to make it more
 readable.

http://cr.openjdk.java.net/~chegar/8207335/webrev.00/


Thanks for generating a webrev.

As I said previously, the patch isn't complete so native code calling 
fork/exec may still have to deal with other file descriptors that are 
inherited into the child. I don't object to doing this in phases of 
course but somehow we have managed to get by for 20 years without this 
being an issue.


The updates to the various site to use the NET_* functions are fine. 
However, I think the new functions in net_util_md.c could be cleaner. I 
think it would be better to fallback to socket/socketpair + fcntl when 
the initial call fails with EINVAL.


-Alan

Re: [PATCH] SOCK_CLOEXEC for opening sockets

[Chris Hegarty]

> On 19 Jul 2018, at 18:41, Andrew Luo  
> wrote:
> 
> Just checking - is there any other changes that I should make to the patch, 
> or anything else you guys need me to do?

A webrev genderated from Andrew’s patch along with:

1) some additional includes of “net_util_md.h” in several missing places
in the jdk.net module’s source, as well as the appropriate make change:
   EXTRA_HEADER_DIRS := \
  java.base:libnet, 

2) simplified the ifdef structure for NET_Socket and NET_SocketPair
in net_util_md.c, and some comment updates, to make it more
readable.

http://cr.openjdk.java.net/~chegar/8207335/webrev.00/

This successfully passes some preliminary testing, but I will need
someone to build / test on AIX.

-Chris.

P.S. The mailing list seems to have dropped the initial space on
the context lines of Andrew’s patch, which I had to restore before
it would apply cleanly.

RE: [PATCH] SOCK_CLOEXEC for opening sockets

[Andrew Luo]

Just checking - is there any other changes that I should make to the patch, or 
anything else you guys need me to do?

Thanks,

-Andrew

-Original Message-
From: net-dev  On Behalf Of Andrew Luo
Sent: Monday, July 16, 2018 11:59 PM
To: Alan Bateman 
Cc: net-dev@openjdk.java.net
Subject: RE: [PATCH] SOCK_CLOEXEC for opening sockets

Thanks for the feedback.  Made those changes (reverted 
src/java.base/linux/native/libnio/fs/LinuxWatchService.c):

diff --git a/make/lib/Lib-jdk.net.gmk b/make/lib/Lib-jdk.net.gmk
--- a/make/lib/Lib-jdk.net.gmk
+++ b/make/lib/Lib-jdk.net.gmk
@@ -35,7 +35,7 @@
   CFLAGS := $(CFLAGS_JDKLIB), \
   LDFLAGS := $(LDFLAGS_JDKLIB) \
   $(call SET_SHARED_LIBRARY_ORIGIN), \
-  LIBS := -ljava, \
+  LIBS := -ljava -lnet, \
   LIBS_solaris := -lsocket, \
   LIBS_linux := -ljvm, \
   ))
diff --git a/src/java.base/aix/native/libnio/ch/AixPollPort.c 
b/src/java.base/aix/native/libnio/ch/AixPollPort.c
--- a/src/java.base/aix/native/libnio/ch/AixPollPort.c
+++ b/src/java.base/aix/native/libnio/ch/AixPollPort.c
@@ -137,7 +137,7 @@
 JNIEXPORT void JNICALL
 Java_sun_nio_ch_AixPollPort_socketpair(JNIEnv* env, jclass clazz, jintArray 
sv) {
 int sp[2];
-if (socketpair(PF_UNIX, SOCK_STREAM, 0, sp) == -1) {
+if (NET_SocketPair(PF_UNIX, SOCK_STREAM, 0, sp) == -1) {
 JNU_ThrowIOExceptionWithLastError(env, "socketpair failed");
 } else {
 jint res[2];
diff --git a/src/java.base/unix/native/libnet/Inet4AddressImpl.c 
b/src/java.base/unix/native/libnet/Inet4AddressImpl.c
--- a/src/java.base/unix/native/libnet/Inet4AddressImpl.c
+++ b/src/java.base/unix/native/libnet/Inet4AddressImpl.c
@@ -264,7 +264,7 @@
 int connect_rv = -1;
 
 // open a TCP socket
-fd = socket(AF_INET, SOCK_STREAM, 0);
+fd = NET_Socket(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_Socket(AF_INET, SOCK_RAW, IPPROTO_ICMP);
 if (fd == -1) {
 return tcp_ping4(env, , netif, timeout, ttl);
 } else {
diff --git a/src/java.base/unix/native/libnet/Inet6AddressImpl.c 
b/src/java.base/unix/native/libnet/Inet6AddressImpl.c
--- a/src/java.base/unix/native/libnet/Inet6AddressImpl.c
+++ b/src/java.base/unix/native/libnet/Inet6AddressImpl.c
@@ -461,7 +461,7 @@
 int connect_rv = -1;
 
 // open a TCP socket
-fd = socket(AF_INET6, SOCK_STREAM, 0);
+fd = NET_Socket(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_Socket(AF_INET6, SOCK_RAW, IPPROTO_ICMPV6);
 if (fd == -1) {
 return tcp_ping6(env, , netif, timeout, ttl);
 } else {
diff --git a/src/java.base/unix/native/libnet/NetworkInterface.c 
b/src/java.base/unix/native/libnet/NetworkInterface.c
--- a/src/java.base/unix/native/libnet/NetworkInterface.c
+++ b/src/java.base/unix/native/libnet/NetworkInterface.c
@@ -1055,7 +1055,7 @@
 static int openSocket(JNIEnv *env, int proto) {
 int sock;
 
-if ((sock = socket(proto, SOCK_DGRAM, 0)) < 0) {
+if ((sock = NET_Socket(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_Socket(AF_INET, SOCK_DGRAM, 0)) < 0) {
 if (errno == EPROTONOSUPPORT) {
-if ((sock = socket(AF_INET6, SOCK_DGRAM, 0)) < 0) {
+if ((sock = NET_Socket(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_Socket(AF_INET, SOCK_DGRAM, 0)) < 0) {
 if (errno == EPROTONOSUPPORT) {
-if ((sock = socket(AF_INET6, SOCK_DGRAM, 0)) < 0) {
+if ((sock = NET_Socket(AF_INET6, SOCK_DGRAM, 0)) < 0) {
 JNU_ThrowByNameWithMessag

RE: [PATCH] SOCK_CLOEXEC for opening sockets

[Andrew Luo]

REAM, IPPROTO_TCP);
 if (s < 0) {
 return JNI_FALSE;
 }
diff --git a/src/jdk.sctp/unix/native/libsctp/SctpNet.c 
b/src/jdk.sctp/unix/native/libsctp/SctpNet.c
--- a/src/jdk.sctp/unix/native/libsctp/SctpNet.c
+++ b/src/jdk.sctp/unix/native/libsctp/SctpNet.c
@@ -151,7 +151,7 @@
 Java_sun_nio_ch_sctp_SctpNet_init
   (JNIEnv *env, jclass cl) {
 int sp[2];
-if (socketpair(PF_UNIX, SOCK_STREAM, 0, sp) < 0) {
+if (NET_SocketPair(PF_UNIX, SOCK_STREAM, 0, sp) < 0) {
 JNU_ThrowIOExceptionWithLastError(env, "socketpair failed");
 return;
 }
@@ -180,7 +180,7 @@
 return 0;
 }
 
-fd = socket(domain, (oneToOne ? SOCK_STREAM : SOCK_SEQPACKET), 
IPPROTO_SCTP);
+fd = NET_Socket(domain, (oneToOne ? SOCK_STREAM : SOCK_SEQPACKET), 
IPPROTO_SCTP);
 
 if (fd < 0) {
 return handleSocketError(env, errno);

Thanks,

-Andrew

-Original Message-
From: Alan Bateman  
Sent: Monday, July 16, 2018 11:41 PM
To: Andrew Luo 
Cc: net-dev@openjdk.java.net
Subject: Re: [PATCH] SOCK_CLOEXEC for opening sockets

On 17/07/2018 07:33, Andrew Luo wrote:
> Great, thanks.
>
> By the way, I do see other places where we use NET_* functions in 
> libnio, but if you prefer that I duplicate that code instead,
It's okay for the code in libnio/ch to use the NET_* functions. The issue is 
the changes to libnio/fs which is the file system code as that should have no 
dependences on functions exported by libnet. I think it would be better to drop 
the changes to the fs code in this patch. As we've been discussing, there are 
dozens of other areas that will work to allow fork/exec work reliably in native 
code, I think the networking and channels area is just the first step.

-Alan

Re: [PATCH] SOCK_CLOEXEC for opening sockets

[Alan Bateman]

On 17/07/2018 07:33, Andrew Luo wrote:

Great, thanks.

By the way, I do see other places where we use NET_* functions in libnio, but 
if you prefer that I duplicate that code instead,
It's okay for the code in libnio/ch to use the NET_* functions. The 
issue is the changes to libnio/fs which is the file system code as that 
should have no dependences on functions exported by libnet. I think it 
would be better to drop the changes to the fs code in this patch. As 
we've been discussing, there are dozens of other areas that will work to 
allow fork/exec work reliably in native code, I think the networking and 
channels area is just the first step.


-Alan

RE: [PATCH] SOCK_CLOEXEC for opening sockets

[Andrew Luo]

 return;
 }
diff --git a/src/java.base/unix/native/libnio/ch/Net.c 
b/src/java.base/unix/native/libnio/ch/Net.c
--- a/src/java.base/unix/native/libnio/ch/Net.c
+++ b/src/java.base/unix/native/libnio/ch/Net.c
@@ -198,7 +198,7 @@
 int type = (stream ? SOCK_STREAM : SOCK_DGRAM);
 int domain = (ipv6_available() && preferIPv6) ? AF_INET6 : AF_INET;
 
-fd = socket(domain, type, 0);
+fd = NET_Socket(domain, type, 0);
 if (fd < 0) {
 return handleSocketError(env, errno);
 }
diff --git a/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c 
b/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
--- a/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
+++ b/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
@@ -86,7 +86,7 @@
 (JNIEnv *env, jobject unused) {
 int one = 1;
 int rv, s;
-s = socket(PF_INET, SOCK_STREAM, 0);
+s = NET_Socket(PF_INET, SOCK_STREAM, 0);
 if (s < 0) {
 return JNI_FALSE;
 }
@@ -103,7 +103,7 @@
 static jint socketOptionSupported(jint sockopt) {
 jint one = 1;
 jint rv, s;
-s = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
+s = NET_Socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
 if (s < 0) {
 return 0;
 }
diff --git a/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c 
b/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
--- a/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
+++ b/src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
@@ -35,7 +35,7 @@
 static jint socketOptionSupported(jint sockopt) {
 jint one = 1;
 jint rv, s;
-s = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
+s = NET_Socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
 if (s < 0) {
 return 0;
 }
diff --git a/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c 
b/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
--- a/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
+++ b/src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
@@ -157,7 +157,7 @@
 sock_flow_props_t props;
 int rv, s;
 
-s = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
+s = NET_Socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
 if (s < 0) {
 return JNI_FALSE;
 }
diff --git a/src/jdk.sctp/unix/native/libsctp/SctpNet.c 
b/src/jdk.sctp/unix/native/libsctp/SctpNet.c
--- a/src/jdk.sctp/unix/native/libsctp/SctpNet.c
+++ b/src/jdk.sctp/unix/native/libsctp/SctpNet.c
@@ -151,7 +151,7 @@
 Java_sun_nio_ch_sctp_SctpNet_init
   (JNIEnv *env, jclass cl) {
 int sp[2];
-if (socketpair(PF_UNIX, SOCK_STREAM, 0, sp) < 0) {
+if (NET_SocketPair(PF_UNIX, SOCK_STREAM, 0, sp) < 0) {
 JNU_ThrowIOExceptionWithLastError(env, "socketpair failed");
 return;
 }
@@ -180,7 +180,7 @@
 return 0;
 }
 
-fd = socket(domain, (oneToOne ? SOCK_STREAM : SOCK_SEQPACKET), 
IPPROTO_SCTP);
+fd = NET_Socket(domain, (oneToOne ? SOCK_STREAM : SOCK_SEQPACKET), 
IPPROTO_SCTP);
 
 if (fd < 0) {
 return handleSocketError(env, errno);

By the way, I only have x86 systems (and VMs) - any way I can test this on AIX, 
or what is the preferred way to ensure my changes will build properly on all 
platforms?

Thanks,

-Andrew

-Original Message-
From: Chris Hegarty  
Sent: Monday, July 16, 2018 2:58 AM
To: Andrew Luo 
Cc: net-dev@openjdk.java.net
Subject: Re: [PATCH] SOCK_CLOEXEC for opening sockets

Andrew,

I filed the following issue to track this:
   https://bugs.openjdk.java.net/browse/JDK-8207335

Once you produce an updated patch, I can review and sponsor this for you.

-Chris.

On 12/07/18 08:21, Andrew Luo wrote:
> Thanks, I can refactor it.  I'm not as familiar with the OpenJDK 
> architecture - should I just duplicate the function into libnio or is 
> there some common utility library that I should move it into?  Also, 
> let me know what in net_util_* needs cleanup.  The other instances of 
> socket/socketpair in Mac/AIX/Solaris I'm willing to handle as well.
> 
> Files are mostly already handled: 
> https://bugs.openjdk.java.net/browse/JDK-8043780 (in a similar way - 
> O_CLOEXEC on systems that support it and fcntl on other POSIX systems).
> One other resource you mentioned is epolls.  Perhaps there's also 
> other file descriptors that are being used elsewhere, but I think it 
> will take me longer time to figure out.  I'm willing to work on that 
> as well if we all think it's a good idea.
> 
> Anyways, while I agree that it would be ideal to address those other 
> file descriptors as well, is it ok to address those in a separate 
> changeset, or do you think that this changeset doesn't really provide 
> value unless we make this change everywhere?  Sockets are somewhat 
> more problematic than other types of file descriptors (in my opinion) 
> as if you use it to listen on a port and then fork child processes 
> 

Re: [PATCH] SOCK_CLOEXEC for opening sockets

[Chris Hegarty]

Andrew,

I filed the following issue to track this:
  https://bugs.openjdk.java.net/browse/JDK-8207335

Once you produce an updated patch, I can review and
sponsor this for you.

-Chris.

On 12/07/18 08:21, Andrew Luo wrote:
Thanks, I can refactor it.  I’m not as familiar with the OpenJDK 
architecture – should I just duplicate the function into libnio or is 
there some common utility library that I should move it into?  Also, let 
me know what in net_util_* needs cleanup.  The other instances of 
socket/socketpair in Mac/AIX/Solaris I’m willing to handle as well.


Files are mostly already handled: 
https://bugs.openjdk.java.net/browse/JDK-8043780 (in a similar way – 
O_CLOEXEC on systems that support it and fcntl on other POSIX systems).  
One other resource you mentioned is epolls.  Perhaps there’s also other 
file descriptors that are being used elsewhere, but I think it will take 
me longer time to figure out.  I’m willing to work on that as well if we 
all think it’s a good idea.


Anyways, while I agree that it would be ideal to address those other 
file descriptors as well, is it ok to address those in a separate 
changeset, or do you think that this changeset doesn’t really provide 
value unless we make this change everywhere?  Sockets are somewhat more 
problematic than other types of file descriptors (in my opinion) as if 
you use it to listen on a port and then fork child processes (from 
native code) then restart the parent process, it won’t be able to listen 
to the same port until all of the children processes (of the previous 
process) exit/close the fd.  Other file descriptors, unless you open in 
exclusive mode, won’t have the same problem (although the behavior is 
undesirable).


Thanks,

-Andrew

*From:*Alan Bateman 
*Sent:* Wednesday, July 11, 2018 11:59 PM
*To:* Andrew Luo 
*Cc:* Martin Buchholz ; net-dev@openjdk.java.net
*Subject:* Re: [PATCH] SOCK_CLOEXEC for opening sockets

On 12/07/2018 05:55, Andrew Luo wrote:

Ok, fixed a few more places (and a bug where fcntl was being run on
a -1 fd).  Patch is below, let me know if there’s any other
suggestions/etc.

The file system code should not be calling into NET_* functions.  The 
changes to net_util_* will also need cleanup. Otherwise I think you've 
got all the places in the networking / NIO APIs, at least on Linux. 
There are others on macOS, Solaris, ... of course.


That said,  I assume we have many more places in the JDK that have file 
descriptors to open files and other resources. If we really want to 
support the case of someone calling fork/exec from JNI code then it will 
likely need work in several other areas.


-Alan

Re: [PATCH] SOCK_CLOEXEC for opening sockets

[Alan Bateman]

On 12/07/2018 08:21, Andrew Luo wrote:


Thanks, I can refactor it.  I’m not as familiar with the OpenJDK 
architecture – should I just duplicate the function into libnio or is 
there some common utility library that I should move it into?  Also, 
let me know what in net_util_* needs cleanup.  The other instances of 
socket/socketpair in Mac/AIX/Solaris I’m willing to handle as well.


Files are mostly already handled: 
https://bugs.openjdk.java.net/browse/JDK-8043780 (in a similar way – 
O_CLOEXEC on systems that support it and fcntl on other POSIX 
systems).  One other resource you mentioned is epolls.  Perhaps 
there’s also other file descriptors that are being used elsewhere, but 
I think it will take me longer time to figure out.  I’m willing to 
work on that as well if we all think it’s a good idea.


JDK-8043780 was in libjvm so it doesn't cover file and networking I/O or 
other file descriptors created in the libraries.


I agree with your comment that this could be done in steps as there are 
many areas of the libraries that would changes to work with code that 
uses fork/exec directly rather than ProcessBuilder/Process. It's okay to 
start with the socket and channel APIs (native code in libnet and libnio).


-Alan.

RE: [PATCH] SOCK_CLOEXEC for opening sockets

[Andrew Luo]

Thanks, I can refactor it.  I'm not as familiar with the OpenJDK architecture - 
should I just duplicate the function into libnio or is there some common 
utility library that I should move it into?  Also, let me know what in 
net_util_* needs cleanup.  The other instances of socket/socketpair in 
Mac/AIX/Solaris I'm willing to handle as well.

Files are mostly already handled: 
https://bugs.openjdk.java.net/browse/JDK-8043780 (in a similar way - O_CLOEXEC 
on systems that support it and fcntl on other POSIX systems).  One other 
resource you mentioned is epolls.  Perhaps there's also other file descriptors 
that are being used elsewhere, but I think it will take me longer time to 
figure out.  I'm willing to work on that as well if we all think it's a good 
idea.

Anyways, while I agree that it would be ideal to address those other file 
descriptors as well, is it ok to address those in a separate changeset, or do 
you think that this changeset doesn't really provide value unless we make this 
change everywhere?  Sockets are somewhat more problematic than other types of 
file descriptors (in my opinion) as if you use it to listen on a port and then 
fork child processes (from native code) then restart the parent process, it 
won't be able to listen to the same port until all of the children processes 
(of the previous process) exit/close the fd.  Other file descriptors, unless 
you open in exclusive mode, won't have the same problem (although the behavior 
is undesirable).

Thanks,

-Andrew

From: Alan Bateman 
Sent: Wednesday, July 11, 2018 11:59 PM
To: Andrew Luo 
Cc: Martin Buchholz ; net-dev@openjdk.java.net
Subject: Re: [PATCH] SOCK_CLOEXEC for opening sockets

On 12/07/2018 05:55, Andrew Luo wrote:

Ok, fixed a few more places (and a bug where fcntl was being run on a -1 fd).  
Patch is below, let me know if there's any other suggestions/etc.
The file system code should not be calling into NET_* functions.  The changes 
to net_util_* will also need cleanup. Otherwise I think you've got all the 
places in the networking / NIO APIs, at least on Linux. There are others on 
macOS, Solaris, ... of course.

That said,  I assume we have many more places in the JDK that have file 
descriptors to open files and other resources. If we really want to support the 
case of someone calling fork/exec from JNI code then it will likely need work 
in several other areas.

-Alan

Re: [PATCH] SOCK_CLOEXEC for opening sockets

[Alan Bateman]

On 12/07/2018 05:55, Andrew Luo wrote:


Ok, fixed a few more places (and a bug where fcntl was being run on a 
-1 fd).  Patch is below, let me know if there’s any other suggestions/etc.


The file system code should not be calling into NET_* functions. The 
changes to net_util_* will also need cleanup. Otherwise I think you've 
got all the places in the networking / NIO APIs, at least on Linux. 
There are others on macOS, Solaris, ... of course.


That said,  I assume we have many more places in the JDK that have file 
descriptors to open files and other resources. If we really want to 
support the case of someone calling fork/exec from JNI code then it will 
likely need work in several other areas.


-Alan

RE: [PATCH] SOCK_CLOEXEC for opening sockets

[Andrew Luo]

#else
+// Best effort
+// Return value is intentionally ignored since socket was successfully 
opened anyways
+fcntl(socketFileDescriptor, F_SETFD, FD_CLOEXEC);
+#endif
+
+return socketFileDescriptor;
+}
+
#define RESTARTABLE(_cmd, _result) do { \
 do { \
 _result = _cmd; \
@@ -295,7 +349,7 @@
 SOCKETADDRESS sa;
 socklen_t sa_len = sizeof(SOCKETADDRESS);
-fd = socket(AF_INET6, SOCK_STREAM, 0) ;
+fd = NET_Socket(AF_INET6, SOCK_STREAM, 0) ;
 if (fd < 0) {
 /*
  *  TODO: We really cant tell since it may be an unrelated error
@@ -402,7 +456,7 @@
 /* Do a simple dummy call, and try to figure out from that */
 int one = 1;
 int rv, s;
-s = socket(PF_INET, SOCK_STREAM, 0);
+s = NET_Socket(PF_INET, SOCK_STREAM, 0);
 if (s < 0) {
 return JNI_FALSE;
 }
diff -r 95c0644a1c47 src/java.base/unix/native/libnet/net_util_md.h
--- a/src/java.base/unix/native/libnet/net_util_md.h   Fri Jun 15 17:34:01 
2018 -0700
+++ b/src/java.base/unix/native/libnet/net_util_md.hWed Jul 11 01:46:03 
2018 -0700
@@ -89,6 +89,8 @@
int NET_Writev(int s, const struct iovec * vector, int count);
int NET_Connect(int s, struct sockaddr *addr, int addrlen);
int NET_Accept(int s, struct sockaddr *addr, socklen_t *addrlen);
+JNIEXPORT int JNICALL NET_Socket(int domain, int type, int protocol);
+JNIEXPORT int JNICALL NET_SocketPair(int domain, int type, int protocol, int 
socket_vector[2]);
int NET_SocketClose(int s);
int NET_Dup2(int oldfd, int newfd);
int NET_Poll(struct pollfd *ufds, unsigned int nfds, int timeout);
diff -r 95c0644a1c47 src/java.base/unix/native/libnio/ch/FileDispatcherImpl.c
--- a/src/java.base/unix/native/libnio/ch/FileDispatcherImpl.c Fri Jun 15 
17:34:01 2018 -0700
+++ b/src/java.base/unix/native/libnio/ch/FileDispatcherImpl.c  Wed Jul 11 
01:46:03 2018 -0700
@@ -67,7 +67,7 @@
Java_sun_nio_ch_FileDispatcherImpl_init(JNIEnv *env, jclass cl)
{
 int sp[2];
-if (socketpair(PF_UNIX, SOCK_STREAM, 0, sp) < 0) {
+if (NET_SocketPair(PF_UNIX, SOCK_STREAM, 0, sp) < 0) {
 JNU_ThrowIOExceptionWithLastError(env, "socketpair failed");
 return;
 }
diff -r 95c0644a1c47 src/java.base/unix/native/libnio/ch/Net.c
--- a/src/java.base/unix/native/libnio/ch/Net.c   Fri Jun 15 17:34:01 2018 -0700
+++ b/src/java.base/unix/native/libnio/ch/Net.c Wed Jul 11 01:46:03 2018 -0700
@@ -198,7 +198,7 @@
 int type = (stream ? SOCK_STREAM : SOCK_DGRAM);
 int domain = (ipv6_available() && preferIPv6) ? AF_INET6 : AF_INET;
-fd = socket(domain, type, 0);
+    fd = NET_Socket(domain, type, 0);
 if (fd < 0) {
 return handleSocketError(env, errno);
 }

From: Alan Bateman 
Sent: Wednesday, July 11, 2018 12:12 AM
To: Andrew Luo ; Norman Maurer 

Cc: Martin Buchholz ; net-dev@openjdk.java.net
Subject: Re: [PATCH] SOCK_CLOEXEC for opening sockets

This seems to cover the sockets created in libnet but there is also a usage in 
libnio that will need update. It might be cleaner to name it NET_Socket to be 
consistent with the other wrappers (NET_Bind, NET_SetSocketOpt, etc.).

There are a lot of other file descriptors that you may run into. We use 
socketpair in a few places, we have file descriptors for epoll and more. It 
makes me wonder if it would be better to use FD_CLOEXEC consistently. In the 
Windows port then you'll see that we change the inheritance flag on all newly 
created SOCKETs, that is because is because we don't have the same opportunity 
to close inherited handles in the child process that we do on Unix platforms.

-Alan

On 11/07/2018 07:35, Andrew Luo wrote:
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, , netif, timeout, ttl);
 } else {
diff -r 95c0644a1c47 src/java.base/unix/native/libnet/Inet6AddressImpl.c
--- a/src/java.base/unix/native/libnet/Inet6AddressImpl.c

Re: [PATCH] SOCK_CLOEXEC for opening sockets

[Alan Bateman]

rn -1;

@@ -1616,7 +1616,7 @@

strncpy(if2.lifr_name, ifname, sizeof(if2.lifr_name) - 1);

if (ioctl(sock, SIOCGLIFNETMASK, (char *)) < 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 -1;

@@ -1941,9 +1941,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;

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 23:32:21 2018 -0700


@@ -178,7 +178,7 @@

return;

}

- if ((fd = socket(domain, type, 0)) == -1) {

+ if ((fd = NET_SocketOpen(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.

diff -r 95c0644a1c47 src/java.base/unix/native/libnet/SdpSupport.c

--- a/src/java.base/unix/native/libnet/SdpSupport.c Fri Jun 15 
17:34:01 2018 -0700


+++ b/src/java.base/unix/native/libnet/SdpSupport.c  Tue Jul 10 
23:32:21 2018 -0700


@@ -57,7 +57,7 @@

 #if defined(__solaris__)

int domain = ipv6_available() ? AF_INET6 : AF_INET;

- s = socket(domain, SOCK_STREAM, PROTO_SDP);

+ s = NET_SocketOpen(domain, SOCK_STREAM, PROTO_SDP);

#elif defined(__linux__)

/**

* IPv6 not supported by SDP on Linux

@@ -66,7 +66,7 @@

JNU_ThrowIOException(env, "IPv6 not supported");

return -1;

}

- s = socket(AF_INET_SDP, SOCK_STREAM, 0);

+ s = NET_SocketOpen(AF_INET_SDP, SOCK_STREAM, 0);

#else

/* not supported on other platforms at this time */

s = -1;

diff -r 95c0644a1c47 src/java.base/unix/native/libnet/net_util_md.c

--- a/src/java.base/unix/native/libnet/net_util_md.c   Fri Jun 15 
17:34:01 2018 -0700


+++ b/src/java.base/unix/native/libnet/net_util_md.c    Tue Jul 10 
23:32:21 2018 -0700


@@ -117,6 +117,34 @@

return defaultIndex;

}

+/*

+ * Opens a socket

+ * On systems where supported, uses SOCK_CLOEXEC where possible

+ */

+int NET_SocketOpen(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;

+}

+

#define RESTARTABLE(_cmd, _result) do { \

do { \

_result = _cmd; \

@@ -295,7 +323,7 @@

SOCKETADDRESS sa;

socklen_t sa_len = sizeof(SOCKETADDRESS);

- fd = socket(AF_INET6, SOCK_STREAM, 0) ;

+ fd = NET_SocketOpen(AF_INET6, SOCK_STREAM, 0) ;

if (fd < 0) {

/*

*  TODO: We really cant tell since it may be an unrelated error

@@ -402,7 +430,7 @@

/* Do a simple dummy call, and try to figure out from that */

int one = 1;

int rv, s;

- s = socket(PF_INET, SOCK_STREAM, 0);

+ s = NET_SocketOpen(PF_INET, SOCK_STREAM, 0);

if (s < 0) {

return JNI_FALSE;

}

diff -r 95c0644a1c47 src/java.base/unix/native/libnet/net_util_md.h

--- a/src/java.base/unix/native/libnet/net_util_md.h   Fri Jun 15 
17:34:01 2018 -0700


+++ b/src/java.base/unix/native/libnet/net_util_md.h    Tue Jul 10 
23:32:21 2018 -0700


@@ -89,6 +89,7 @@

int NET_Writev(int s, const struct iovec * vector, int count);

int NET_Connect(int s, struct sockaddr *addr, int addrlen);

int NET_Accept(int s, struct sockaddr *addr, socklen_t *addrlen);

+int NET_SocketOpen(int domain, int type, int protocol);

int NET_SocketClose(int s);

int NET_Dup2(int oldfd, int newfd);

int NET_Poll(struct pollfd *ufds, unsigned int nfds, int timeout);

*From:*Norman Maurer 
*Sent:* Tuesday, July 10, 2018 9:55 AM
*To:* Alan Bateman 
*Cc:* Martin Buchholz ; Andrew Luo 
; net-dev@openjdk.java.net

*Subject:* Re: [PATCH] SOCK_CLOEXEC for opening sockets

+1 I think this makes a lot of sense



On 10. Jul 2018, at 17:54, Alan Bateman mailto:alan.bate...@oracle

RE: [PATCH] SOCK_CLOEXEC for opening sockets

[Andrew Luo]

dLastError
 (env, JNU_JAVANETPKG "SocketException", "IPV6 Socket 
creation failed");
 return -1;
@@ -1941,9 +1941,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;
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 
23:32:21 2018 -0700
@@ -178,7 +178,7 @@
 return;
 }
-if ((fd = socket(domain, type, 0)) == -1) {
+if ((fd = NET_SocketOpen(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.
diff -r 95c0644a1c47 src/java.base/unix/native/libnet/SdpSupport.c
--- a/src/java.base/unix/native/libnet/SdpSupport.c Fri Jun 15 17:34:01 
2018 -0700
+++ b/src/java.base/unix/native/libnet/SdpSupport.c  Tue Jul 10 23:32:21 
2018 -0700
@@ -57,7 +57,7 @@
 #if defined(__solaris__)
 int domain = ipv6_available() ? AF_INET6 : AF_INET;
-s = socket(domain, SOCK_STREAM, PROTO_SDP);
+s = NET_SocketOpen(domain, SOCK_STREAM, PROTO_SDP);
#elif defined(__linux__)
 /**
  * IPv6 not supported by SDP on Linux
@@ -66,7 +66,7 @@
 JNU_ThrowIOException(env, "IPv6 not supported");
 return -1;
 }
-s = socket(AF_INET_SDP, SOCK_STREAM, 0);
+s = NET_SocketOpen(AF_INET_SDP, SOCK_STREAM, 0);
#else
 /* not supported on other platforms at this time */
 s = -1;
diff -r 95c0644a1c47 src/java.base/unix/native/libnet/net_util_md.c
--- a/src/java.base/unix/native/libnet/net_util_md.c   Fri Jun 15 17:34:01 
2018 -0700
+++ b/src/java.base/unix/native/libnet/net_util_md.cTue Jul 10 23:32:21 
2018 -0700
@@ -117,6 +117,34 @@
 return defaultIndex;
}
+/*
+ * Opens a socket
+ * On systems where supported, uses SOCK_CLOEXEC where possible
+ */
+int NET_SocketOpen(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;
+}
+
#define RESTARTABLE(_cmd, _result) do { \
 do { \
 _result = _cmd; \
@@ -295,7 +323,7 @@
 SOCKETADDRESS sa;
 socklen_t sa_len = sizeof(SOCKETADDRESS);
-fd = socket(AF_INET6, SOCK_STREAM, 0) ;
+fd = NET_SocketOpen(AF_INET6, SOCK_STREAM, 0) ;
 if (fd < 0) {
 /*
  *  TODO: We really cant tell since it may be an unrelated error
@@ -402,7 +430,7 @@
 /* Do a simple dummy call, and try to figure out from that */
 int one = 1;
 int rv, s;
-s = socket(PF_INET, SOCK_STREAM, 0);
+s = NET_SocketOpen(PF_INET, SOCK_STREAM, 0);
 if (s < 0) {
 return JNI_FALSE;
 }
diff -r 95c0644a1c47 src/java.base/unix/native/libnet/net_util_md.h
--- a/src/java.base/unix/native/libnet/net_util_md.h   Fri Jun 15 17:34:01 
2018 -0700
+++ b/src/java.base/unix/native/libnet/net_util_md.hTue Jul 10 23:32:21 
2018 -0700
@@ -89,6 +89,7 @@
int NET_Writev(int s, const struct iovec * vector, int count);
int NET_Connect(int s, struct sockaddr *addr, int addrlen);
int NET_Accept(int s, struct sockaddr *addr, socklen_t *addrlen);
+int NET_SocketOpen(int domain, int type, int protocol);
int NET_SocketClose(int s);
int NET_Dup2(int oldfd, int newfd);
int NET_Poll(struct pollfd *ufds, unsigned int nfds, int timeout);

From: Norman Maurer 
Sent: Tuesday, July 10, 2018 9:55 AM
To: Alan Bateman 
Cc: Martin Buchholz ; Andrew Luo 
; net-dev@openjdk.java.net
Subject: Re: [PATCH] SOCK_CLOEXEC for opening sockets

+1 I think this makes a lot of sense



On 10. Jul 2018, at 17:54, Alan Bateman 
mailto:alan.bate...@oracle.com>> wrote:

On 10/07/2018 17:40

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

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

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