RFR: 8067680: (sctp) Possible race initializing native IDs

[Rob McKenna]

This is a fix to a possible race in the current sctp code. In a nutshell 
the conditional only checks that isaCls is not null. However if isaCls 
is set by one thread and that thread is interrupted before setting 
isaCtrID, the interrupting thread will falsely assume that isaCtrID has 
been set.


http://cr.openjdk.java.net/~robm/8067680/webrev.01/

-Rob

Re: RFR: 8067680: (sctp) Possible race initializing native IDs

[Roger Riggs]

Looks good.

Roger

On 1/28/2015 11:46 AM, Rob McKenna wrote:
This is a fix to a possible race in the current sctp code. In a 
nutshell the conditional only checks that isaCls is not null. However 
if isaCls is set by one thread and that thread is interrupted before 
setting isaCtrID, the interrupting thread will falsely assume that 
isaCtrID has been set.


http://cr.openjdk.java.net/~robm/8067680/webrev.01/

-Rob

Re: RFR: 8067680: (sctp) Possible race initializing native IDs

[Chris Hegarty]

Looks good to me, thanks Rob.

-Chris.

On 28 Jan 2015, at 16:46, Rob McKenna  wrote:

> This is a fix to a possible race in the current sctp code. In a nutshell the 
> conditional only checks that isaCls is not null. However if isaCls is set by 
> one thread and that thread is interrupted before setting isaCtrID, the 
> interrupting thread will falsely assume that isaCtrID has been set.
> 
> http://cr.openjdk.java.net/~robm/8067680/webrev.01/
> 
>-Rob
> 

RFR 8067105: Socket returned by ServerSocket.accept() is inherited by child process on Windows

[Chris Hegarty]

Reviving an old code review [1], after further investigation…

Pertinent details from previous review:
"A socket connection which is returned by ServerSocket.accept() is 
inherited by a child process. The expected behavior is that the socket 
connection is not inherited by the child process. This is an oversight 
in the original implementation, that only sets HANDLE_FLAG_INHERIT for 
newly created sockets.

The native socket returned by ServerSocket.accept() should be configured 
so it will not be inherited by a child process, 
SetHandleInformation(, HANDLE_FLAG_INHERIT, FALSE)."
http://cr.openjdk.java.net/~chegar/8067105/webrev.00/webrev/

—

There were on objections to the changes, since they are mainly benign, but I 
took the action to investigate why we are calling CreateProcess with 
bInheritHandles set to TRUE. It appears that, without major work, we cannot 
change this.

From 7147084 [2]:

Java does not provide the API to change inherit-bit for any handle. More over, 
since at least the JDK 6, it is assumed that all Java-created handles have no 
installed inherit-bit . The only handles that change the inherit-bit to 1 in 
the Java call are the handles of redirected Input, Output, and Error streams 
(IOE streams for short) for child process. That is the way these redirect the 
streams work. That's why we can not give up the nomination in [TRUE] the 
parameter [bInheritHandles] in the [CreateProcess] call. And I want to mention 
again that this is the only place in JDK where Java installs the inherit-bit. 
Java itself does not use handle inheritance.
—

Ivan pointed out that HANDLE_FLAG_INHERIT will not always work, in the case of 
Layered Service Providers, see [3], but it will work in the standard case.

Finally, I think that we will need to revisit the process creation 
implementation at some point, to see if bInheritHandles can be set to FALSE, 
but that is a larger, more significant, piece of work, that should be done 
separately.

-Chris.

P.S. if there are no objections to the changes I will amend an existing test 
case to cover these new cases.

[1] http://mail.openjdk.java.net/pipermail/net-dev/2014-December/008789.html
[2] 
https://bugs.openjdk.java.net/browse/JDK-7147084?focusedCommentId=13322689&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13322689
[3] 
http://stackoverflow.com/questions/12058911/can-tcp-socket-handles-be-set-not-inheritable

