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: TLS ALPN Proposal v4
Hi, On Mon, Sep 7, 2015 at 5:54 AM, Bradford Wetmore wrote: > IMHO, the following works well. I've added a new method that contains the > ordered list of ciphersuites still to be tried which is a hint for ALPNinboked > selection, but we delay the actual ciphersuite selection until after the > ALPN value is chosen. So the algorithm is now: > > 0. Applications (especially server) might order suites with h2 > first for TLSv1.2. sslParameters.setUseCipherSuitesOrder(true) > should be called on the server to ensure those suites are > tried first. > > 1. Start Handshake. > > 2. Internal server handshaker chooses the TLS version. > > 3. The internal server handshaker finds the client/server/protocol > version intersection of the suites, loads the initial ordered > list into a new method on a SSLSession (obtained by the > getHandshakeSSLSession()), then iterates through the > ordered list of ciphersuites as usual. > > 4. For each "candidate" ciphersuite, first call the > ApplicationProtocolSelector to choose an appropriate ALPN value. > The getHandshakeSSLSession() contains the negotiated TLS > protocol version and the ordered ciphersuite list with the > "candidate" suite as the first entry. > > Note: If the client sent unsupported ALPN values, the Selector > can throw a SSLHandshakeException at this point and generate the > "no_application_protocol" alert. >inboked > The Selector can also either choose to ignore/skip the suite, or > accept the suite but choose no ALPN value. > > 5. Continue the ciphersuite selection routine as usual (check for > appropriate Keys, etc). The KeyManager now has access to the > negotiated TLS version and ALPN value along with theit > ciphersuite via the same/usual > getHandshakeSSLSession(). > This satisfies the RFC 7301 goal of having > the certificate selection mechanism use the ALPN value. > > As ciphersuites are removed from consideration via the > internal iterator in 3, they are also removed from the > corresponding SSLSession entry. > > 6. When handshaking is complete, the applications should verify > that the session parameters (protocol version, ALPN value, and > ciphersuites, etc.) are suitable, and send a HTTP-level > INADEQUATE_SECURITY (H2) if there's a problem. > > In general, if the ciphersuites aren't ordered properly, that should be > considered a configuration error on the part of the application(s). However, > this dynamic ALPN selection approach still allows for appropriate ALPN > values to be selected for each possible ciphersuite. That is, if the suites > were ordered H1/H2, the ALPN value should be H1. > > For an example of such a selector, please see the new H2/H2+H1/H1 > ApplicationProtocolSelector implementions in > ApplicationProtocolSelector.java, and how they are called in > ServerHandshaker.java. > > > Here is the latest: > > http://cr.openjdk.java.net/~wetmore/8051498/webrev.09 Are you sure this is the latest code ? The relevant changes in ServerHandshaker are commented out. I have the feeling that ApplicationProtocolSelector (APS) is not really an application protocol selector, but a cipher suite selector. APS seem to conflate cipher suite selection with application protocol selection: the return value of select() actually is a 2-tuple that means (ciphers[0], protocol) so that returning the empty string or null does not have any effect on protocol selection, but only on cipher selection. You can for example implement APS without even looking at the application protocol, just to exclude dynamically certain ciphers from selection (e.g. based on performance or remote IP). I don't understand exactly bullet 5 above. If APS chooses a cipher and a protocol, but this is not a valid combination for the KeyManager to select a certificate, what happens ? That APS is called again, right ? But what if that was the last cipher ? Then APS won't be called again. How can an application know that it is done trying to select a protocol and that the protocol it chose is actually used ? I guess this also brings to bullet 6: how can an application verify that the N-tuple chosen is actually a correct one ? Is there an additional callback that is being invoked (that applications can implement) before the ServerHello is actually sent to the client ? A very important point that I saw in the comments is support for TLS session resumption - this has been asked multiple times by Jetty users that were using Jetty's ALPN (and we implemented it). Just make sure that whatever proposal enters JDK 9, session resumption is supported. My preference would go to the previous proposal (akin to Jetty - I know I am biased) where protocol selection was happening in isolation *after* cipher selection. It is much simpler, and has
JDK-8022748
Hi, i want to have a closer look to JDK-8022748[1]. I make experimented a little and for me it seemed that the relativize method is making the trouble. Has someone had a look at it, already? Or is it a academic (relativize on "." ) case that is described in the bug-report? Elsewise I would investicate the case and try to find a solution to it. -- Sebastian [1] https://bugs.openjdk.java.net/browse/JDK-8022748
Re: RFR 8072466: Deadlock when starting MulticastSocket and DatagramSocket
This looks like a nice cleanup, and fix. Thanks Ivan. -Chris. On 05/09/15 15:25, Ivan Gerasimov wrote: Hi everyone! The fix didn't bring enough attention back in February for some reason. So, I'd like to re-request a review. I've added a regression test, which reliably reproduces a deadlock on my local Windows 7 machine. BUGURL: https://bugs.openjdk.java.net/browse/JDK-8072466 WEBREV: http://cr.openjdk.java.net/~igerasim/8072466/00/webrev/ Sincerely yours, Ivan On 17.02.2015 9:54, Ivan Gerasimov wrote: Can someone help review this fix please? Comments/suggestions are welcome! Sincerely yours, Ivan On 06.02.2015 20:48, Ivan Gerasimov wrote: Hello! There has been a deadlock in jdk-net code noticed on Windows. In the bug description there's a stack snippet showing that one of the deadlocked threads stuck in the native initialization code of DualStackPlainDatagramSocketImpl and the other -- in initializing of TwoStacksPlainDatagramSocketImpl. I suspect that the reason might be due to unordered initialization: AbstractPlainDatagramSocketImpl, the parent of both classes above, explicitly loads TwoStacksPlainDatagramSocketImpl from the native init(). Thus, when both classes are being initialized concurrently, it may end with a clash. Another issue noticed by Mark Sheppard is that the Windows version of DefaultDatagramSocketImplFactory.createDatagramSocketImpl() isn't synchronized, but reads/modifies shared data. Thus, I removed modification and declared all the accessed fields final. Would you please help review the proposed fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8072466 WEBREV: http://cr.openjdk.java.net/~igerasim/8072466/0/webrev/ The build went fine on all the platforms, all the tests from (net|nio|io) passed Okay. Sincerely yours, Ivan
Re: RFR 8072466: Deadlock when starting MulticastSocket and DatagramSocket
Thank you Chris for the review! Sincerely yours, Ivan On 07.09.2015 17:39, Chris Hegarty wrote: This looks like a nice cleanup, and fix. Thanks Ivan. -Chris. On 05/09/15 15:25, Ivan Gerasimov wrote: Hi everyone! The fix didn't bring enough attention back in February for some reason. So, I'd like to re-request a review. I've added a regression test, which reliably reproduces a deadlock on my local Windows 7 machine. BUGURL: https://bugs.openjdk.java.net/browse/JDK-8072466 WEBREV: http://cr.openjdk.java.net/~igerasim/8072466/00/webrev/ Sincerely yours, Ivan On 17.02.2015 9:54, Ivan Gerasimov wrote: Can someone help review this fix please? Comments/suggestions are welcome! Sincerely yours, Ivan On 06.02.2015 20:48, Ivan Gerasimov wrote: Hello! There has been a deadlock in jdk-net code noticed on Windows. In the bug description there's a stack snippet showing that one of the deadlocked threads stuck in the native initialization code of DualStackPlainDatagramSocketImpl and the other -- in initializing of TwoStacksPlainDatagramSocketImpl. I suspect that the reason might be due to unordered initialization: AbstractPlainDatagramSocketImpl, the parent of both classes above, explicitly loads TwoStacksPlainDatagramSocketImpl from the native init(). Thus, when both classes are being initialized concurrently, it may end with a clash. Another issue noticed by Mark Sheppard is that the Windows version of DefaultDatagramSocketImplFactory.createDatagramSocketImpl() isn't synchronized, but reads/modifies shared data. Thus, I removed modification and declared all the accessed fields final. Would you please help review the proposed fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8072466 WEBREV: http://cr.openjdk.java.net/~igerasim/8072466/0/webrev/ The build went fine on all the platforms, all the tests from (net|nio|io) passed Okay. Sincerely yours, Ivan
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: JDK-8022748
Hi, i think my first sugesstion was false. While it seems that we can fix the symptom by changing the final URI-creation in relativize through reparsing we need to be sure that the relativize doesn't change to intension of the original url (which is an absolute path without an shema). I think the problem is in the Parser (while it can parse the URI "/a:b" ) it misses to encode the forbidden punction into "/a%3Ab". After relativize it is only a URI with the path a:b. But to string and reparsing makes it a URI with a schema of "a" and an schemaspecific-part of "b" I would like to add another low_mask and high_mask to manage all forbidden chars and put this mask into the encode method. So we can encode all forbidden chars for every part of the URI differently. What do you think? Is there someone who would sponsor this? -- Sebastian Am 07.09.2015 um 16:30 schrieb Sebastian Sickelmann: > Hi, > > i want to have a closer look to JDK-8022748[1]. > > I make experimented a little and for me it seemed that the relativize > method is making the trouble. > Has someone had a look at it, already? Or is it a academic (relativize > on "." ) case that is described > in the bug-report? Elsewise I would investicate the case and try to find > a solution to it. > > -- Sebastian > > [1] https://bugs.openjdk.java.net/browse/JDK-8022748 > >
Re: [patch] JDK-4906983
Hi, please find the link to the webrev[1] hosted at my dropbox in my first mail in this thread at the buttom of this mail. A direkt link to the including patch file can be found here: https://dl.dropboxusercontent.com/u/43692695/oss-patches/openjdk/JDK-4906983/jdk.patch -- Sebastian Am 08.09.2015 2:03 vorm. schrieb David Holmes : > > On 7/09/2015 11:46 PM, Ivan Krylov wrote: > > Hi Sebastian, > > > > The current OpenJDK rules [1] do not allow to accept patches from people > > that are not OCA signatories. > > For the record, small patches can be accepted: > > "A Participant is an individual who has subscribed to one or more > OpenJDK mailing lists. A Participant may post messages to a list, submit > simple patches, and make other kinds of small contributions. > > A Contributor is a Participant who has signed the Oracle Contributor > Agreement (OCA), ..." > > --- > > I did not see the referenced patch so don't know if it would be > considered small or not. > > Cheers, > David > > > If you would like your patch to be considered - please sign the OCA[2], > > that will be reflected at [3]. > > The process of becoming an OpenJDK contributor is described at [4]. > > > > Thanks, > > > > Ivan > > > > > > [1] http://openjdk.java.net/bylaws > > [2] http://www.oracle.com/technetwork/oca-405177.pdf > > [3] http://openjdk.java.net/census > > [4] http://openjdk.java.net/contribute/ > > > > > > On 06/09/2015 09:58, Sebastian Sickelmann wrote: > >> Please find my small patch[1] to javadoc in java.net.URL that adresses > >> JDK-4906983(javadoc-fix)[2]. > >> > >> -- Sebastian > >> > >> [1] > >> https://dl.dropboxusercontent.com/u/43692695/oss-patches/openjdk/JDK-4906983/index.html > >> > >> > >> [2] https://bugs.openjdk.java.net/browse/JDK-4906983 > >> > >
Re: [patch] JDK-4906983
Hi Sebastian, On 8/09/2015 1:54 PM, Sebastian Sickelmann wrote: Hi, please find the link to the webrev[1] hosted at my dropbox in my first mail in this thread at the buttom of this mail. A direkt link to the including patch file can be found here: https://dl.dropboxusercontent.com/u/43692695/oss-patches/openjdk/JDK-4906983/jdk.patch Another rule is that all submitted patches must come in via OpenJDK infrustructure so they fall under the term-of-use [1] - either within the email to the list (though attachments get stripped often), or hosted on cr.openjdk.java.net. An OpenJDK Author can host the patch on cr.openjdk.java.net on your behalf. Cheers, David [1] http://openjdk.java.net/legal/tou/terms -- Sebastian Am 08.09.2015 2:03 vorm. schrieb David Holmes : On 7/09/2015 11:46 PM, Ivan Krylov wrote: Hi Sebastian, The current OpenJDK rules [1] do not allow to accept patches from people that are not OCA signatories. For the record, small patches can be accepted: "A Participant is an individual who has subscribed to one or more OpenJDK mailing lists. A Participant may post messages to a list, submit simple patches, and make other kinds of small contributions. A Contributor is a Participant who has signed the Oracle Contributor Agreement (OCA), ..." --- I did not see the referenced patch so don't know if it would be considered small or not. Cheers, David If you would like your patch to be considered - please sign the OCA[2], that will be reflected at [3]. The process of becoming an OpenJDK contributor is described at [4]. Thanks, Ivan [1] http://openjdk.java.net/bylaws [2] http://www.oracle.com/technetwork/oca-405177.pdf [3] http://openjdk.java.net/census [4] http://openjdk.java.net/contribute/ On 06/09/2015 09:58, Sebastian Sickelmann wrote: Please find my small patch[1] to javadoc in java.net.URL that adresses JDK-4906983(javadoc-fix)[2]. -- Sebastian [1] https://dl.dropboxusercontent.com/u/43692695/oss-patches/openjdk/JDK-4906983/index.html [2] https://bugs.openjdk.java.net/browse/JDK-4906983
