7025238 : HttpURLConnection does not handle URLs with an empty path component

[Chris Hegarty]

On 06/20/2013 10:33 AM, Andreas Rieber wrote:
> I see, short test without leading "/" in the path causes the httpserver
> to throws exception:
>
> Exception in thread "main" java.lang.IllegalArgumentException: Illegal
> value for path or protocol
>      at sun.net.httpserver.HttpContextImpl.<init>(HttpContextImpl.java:60)
>      at sun.net.httpserver.ServerImpl.createContext(ServerImpl.java:214)
>      at
> sun.net.httpserver.HttpServerImpl.createContext(HttpServerImpl.java:74)
>      at
> sun.net.httpserver.HttpServerImpl.createContext(HttpServerImpl.java:39)
>      at SimpleServer.main(SimpleServer.java:11)
>
> but the api is clear there: The first character of path must be '/'.

Sorry for the confusion, my point was; should the server with a valid 
context for '/' tolerate a 'GET ?xxyyzz HTTP/1.1'?

-Chris.

>
> If you want we can change that to "should" in the api and add a "/" if
> missing. I guess there are other issues to fix first.
>
> Andreas
>
>
> On 20.06.13 10:40, Chris Hegarty wrote:
>> I see Kurchi pushed this change for you.
>>    http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2b156531b7eb
>>
>> For extra credits ;-) does it make sense to something similar on the
>> server side, sun.net.httpserver???
>>
>> -Chris.
>>
>> On 06/19/2013 07:29 PM, Andreas Rieber wrote:
>>> Hi Kurchi,
>>>
>>> to change the path in URL.java would not be a good idea, it supports
>>> many other protocols as well and what i can read out of the URL
>>> specification is that path can be empty. True, ParserUtil.toUri() uses
>>> the path and query elements separate. Here in HttpClient.java i guess it
>>> saves at least one null check ;-)
>>>
>>> cheers
>>> Andreas
>>>
>>>
>>> On 19.06.13 20:02, Kurchi Hazra wrote:
>>>> Hi Andreas,
>>>>
>>>>   I looked at your changes, and they look good to me. Although we are
>>>> not changing the path of the URL itself (it is not modifiable
>>>> too), but from what I see the only other relevant place where URL.path
>>>> is logically used in http client is in ParseUtil.toURI(), which
>>>> basically does
>>>> the same thing as your fix.
>>>>
>>>>  I'll run the fix against all networking tests on all platforms and
>>>> let you know how things look.
>>>>
>>>> Thanks!
>>>> Kurchi
>>>>
>>>> On 6/19/2013 7:14 AM, Andreas Rieber wrote:
>>>>> Hi Chris and Kurchi,
>>>>>
>>>>> i have updated and rerun the test (removed the "@run main/othervm
>>>>> B7025238").
>>>>>
>>>>> New webrev is here:
>>>>> http://cr.openjdk.java.net/~arieber/7025238/webrev.01/
>>>>>
>>>>> thanks
>>>>> Andreas
>>>>>
>>>>>
>>>>> On 19.06.13 15:33, Chris Hegarty wrote:
>>>>>> Hi Andreas,
>>>>>>
>>>>>> On 18/06/2013 20:19, Andreas Rieber wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> i am looking for a sponsor of this issue.
>>>>>>>
>>>>>>> The bug is here:
>>>>>>> http://bugs.sun.com/view_bug.do?bug_id=7025238
>>>>>>>
>>>>>>> First i verified that the problem still exists. Then i checked the
>>>>>>> problem against some other web servers. Apache handles a missing
>>>>>>> "/" in
>>>>>>> the path. Tomcat, Microsoft-HTTPAPI/2.0 and the openjdk build in
>>>>>>> http
>>>>>>> server behave with same response: 400 Bad Request.
>>>>>>
>>>>>> Nice. Thanks for checking this.
>>>>>>
>>>>>>> I checked the URL specification but could not see any problem with
>>>>>>> empty
>>>>>>> path. The HTTP/1.1 specification is there a bit more detailed. So i
>>>>>>> checked HttpURLconnection.java and HttpClient.java where i found the
>>>>>>> problem. If the path/file from url.getFile() is null or empty, a
>>>>>>> "/" is
>>>>>>> used but not if the url.getFile() returns only a query string. In
>>>>>>> that
>>>>>>> case the path is empty and should have also a "/".
>>>>>>
>>>>>> Sounds reasonable.
>>>>>>
>>>>>>> A webrev can be found here (to be discussed, i am still new to
>>>>>>> openjdk):
>>>>>>> http://cr.openjdk.java.net/~arieber/7025238/webrev.00/
>>>>>>
>>>>>> The source changes look good to me.
>>>>>>
>>>>>>> To write the jtreg test and run them all took longer than the fix
>>>>>>> ;-) I
>>>>>>
>>>>>> Yes, this can often be the case, but thanks for adding a test.
>>>>>>
>>>>>> Trivially, the test does not need to be run in other VM mode. You
>>>>>> can simply remove the line "@run main/othervm B7025238". The default
>>>>>> action for jtreg is to run the test, essentially "@run main
>>>>>> B7025238".
>>>>>>
>>>>>>> did run jtreg on: |test/java/net, | |test/sun/net, |
>>>>>>> |test/java/security
>>>>>>> and | |test/sun/security but sure i don't have all relevant
>>>>>>> platfo||rms.|
>>>>>>
>>>>>> Kurchi sent me mail offline, she has agreed to sponsor this change
>>>>>> for you. I would request that she runs all the networking tests on
>>>>>> all the platforms before pushing. Not a big problem for us here, we
>>>>>> have access to all supported platforms.
>>>>>>
>>>>>> Thanks again,
>>>>>> -Chris.
>>>>>>
>>>>>>>
>>>>>>> thanks
>>>>>>> Andreas
>>>>>>>
>>>>>
>>>>
>>>
>>
>>
>