Re: RFR 8067105: Socket returned by ServerSocket.accept() is inherited by child process on Windows

[Ivan Gerasimov]

Hi Chris!

I think it's better to pass the last argument to SetHandleInformation as 
0 rather than FALSE.

This argument is DWORD flag, which is a bit subset of the second argument.

Sincerely yours,
Ivan

On 28.01.2015 23:01, Chris Hegarty wrote:

Reviving an old code review [1], after further investigation…

Pertinent details from previous review:
"A socket connection which is returned by ServerSocket.accept() is
inherited by a child process. The expected behavior is that the socket
connection is not inherited by the child process. This is an oversight
in the original implementation, that only sets HANDLE_FLAG_INHERIT for
newly created sockets.

The native socket returned by ServerSocket.accept() should be configured
so it will not be inherited by a child process,
SetHandleInformation(, HANDLE_FLAG_INHERIT, FALSE)."
http://cr.openjdk.java.net/~chegar/8067105/webrev.00/webrev/ 



—

There were on objections to the changes, since they are mainly benign, 
but I took the action to investigate why we are calling CreateProcess 
with bInheritHandles set to TRUE. It appears that, without major work, 
we cannot change this.


From 7147084 [2]:

Java does not provide the API to change inherit-bit for any handle. 
More over, since at least the JDK 6, it is assumed that all 
Java-created handles have no installed inherit-bit . The only handles 
that change the inherit-bit to 1 in the Java call are the handles of 
redirected Input, Output, and Error streams (IOE streams for short) 
for child process. That is the way these redirect the streams work. 
That's why we can not give up the nomination in [TRUE] the parameter 
[bInheritHandles] in the [CreateProcess] call. And I want to mention 
again that this is the only place in JDK where Java installs the 
inherit-bit. Java itself does not use handle inheritance.

—

Ivan pointed out that HANDLE_FLAG_INHERIT will not always work, in the 
case of Layered Service Providers, see [3], but it will work in the 
standard case.


Finally, I think that we will need to revisit the process creation 
implementation at some point, to see if bInheritHandles can be set to 
FALSE, but that is a larger, more significant, piece of work, that 
should be done separately.


-Chris.

P.S. if there are no objections to the changes I will amend an 
existing test case to cover these new cases.


[1] 
http://mail.openjdk.java.net/pipermail/net-dev/2014-December/008789.html
[2] 
https://bugs.openjdk.java.net/browse/JDK-7147084?focusedCommentId=13322689&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13322689
[3] 
http://stackoverflow.com/questions/12058911/can-tcp-socket-handles-be-set-not-inheritable

Re: RFR 8067105: Socket returned by ServerSocket.accept() is inherited by child process on Windows

[Chris Hegarty]

On 28 Jan 2015, at 20:48, Ivan Gerasimov  wrote:

> Hi Chris!
> 
> I think it's better to pass the last argument to SetHandleInformation as 0 
> rather than FALSE.
> This argument is DWORD flag, which is a bit subset of the second argument.

Agreed. I will make the change before pushing.

Thanks,
-Chris.

