RFR: 7100957 : Java doesn't correctly handle the SOCKS protocol when used over IPv6

[Chris Hegarty]

The source changes look good to me too.

I see Alan has commented on the test, and I agree. Trivially, can you 
also remove the dependency on the old internal HttpServer, and use the 
newer com.sun.net.httpserver API. It is easier to user, and more robust. 
You can see an example here [1].

-Chris.

[1] 
http://hg.openjdk.java.net/jdk9/dev/jdk/file/tip/test/sun/net/www/protocol/http/StreamingOutputStream.java.


On 05/01/14 11:09, Alan Bateman wrote:
> On 03/01/2014 11:04, Dimitar Mavrodiev wrote:
>> Greetings all,
>>
>> I've fixed this and created a test to cover it, is there a sponsor who
>> could push this through? Here's a link to the webrev
>> https://googledrive.com/host/0B2CI6Ih--1t5bVVwbVlBRmpVMDg/index.html.
>>
>> It's a simple fix that correctly consumes the bytes from a SOCKS reply
>> which represent an IPv6 address or a domain name. I also had to fix
>> SocksServer as it was not correctly constructing a String
>> representation of an IPv6 address from byte[].
>>
>> I didn't find it necessary to cover the case of DOMAIN_NAME in the
>> test as name resolution happens locally and not on the SOCKS server,
>> which is perhaps worth another fix.
>>
> Thanks for the patch and I see that your OCA has been processed.
>
> I checked section 5 of RFC 1928 and it does indeed appear that the
> DOMAINNAME (0x03) and IP V6 address (0x04) cases were not implemented
> correctly. Your patch looks right. In passing, I see that the constants
> for SOCKS are defined in an interface (which is an anti-pattern) and we
> should clean that up at some point (not necessary for this patch of course).
>
> On the test then I think it will need to check that IPv6 is enabled as
> part of the setup, otherwise it looks like it will fail. I realize that
> IPv6 is enabled by default everywhere these days but we regularly test
> on machine that don't have it configured. One other thing about the test
> is that it will require a GPL header. Would you have cycles to expand
> the SOCKS test infrastructure to cover the DOMAIN_NAME case? I ask about
> that case because it was the lack of test coverage that meant this
> mis-handling slipped through (although I don't think it is actually used
> and doesn't appear to have been noticed before).
>
> Otherwise, I think this is a good (and I would be happy to sponsor it).
>
> -Alan.
>