Hi Chris,
I think it is necessary to add a regression test case for this
issue? There was a similar issue on DatagramSocket,
https://bugs.openjdk.java.net/browse/JDK-4945514.
"test/java/net/DatagramSocket/InheritHandle.java" was created for it.
Thanks,
-Felix
On 12/18/2014 10:01 PM, Chris Hegarty wrote:
On 18 Dec 2014, at 13:20, Alan Bateman <alan.bate...@oracle.com> wrote:
On 18/12/2014 11:52, Chris Hegarty wrote:
Thanks for looking at this Alan.
Surprisingly this issue has existed for a long time. I tested with the FCS
version of JRE 5, 6, 7, 8, and 9-ea, on Windows Server 2008 R2, and 2012, and
the newly accepted socket inherit behaviour can be seen. NIO server socket
channels also suffers from the same problem.
Since networking and NIO already sets HANDLE_FLAG_INHERIT on newly created
sockets, I’d like to keep it consistent, and set it on accepted sockets too. I
also think that this fix should be backpored to previous releases, so a small
localised change seems appropriate.
Complete and updated webrev:
http://cr.openjdk.java.net/~chegar/8067105/webrev.00/webrev/
I will file a separate bug to investigate why CreateProcess is being called
with bInheritHandles set to TRUE, as I think any changes here will have a wider
impact.
I'm very surprised by this because the issue of inheriting handles into child
processes has come up several times in the past. The patch you propose is okay
but it doesn't deal with the case of a process being started at around the
time that a connection is accepted.
Right, and this is no different from the construction of new Sockets or
SocketChannels.
The only way to deal with this type of race is to never inherit (Dmitry's point
about the standard in/out/err should already be dealt with it). So yes, I think
it is important to dig into some of the history and to understand whether we
regressed somewhere and why we have the flag set this way.
I have taken a look into the source code archives, and tested with releases
back to 1.2. From what I can see this looks like it has been the case forever.
For what it’s worth, I also remember the issue of inheriting socket handles
coming up a number of times in the past, with various flavours of socket
implementations, but they always seemed to centre around the creation of the
socket. I think accept was just overlooked.
There seems to be enough concern about root cause of this issue pointing back
to the process implementation, so I will not proceed with this change, and file
a separate issue against java.lang.Process. We can revisit this issue when the
Process investigation is complete. Also, the networking and NIO native code
should have its explicit setting of HANDLE_FLAG_INHERIT cleaned up/removed if
it is deemed racy, or not needed.
-Chris.
