RFR 6563286 6797318 8177648 - Undeclared IAE thrown from HttpURLConnection.connect for some URLs

[Michael McMahon]

Hi Jaikiran,

On 29/07/2019, 13:12, Jaikiran Pai wrote:
> Hello Michael,
>
> Thank you for reviewing this. Comments inline.
>
> On 24/07/19 10:37 PM, Michael McMahon wrote:
>> Hi Jaikiran,
>>
>> Thanks for looking at this issue. ProxySelector::select is not
>> a particularly well specified method. The spec is a little vague
>> on what is expected of the supplied URI parameter.
>>
>> It seems to me however, that the intent is that the supplied URI should
>> contain a valid protocol (scheme) and host, even though that is
>> perhaps not stated clearly enough. Though it does refer to
>> the protocol and the destination address as the components
>> it is looking for.
>>
>> The concern I have with the suggested fix is that it appears
>> to contradict that implicit requirement.
>>
>> I wonder if another solution is possible where the IAE is caught
>> at the appropriate place(s) and converted to an IOException
>> which is what users are expecting.
> I can update the proposed patch to redo it to catch the IAE and throw
> the IOException at the relevant places. Would you also want me to update
> any javadoc of the ProxySelector to make it more clear of this (so far)
> implicit behaviour? Does that require a CSR?

I think it would be good to do that, and it would require a CSR.
The existing @throws statement could be expanded to make this explicit 
perhaps.

Thanks,
Michael
> -Jaikiran
>
>
>> Michael.
>>
>> On 19/07/2019, 16:14, Jaikiran Pai wrote:
>>> Hello,
>>>
>>> Could I please get a review and a sponsor for a patch which fixes an
>>> issue in sun.net.spi.DefaultProxySelector? The patch is available as a
>>> webrev at
>>> http://cr.openjdk.java.net/~jpai/webrev/defaultproxyselector/1/webrev/
>>>
>>> The issue has been reported in multiple different JBS issues[1][2][3]
>>> all coming back to the same root cause. There probably are other similar
>>> JBS issues related to this. The root cause is that the
>>> DefaultProxySelector throws IllegalArgumentException when the passed URI
>>> is non-null and thus contradicts its javadoc which (only) states the
>>> IllegalArgumentException is thrown when the URI is null.
>>>
>>> The change in the patch, now returns a Proxy.NO_PROXY when either the
>>> protocol or the host of the passed URI is null. This is the same as
>>> what's suggested in JDK-6797318.
>>>
>>> In addition to the change to DefaultProxySelector, this patch also
>>> changes sun.net.www.protocol.http.HttpURLConnection and
>>> sun.net.www.protocol.ftp.FtpURLConnection. Just before invoking the
>>> ProxySelector.select(...), both these classes use
>>> sun.net.www.ParseUtil.toURI(...) which can return null. The changes to
>>> these classes now ensure that the null isn't passed to the selector (to
>>> avoid a IllegalArgumentException) and instead is handled properly.
>>>
>>> 2 new test classes have been added in the patch. These tests fail (as
>>> expected) without the change to the source classes above and pass with
>>> these changes. After these changes, locally, I have also run the
>>> existing jtreg tests under test/jdk/java/net/HttpURLConnection and
>>> test/jdk/sun/net/www/http/HttpURLConnection/ and they have passed
>>> without regressions.
>>>
>>> I haven't added anything new for the FTP testing, since I didn't find a
>>> easy way to do that (based on my very limited search of existing tests).
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-6797318
>>>
>>> [2] https://bugs.openjdk.java.net/browse/JDK-8177648
>>>
>>> [3] https://bugs.openjdk.java.net/browse/JDK-6563286
>>>
>>> -Jaikiran
>>>
>>>