> Sincerely yours,
> Ivan
> 
> On 28.01.2015 23:01, Chris Hegarty wrote:
>> Reviving an old code review [1], after further investigation…
>> 
>> Pertinent details from previous review:
>> "A socket connection which is returned by ServerSocket.accept() is 
>> inherited by a child process. The expected behavior is that the socket 
>> connection is not inherited by the child process. This is an oversight 
>> in the original implementation, that only sets HANDLE_FLAG_INHERIT for 
>> newly created sockets.
>> 
>> The native socket returned by ServerSocket.accept() should be configured 
>> so it will not be inherited by a child process, 
>> SetHandleInformation(, HANDLE_FLAG_INHERIT, FALSE)."
>> http://cr.openjdk.java.net/~chegar/8067105/webrev.00/webrev/
>> 
>> —
>> 
>> There were on objections to the changes, since they are mainly benign, but I 
>> took the action to investigate why we are calling CreateProcess with 
>> bInheritHandles set to TRUE. It appears that, without major work, we cannot 
>> change this.
>> 
>> From 7147084 [2]:
>> 
>> Java does not provide the API to change inherit-bit for any handle. More 
>> over, since at least the JDK 6, it is assumed that all Java-created handles 
>> have no installed inherit-bit . The only handles that change the inherit-bit 
>> to 1 in the Java call are the handles of redirected Input, Output, and Error 
>> streams (IOE streams for short) for child process. That is the way these 
>> redirect the streams work. That's why we can not give up the nomination in 
>> [TRUE] the parameter [bInheritHandles] in the [CreateProcess] call. And I 
>> want to mention again that this is the only place in   JDK where 
>> Java installs the inherit-bit. Java itself does not use handle inheritance.
>> —
>> 
>> Ivan pointed out that HANDLE_FLAG_INHERIT will not always work, in the case 
>> of Layered Service Providers, see [3], but it will work in the standard case.
>> 
>> Finally, I think that we will need to revisit the process creation 
>> implementation at some point, to see if bInheritHandles can be set to FALSE, 
>> but that is a larger, more significant, piece of work, that should be done 
>> separately.
>> 
>> -Chris.
>> 
>> P.S. if there are no objections to the changes I will amend an existing test 
>> case to cover these new cases.
>> 
>> [1] http://mail.openjdk.java.net/pipermail/net-dev/2014-December/008789.html
>> [2] 
>> https://bugs.openjdk.java.net/browse/JDK-7147084?focusedCommentId=13322689&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13322689
>> [3] 
>> http://stackoverflow.com/questions/12058911/can-tcp-socket-handles-be-set-not-inheritable
>> 
> 

Re: RFR 8067105: Socket returned by ServerSocket.accept() is inherited by child process on Windows

[Alan Bateman]

On 28/01/2015 20:01, Chris Hegarty wrote:

Reviving an old code review [1], after further investigation…

Pertinent details from previous review:
"A socket connection which is returned by ServerSocket.accept() is
inherited by a child process. The expected behavior is that the socket
connection is not inherited by the child process. This is an oversight
in the original implementation, that only sets HANDLE_FLAG_INHERIT for
newly created sockets.

The native socket returned by ServerSocket.accept() should be configured
so it will not be inherited by a child process,
SetHandleInformation(, HANDLE_FLAG_INHERIT, FALSE)."
http://cr.openjdk.java.net/~chegar/8067105/webrev.00/webrev/ 



I think you'll need to check for AcceptEx usages too but this is 
otherwise okay.


-Alan

Re: RFR 8067105: Socket returned by ServerSocket.accept() is inherited by child process on Windows

[Chris Hegarty]

On 28 Jan 2015, at 21:24, Alan Bateman  wrote:
> 
> On 28/01/2015 20:01, Chris Hegarty wrote:
>> Reviving an old code review [1], after further investigation…
>> 
>> Pertinent details from previous review:
>> "A socket connection which is returned by ServerSocket.accept() is
>> inherited by a child process. The expected behavior is that the socket
>> connection is not inherited by the child process. This is an oversight
>> in the original implementation, that only sets HANDLE_FLAG_INHERIT for
>> newly created sockets.
>> 
>> The native socket returned by ServerSocket.accept() should be configured
>> so it will not be inherited by a child process,
>> SetHandleInformation(, HANDLE_FLAG_INHERIT, FALSE)."
>> http://cr.openjdk.java.net/~chegar/8067105/webrev.00/webrev/ 
>> 
>> 
> I think you'll need to check for AcceptEx usages too but this is otherwise 
> okay.

Good catch, there is one usage of AcceptEx in 
WindowsAsynchronousServerSocketChannelImpl.c 
.

I’ll make the change, add a test, and then update the webrev.

Thanks,
-Chris.

> -Alan