RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java
Hi everyone, Can you please review my changes for below bug. Bug: JDK-8080402 : File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java Webrev: http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.01/ This change ensure that if close() fails we throw correct io exception and there is no file leak. Thanks, Vyom
Re: RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java
Hi Vyom, On 07/09/15 10:26, Vyom Tewari wrote: Hi everyone, Can you please review my changes for below bug. Bug: JDK-8080402 : File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java Webrev: http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.01/ This change ensure that if close() fails we throw correct io exception and there is no file leak. However unlikely to happen, I think your changes are correct. Reviewed. -Chris.
Re: RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java
Hi! The close() function isn't really restartable. So, I think, it's more correct to replace RESTARTABLE(close(s), res); with res = close(s); Sincerely yours, Ivan On 07.09.2015 12:26, Vyom Tewari wrote: Hi everyone, Can you please review my changes for below bug. Bug: JDK-8080402 : File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java Webrev: http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.01/ This change ensure that if close() fails we throw correct io exception and there is no file leak. Thanks, Vyom
Re: RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java
On 07/09/2015 10:26, Vyom Tewari wrote: Hi everyone, Can you please review my changes for below bug. Bug: JDK-8080402 : File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java Webrev: http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.01/ This change ensure that if close() fails we throw correct io exception and there is no file leak. close isn't restartable so I think we need to look at that while we are here. -Alan.
Re: RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java
Hi All, Please find the latest diff, which contains the latest fix. http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.02/ Thanks, Vyom On 9/7/2015 3:48 PM, Alan Bateman wrote: On 07/09/2015 10:26, Vyom Tewari wrote: Hi everyone, Can you please review my changes for below bug. Bug: JDK-8080402 : File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java Webrev: http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.01/ This change ensure that if close() fails we throw correct io exception and there is no file leak. close isn't restartable so I think we need to look at that while we are here. -Alan.
Re: RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java
Thanks! It looks good to me now. Sincerely yours, Ivan On 07.09.2015 16:08, Vyom Tewari wrote: Hi All, Please find the latest diff, which contains the latest fix. http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.02/ Thanks, Vyom On 9/7/2015 3:48 PM, Alan Bateman wrote: On 07/09/2015 10:26, Vyom Tewari wrote: Hi everyone, Can you please review my changes for below bug. Bug: JDK-8080402 : File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java Webrev: http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.01/ This change ensure that if close() fails we throw correct io exception and there is no file leak. close isn't restartable so I think we need to look at that while we are here. -Alan.
Re: RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java
a couple of other considerations in the context of this issue perhaps? in this s is being duped onto fd, and part of the dup2 operation is the closing of fd, but what's is the expected state of file descriptor fd in the event of a dup2 failure? s is closed in any case, but what about fd, should it be attended to if dup2 < 0 and closed ? is it still considered a valid fd? what can be said about the state of the encapsulating FileDescriptor associated with fd? at this stage can it still be considered valid? should valid() return false? regards Mark On 07/09/2015 14:29, Ivan Gerasimov wrote: Thanks! It looks good to me now. Sincerely yours, Ivan On 07.09.2015 16:08, Vyom Tewari wrote: Hi All, Please find the latest diff, which contains the latest fix. http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.02/ Thanks, Vyom On 9/7/2015 3:48 PM, Alan Bateman wrote: On 07/09/2015 10:26, Vyom Tewari wrote: Hi everyone, Can you please review my changes for below bug. Bug: JDK-8080402 : File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java Webrev: http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.01/ This change ensure that if close() fails we throw correct io exception and there is no file leak. close isn't restartable so I think we need to look at that while we are here. -Alan.
Re: RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java
On 07/09/2015 14:08, Vyom Tewari wrote: Hi All, Please find the latest diff, which contains the latest fix. http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.02/ I think this looks okay now. -Alan
Re: RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java
Hi Vyom, I will sponsor that and push it for you. best regards, -- daniel On 07/09/15 18:52, Alan Bateman wrote: On 07/09/2015 14:08, Vyom Tewari wrote: Hi All, Please find the latest diff, which contains the latest fix. http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.02/ I think this looks okay now. -Alan
Re: RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java
Oh - sorry - I hadn't seen Mark question. I will wait until those are resolved... On 07/09/15 19:24, Daniel Fuchs wrote: Hi Vyom, I will sponsor that and push it for you. best regards, -- daniel On 07/09/15 18:52, Alan Bateman wrote: On 07/09/2015 14:08, Vyom Tewari wrote: Hi All, Please find the latest diff, which contains the latest fix. http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.02/ I think this looks okay now. -Alan
Re: RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java
On 7 Sep 2015, at 17:41, Mark Sheppard wrote: > a couple of other considerations in the context of this issue perhaps? > > in this s is being duped onto fd, and part of the dup2 operation is the > closing of fd, but > what's is the expected state of file descriptor fd in the event of a dup2 > failure? I’m not sure that the documentation for dup2 gives us enough detail here. The only situation I can see where fd would not be closed is when EBADF is returned. Should not be an issue here. > s is closed in any case, but what about fd, should it be attended to if dup2 > < 0 > and closed ? is it still considered a valid fd? > > what can be said about the state of the encapsulating FileDescriptor > associated with fd? > at this stage can it still be considered valid? > should valid() return false? Probably, but may not be worth bothering with unless there are operations that can access it after this method throws. -Chris. > regards > Mark > > On 07/09/2015 14:29, Ivan Gerasimov wrote: >> Thanks! >> >> It looks good to me now. >> >> Sincerely yours, >> Ivan >> >> On 07.09.2015 16:08, Vyom Tewari wrote: >>> Hi All, >>> >>> Please find the latest diff, which contains the latest fix. >>> http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.02/ >>> >>> Thanks, >>> Vyom >>> >>> >>> On 9/7/2015 3:48 PM, Alan Bateman wrote: On 07/09/2015 10:26, Vyom Tewari wrote: > Hi everyone, > Can you please review my changes for below bug. > > Bug: > JDK-8080402 : File Leak in > jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java > Webrev: > http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.01/ > > This change ensure that if close() fails we throw correct io exception > and there is no file leak. close isn't restartable so I think we need to look at that while we are here. -Alan. >>> >>> >> >
Re: RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java
that's true, the documentation is a bit nebulous on this issue. but the inference is that the file descriptors are indeterminate state. some older man pages allude to this as per solaris man pages, close will " If close() is interrupted by a signal that is to be caught, it will return -1 with errno set to EINTR and the state of fildes is unspecified" if dup2(s, fd) is viewed as a combination of close(fd) and fcntl (s, F_DUP2FD, fd), and close is not restartable then similar semantics could be inferred for dup2 in a EINTR scenario? presume that subsequent call in the RESTARTABLE loop will return another error. On 08/09/2015 09:28, Chris Hegarty wrote: On 7 Sep 2015, at 17:41, Mark Sheppard wrote: a couple of other considerations in the context of this issue perhaps? in this s is being duped onto fd, and part of the dup2 operation is the closing of fd, but what's is the expected state of file descriptor fd in the event of a dup2 failure? I’m not sure that the documentation for dup2 gives us enough detail here. The only situation I can see where fd would not be closed is when EBADF is returned. Should not be an issue here. s is closed in any case, but what about fd, should it be attended to if dup2 < 0 and closed ? is it still considered a valid fd? what can be said about the state of the encapsulating FileDescriptor associated with fd? at this stage can it still be considered valid? should valid() return false? Probably, but may not be worth bothering with unless there are operations that can access it after this method throws. -Chris. regards Mark On 07/09/2015 14:29, Ivan Gerasimov wrote: Thanks! It looks good to me now. Sincerely yours, Ivan On 07.09.2015 16:08, Vyom Tewari wrote: Hi All, Please find the latest diff, which contains the latest fix. http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.02/ Thanks, Vyom On 9/7/2015 3:48 PM, Alan Bateman wrote: On 07/09/2015 10:26, Vyom Tewari wrote: Hi everyone, Can you please review my changes for below bug. Bug: JDK-8080402 : File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java Webrev: http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.01/ This change ensure that if close() fails we throw correct io exception and there is no file leak. close isn't restartable so I think we need to look at that while we are here. -Alan.
Re: RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java
Hi Mark, Is it OK to go ahead with the patch as it is, or you are expecting any additional modification is required ?. Thanks, Vyom On 9/8/2015 6:35 PM, Mark Sheppard wrote: that's true, the documentation is a bit nebulous on this issue. but the inference is that the file descriptors are indeterminate state. some older man pages allude to this as per solaris man pages, close will " If close() is interrupted by a signal that is to be caught, it will return -1 with errno set to EINTR and the state of fildes is unspecified" if dup2(s, fd) is viewed as a combination of close(fd) and fcntl (s, F_DUP2FD, fd), and close is not restartable then similar semantics could be inferred for dup2 in a EINTR scenario? presume that subsequent call in the RESTARTABLE loop will return another error. On 08/09/2015 09:28, Chris Hegarty wrote: On 7 Sep 2015, at 17:41, Mark Sheppard wrote: a couple of other considerations in the context of this issue perhaps? in this s is being duped onto fd, and part of the dup2 operation is the closing of fd, but what's is the expected state of file descriptor fd in the event of a dup2 failure? I’m not sure that the documentation for dup2 gives us enough detail here. The only situation I can see where fd would not be closed is when EBADF is returned. Should not be an issue here. s is closed in any case, but what about fd, should it be attended to if dup2 < 0 and closed ? is it still considered a valid fd? what can be said about the state of the encapsulating FileDescriptor associated with fd? at this stage can it still be considered valid? should valid() return false? Probably, but may not be worth bothering with unless there are operations that can access it after this method throws. -Chris. regards Mark On 07/09/2015 14:29, Ivan Gerasimov wrote: Thanks! It looks good to me now. Sincerely yours, Ivan On 07.09.2015 16:08, Vyom Tewari wrote: Hi All, Please find the latest diff, which contains the latest fix. http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.02/ Thanks, Vyom On 9/7/2015 3:48 PM, Alan Bateman wrote: On 07/09/2015 10:26, Vyom Tewari wrote: Hi everyone, Can you please review my changes for below bug. Bug: JDK-8080402 : File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java Webrev: http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.01/ This change ensure that if close() fails we throw correct io exception and there is no file leak. close isn't restartable so I think we need to look at that while we are here. -Alan.
Re: RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java
Hi Vyom, yes, I believe the consensus is to proceed with the changes. regards Mark On 09/09/2015 16:23, Vyom Tewari wrote: Hi Mark, Is it OK to go ahead with the patch as it is, or you are expecting any additional modification is required ?. Thanks, Vyom On 9/8/2015 6:35 PM, Mark Sheppard wrote: that's true, the documentation is a bit nebulous on this issue. but the inference is that the file descriptors are indeterminate state. some older man pages allude to this as per solaris man pages, close will " If close() is interrupted by a signal that is to be caught, it will return -1 with errno set to EINTR and the state of fildes is unspecified" if dup2(s, fd) is viewed as a combination of close(fd) and fcntl (s, F_DUP2FD, fd), and close is not restartable then similar semantics could be inferred for dup2 in a EINTR scenario? presume that subsequent call in the RESTARTABLE loop will return another error. On 08/09/2015 09:28, Chris Hegarty wrote: On 7 Sep 2015, at 17:41, Mark Sheppard wrote: a couple of other considerations in the context of this issue perhaps? in this s is being duped onto fd, and part of the dup2 operation is the closing of fd, but what's is the expected state of file descriptor fd in the event of a dup2 failure? I’m not sure that the documentation for dup2 gives us enough detail here. The only situation I can see where fd would not be closed is when EBADF is returned. Should not be an issue here. s is closed in any case, but what about fd, should it be attended to if dup2 < 0 and closed ? is it still considered a valid fd? what can be said about the state of the encapsulating FileDescriptor associated with fd? at this stage can it still be considered valid? should valid() return false? Probably, but may not be worth bothering with unless there are operations that can access it after this method throws. -Chris. regards Mark On 07/09/2015 14:29, Ivan Gerasimov wrote: Thanks! It looks good to me now. Sincerely yours, Ivan On 07.09.2015 16:08, Vyom Tewari wrote: Hi All, Please find the latest diff, which contains the latest fix. http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.02/ Thanks, Vyom On 9/7/2015 3:48 PM, Alan Bateman wrote: On 07/09/2015 10:26, Vyom Tewari wrote: Hi everyone, Can you please review my changes for below bug. Bug: JDK-8080402 : File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java Webrev: http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.01/ This change ensure that if close() fails we throw correct io exception and there is no file leak. close isn't restartable so I think we need to look at that while we are here. -Alan.
Re: RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java
On 09/09/15 17:54, Mark Sheppard wrote: Hi Vyom, yes, I believe the consensus is to proceed with the changes. OK - I will be pushing them then. best regards, -- daniel regards Mark On 09/09/2015 16:23, Vyom Tewari wrote: Hi Mark, Is it OK to go ahead with the patch as it is, or you are expecting any additional modification is required ?. Thanks, Vyom On 9/8/2015 6:35 PM, Mark Sheppard wrote: that's true, the documentation is a bit nebulous on this issue. but the inference is that the file descriptors are indeterminate state. some older man pages allude to this as per solaris man pages, close will " If close() is interrupted by a signal that is to be caught, it will return -1 with errno set to EINTR and the state of fildes is unspecified" if dup2(s, fd) is viewed as a combination of close(fd) and fcntl (s, F_DUP2FD, fd), and close is not restartable then similar semantics could be inferred for dup2 in a EINTR scenario? presume that subsequent call in the RESTARTABLE loop will return another error. On 08/09/2015 09:28, Chris Hegarty wrote: On 7 Sep 2015, at 17:41, Mark Sheppard wrote: a couple of other considerations in the context of this issue perhaps? in this s is being duped onto fd, and part of the dup2 operation is the closing of fd, but what's is the expected state of file descriptor fd in the event of a dup2 failure? I’m not sure that the documentation for dup2 gives us enough detail here. The only situation I can see where fd would not be closed is when EBADF is returned. Should not be an issue here. s is closed in any case, but what about fd, should it be attended to if dup2 < 0 and closed ? is it still considered a valid fd? what can be said about the state of the encapsulating FileDescriptor associated with fd? at this stage can it still be considered valid? should valid() return false? Probably, but may not be worth bothering with unless there are operations that can access it after this method throws. -Chris. regards Mark On 07/09/2015 14:29, Ivan Gerasimov wrote: Thanks! It looks good to me now. Sincerely yours, Ivan On 07.09.2015 16:08, Vyom Tewari wrote: Hi All, Please find the latest diff, which contains the latest fix. http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.02/ Thanks, Vyom On 9/7/2015 3:48 PM, Alan Bateman wrote: On 07/09/2015 10:26, Vyom Tewari wrote: Hi everyone, Can you please review my changes for below bug. Bug: JDK-8080402 : File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java Webrev: http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.01/ This change ensure that if close() fails we throw correct io exception and there is no file leak. close isn't restartable so I think we need to look at that while we are here. -Alan.
