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.
