Re: [PATCH] JDK-8217705 - HTTPClient wrong exception type when bad status line is received
Hi Jaikiran, I have eyeballed your patch and on the surface I'd say it looks good to me. I'll need to have a deeper look at the full code after importing it but I'm not expecting any surprise. I can work as your sponsor for this. Thanks for adding the new 3 digits check and finding the appropriate test for this :-). Please give me some time to import your patch and send it through our test system - and try to shake it a bit :-) If I'm not mistaken you're a JDK author [1] right? If so you should have your own web page [2] available to publish (you can use scp) webrevs [3] should you wish to do so (webrevs are usually more convenient to reviewers as they contain the full context) We're not quite there yet as I still need to test your patch first - but since you are an author you should also be prepared to generate a changeset with a properly formatted comment [4] that can be directly hg imported and then pushed to the jdk repo. best regards, -- daniel [1] http://openjdk.java.net/census#jpai [2] http://cr.openjdk.java.net/~jpai/ [3] http://openjdk.java.net/projects/code-tools/ [4] http://openjdk.java.net/guide/producingChangeset.html On 12/06/2019 02:46, Jaikiran Pai wrote: Hello, Attached is a patch for the issue reported at https://bugs.openjdk.java.net/browse/JDK-8217705. In addition to catching the NumberFormatException that can arise while parsing (an invalid) status code in the status line, this change also checks that the status code is indeed a 3-digit integer, as required by the RFC-2616, section 6.1.1 [1]. In either of these cases, where the status code is incorrect, this change now throws a java.net.ProtocolException similar to other cases where it's thrown for issues encountered during parsing of the status line. The patch also contains an update to an existing test case to include testing of these invalid status codes. Locally, on top of this patch, I've run: jtreg -jdk:build/macosx-x86_64-server-release/images/jdk -a -ea -esa -agentvm -conc:4 -ignore:quiet test/jdk/java/net/httpclient and all tests have passed: Test results: passed: 190 Could I please get a review of this patch and someone to sponsor it? [1] https://tools.ietf.org/html/rfc2616#section-6.1.1 -Jaikiran
RFR [13] 8225651: Missed the `@` in a couple of code tags of SocketImpl
The fix for JDK-8224477 missed the `@` in a couple of code tags.
diff --git a/src/java.base/share/classes/java/net/SocketImpl.java
b/src/java.base/share/classes/java/net/SocketImpl.java
--- a/src/java.base/share/classes/java/net/SocketImpl.java
+++ b/src/java.base/share/classes/java/net/SocketImpl.java
@@ -396,7 +396,7 @@
*
* @implSpec
* The default implementation of this method first checks that the
given
- * socket option {code name} is not null, then throws {@code
+ * socket option {@code name} is not null, then throws {@code
* UnsupportedOperationException}. Subclasses should override this
method
* with an appropriate implementation.
*
@@ -424,7 +424,7 @@
*
* @implSpec
* The default implementation of this method first checks that the
given
- * socket option {code name} is not null, then throws {@code
+ * socket option {@code name} is not null, then throws {@code
* UnsupportedOperationException}. Subclasses should override this
method
* with an appropriate implementation.
*
-Chris.
Re: RFR [13] 8225651: Missed the `@` in a couple of code tags of SocketImpl
On 12/06/2019 11:21, Chris Hegarty wrote: The fix for JDK-8224477 missed the `@` in a couple of code tags. Looks good.
Re: RFR [13] 8225651: Missed the `@` in a couple of code tags of SocketImpl
Looks good Chris!
-- daniel
On 12/06/2019 11:21, Chris Hegarty wrote:
The fix for JDK-8224477 missed the `@` in a couple of code tags.
diff --git a/src/java.base/share/classes/java/net/SocketImpl.java
b/src/java.base/share/classes/java/net/SocketImpl.java
--- a/src/java.base/share/classes/java/net/SocketImpl.java
+++ b/src/java.base/share/classes/java/net/SocketImpl.java
@@ -396,7 +396,7 @@
*
* @implSpec
* The default implementation of this method first checks that the
given
- * socket option {code name} is not null, then throws {@code
+ * socket option {@code name} is not null, then throws {@code
* UnsupportedOperationException}. Subclasses should override this
method
* with an appropriate implementation.
*
@@ -424,7 +424,7 @@
*
* @implSpec
* The default implementation of this method first checks that the
given
- * socket option {code name} is not null, then throws {@code
+ * socket option {@code name} is not null, then throws {@code
* UnsupportedOperationException}. Subclasses should override this
method
* with an appropriate implementation.
*
-Chris.
Re: RFR 8216417: cleanup of IPv6 scope-id handling
Michael, On 11/06/2019 18:14, Michael McMahon wrote: ... Updated webrev: http://cr.openjdk.java.net/~michaelm/8216417/webrev.4/ This looks very good. -Chris.
Re: [PATCH] JDK-8217705 - HTTPClient wrong exception type when bad status line is received
Hello Daniel, On 12/06/19 3:42 PM, Daniel Fuchs wrote: > Hi Jaikiran, > > I have eyeballed your patch and on the surface I'd say it looks > good to me. I'll need to have a deeper look at the full code > after importing it but I'm not expecting any surprise. > > I can work as your sponsor for this. Thank you :) > Thanks for adding the new 3 digits check and finding > the appropriate test for this :-). > > Please give me some time to import your patch and send it > through our test system - and try to shake it a bit :-) Please take your time. > > If I'm not mistaken you're a JDK author [1] right? Yes, I am. > If so you should have your own web page [2] available > to publish (you can use scp) webrevs [3] should you wish > to do so (webrevs are usually more convenient to reviewers > as they contain the full context) I wasn't aware that the author status allows me to create webrev hosted at cr.openjdk.java.net. Thanks for pointing me to that. I read up a bit and have tested my access to that server and it's working fine. > > We're not quite there yet as I still need to test > your patch first - but since you are an author you > should also be prepared to generate a changeset > with a properly formatted comment [4] that can > be directly hg imported and then pushed to the > jdk repo. I decided to give it a try and host it on cr.openjdk.java.net. I have now published the webrev for this patch, here http://cr.openjdk.java.net/~jpai/8217705/00/webrev/ This is my first usage of this tool, so if there's something that I missed or got wrong, please do let me know. And of course, if the webrev needs to be regenerated after you are done testing the patch, I can do that too. Thank you for your help so far. -Jaikiran > > best regards, > > -- daniel > > [1] http://openjdk.java.net/census#jpai > [2] http://cr.openjdk.java.net/~jpai/ > [3] http://openjdk.java.net/projects/code-tools/ > [4] http://openjdk.java.net/guide/producingChangeset.html > > On 12/06/2019 02:46, Jaikiran Pai wrote: >> Hello, >> >> Attached is a patch for the issue reported at >> https://bugs.openjdk.java.net/browse/JDK-8217705. >> >> In addition to catching the NumberFormatException that can arise while >> parsing (an invalid) status code in the status line, this change also >> checks that the status code is indeed a 3-digit integer, as required by >> the RFC-2616, section 6.1.1 [1]. In either of these cases, where the >> status code is incorrect, this change now throws a >> java.net.ProtocolException similar to other cases where it's thrown for >> issues encountered during parsing of the status line. >> >> The patch also contains an update to an existing test case to include >> testing of these invalid status codes. >> >> Locally, on top of this patch, I've run: >> >> jtreg -jdk:build/macosx-x86_64-server-release/images/jdk -a -ea -esa >> -agentvm -conc:4 -ignore:quiet test/jdk/java/net/httpclient >> >> and all tests have passed: >> >> Test results: passed: 190 >> >> Could I please get a review of this patch and someone to sponsor it? >> >> [1] https://tools.ietf.org/html/rfc2616#section-6.1.1 >> >> -Jaikiran >> >
Re: [PATCH] JDK-8217705 - HTTPClient wrong exception type when bad status line is received
Hi Jaikiran, On 12/06/2019 13:41, Jaikiran Pai wrote: I decided to give it a try and host it on cr.openjdk.java.net. I have now published the webrev for this patch, here http://cr.openjdk.java.net/~jpai/8217705/00/webrev/ This is my first usage of this tool, so if there's something that I missed or got wrong, please do let me know. And of course, if the webrev needs to be regenerated after you are done testing the patch, I can do that too. Thanks for that! Since you're an author - you also don't need the Contributed-by: line. This is only needed when there are several contributors, or when the contributor is not an author. If you have configured your ~/.hgrc as shown in [4] then your openjdk user id should appear in the commit (the user: field you can see when you do `hg log -r `) I believe you can also pass `--user jpai` to `hg commit` if not. Hope this helps, -- daniel > > [4] http://openjdk.java.net/guide/producingChangeset.html
Re: [PATCH] JDK-8217705 - HTTPClient wrong exception type when bad status line is received
Hi Daniel, On 12/06/19 7:30 PM, Daniel Fuchs wrote: > Hi Jaikiran, > > On 12/06/2019 13:41, Jaikiran Pai wrote: >> I decided to give it a try and host it on cr.openjdk.java.net. I have >> now published the webrev for this patch, here >> http://cr.openjdk.java.net/~jpai/8217705/00/webrev/ >> >> This is my first usage of this tool, so if there's something that I >> missed or got wrong, please do let me know. And of course, if the webrev >> needs to be regenerated after you are done testing the patch, I can do >> that too. > > Thanks for that! > > Since you're an author - you also don't need the > Contributed-by: line. > This is only needed when there are several contributors, > or when the contributor is not an author. Fixed - removed it in the new webrev. > > If you have configured your ~/.hgrc as shown in [4] > then your openjdk user id should appear in the commit > (the user: field you can see when you do `hg log -r `) > I believe you can also pass `--user jpai` to `hg commit` > if not. > Sorry, missed that previously. Fixed this too. The new webrev is now available for review at http://cr.openjdk.java.net/~jpai/8217705/01/webrev/ -Jaikiran > > Hope this helps, > > -- daniel > > > > [4] http://openjdk.java.net/guide/producingChangeset.html
[teststabilization] RFR: 8225578: Replace wildcard address with loopback or local host in tests - part 16
Hi, Please find below a patch that fixes java/net/Authenticator and java/net/CookieHandler to use the loopback address - or the local host address if needed, instead of the wildcard whenever that is possible: webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8225578/webrev.00/ JBS: https://bugs.openjdk.java.net/browse/JDK-8225578 best regards, -- daniel
Re: 8223813: (aio) Iocp.getErrorMessage should drop trailing \r\n
Hi Ivan,
I am perhaps beating a dead horse here, but how about this instead?
if (n > 0) {
NET_ThrowNew(env, err, buf);
LocalFree(buf);
} else {
NET_ThrowNew(env, err, "FormatMessage failed");
}
After all, an error *did* occur.
Thanks,
Brian
> On Jun 11, 2019, at 7:11 PM, Ivan Gerasimov wrote:
>
> The webrev looks fine to me.
>
> I think that *most likely* the check if (buf != NULL) will work as expected:
> buf will only be set to non-NULL value upon success.
>
> However, the documentation for the function FormatMessage states:
> """
> If the function fails, the return value is zero.
> """
>
> So, my preference would be if (n > 0).
Re: 8223813: (aio) Iocp.getErrorMessage should drop trailing \r\n
Actually, never mind, I am being completely lame here: both NET_ThrowNew() and
the Windows function LocalFree() are robust to a NULL-valued buf so I think we
can just remove the n > 0 or buf == NULL check altogether.
Sorry for the noise: I should have checked this first.
Thanks,
Brian
> On Jun 12, 2019, at 9:51 AM, Brian Burkhalter
> wrote:
>
> I am perhaps beating a dead horse here, but how about this instead?
>
> if (n > 0) {
> NET_ThrowNew(env, err, buf);
> LocalFree(buf);
> } else {
> NET_ThrowNew(env, err, "FormatMessage failed");
> }
>
> After all, an error *did* occur.
Re: 8223813: (aio) Iocp.getErrorMessage should drop trailing \r\n
On 6/12/19 10:02 AM, Brian Burkhalter wrote:
Actually, never mind, I am being completely lame here: both
NET_ThrowNew() and the Windows function LocalFree() are robust to a
NULL-valued buf so I think we can just remove the n > 0 or buf == NULL
check altogether.
That's true, assuming that you initialize buf = NULL and hopping that
FormatMessage won't change buf upon failure.
I wish MSDN were a little bit more specific here.
I am fine with
1)
362 TCHAR *buf = NULL;
2)
unconditional
395NET_ThrowNew(env, err, buf);
396LocalFree(buf);
With kind regards,
Ivan
Sorry for the noise: I should have checked this first.
Thanks,
Brian
On Jun 12, 2019, at 9:51 AM, Brian Burkhalter
mailto:brian.burkhal...@oracle.com>> wrote:
I am perhaps beating a dead horse here, but how about this instead?
if (n > 0) {
NET_ThrowNew(env, err, buf);
LocalFree(buf);
} else {
NET_ThrowNew(env, err, "FormatMessage failed");
}
After all, an error *did* occur.
--
With kind regards,
Ivan Gerasimov
Re: 8223813: (aio) Iocp.getErrorMessage should drop trailing \r\n
Hi Ivan, > On Jun 12, 2019, at 4:03 PM, Ivan Gerasimov wrote: > > On 6/12/19 10:02 AM, Brian Burkhalter wrote: >> Actually, never mind, I am being completely lame here: both NET_ThrowNew() >> and the Windows function LocalFree() are robust to a NULL-valued buf so I >> think we can just remove the n > 0 or buf == NULL check altogether. >> > That's true, assuming that you initialize buf = NULL and hopping that > FormatMessage won't change buf upon failure. True. > I wish MSDN were a little bit more specific here. Likewise. > I am fine with > > 1) > 362 TCHAR *buf = NULL; > > 2) > unconditional > 395NET_ThrowNew(env, err, buf); > 396LocalFree(buf); Barring objections I will check it in with the above code after the JDK 13 fork. Thanks, Brian
