RFR(S): JDK-8041435 Make JDWP socket connector accept only local connections by default
Dmitry Samersoff
dmitry.samersoff at oracle.com
Fri May 16 19:13:26 UTC 2014
Serguei,
Thanks. will fix indend before push.
-Dmitry
On 2014-05-16 22:24, serguei.spitsyn at oracle.com wrote:
> Dmitry,
>
> The lines 197-241 still have wrong indent - must be 4.
> Otherwise, it is good - reviewed.
>
> Thanks,
> Serguei
>
> On 5/16/14 8:19 AM, Dmitry Samersoff wrote:
>> Serguei,
>>
>> Fixed in place, please press shift-reload.
>>
>> http://cr.openjdk.java.net/~dsamersoff/JDK-8041435/webrev.02/
>>
>> -Dmitry
>>
>> On 2014-05-16 03:19, serguei.spitsyn at oracle.com wrote:
>>> On 5/15/14 3:20 PM, serguei.spitsyn at oracle.com wrote:
>>>> Dmitry,
>>>>
>>>> I've reviewed the .01 webrev.
>>> Sorry, wanted to say .02 webrev.
>>>
>>>
>>> Thanks,
>>> Serguei
>>>
>>>> src/share/transport/socket/socketTransport.c
>>>>
>>>> Lines 197-240: to be consistent with the rest of the file the indent
>>>> must be 4, not 2.
>>>> The comment at the line 200 is better to start from capital letter:
>>>> "It looks up ..."
>>>>
>>>> 202 struct addrinfo hints, *res;
>>>> ...
>>>> 205 memset(&hints, 0, sizeof(hints));
>>>> Would it more simple to use initialization instead of memset:
>>>>
>>>> 202 struct addrinfo hints = { 0 };
>>>> 203 struct addrinfo *res = NULL;
>>>>
>>>>
>>>> The space is missed after the commas and around '+':
>>>>
>>>> 228 n = strtoul(s_port,&eptr,10);
>>>> 229 if (eptr != s_port+strlen(s_port)) {
>>>> 247 memset((void *)sa,0,sizeof(struct sockaddr_in));
>>>> 261 } else if (strncmp(address,"localhost:",10) == 0) {
>>>>
>>>> Unnecessary dot:
>>>> 235 // invalid port number.
>>>>
>>>>
>>>> The n is declared as u_long but a cast to u_short is used in
>>>> comparison:
>>>> 220 u_long n;
>>>> ...
>>>> 234 if (n > (u_short) -1) {
>>>> Not sure I understand the condition at line 234.
>>>> The unsigned long n is always bigger than -1, right?
>>>> Or the type u_long is not unsigned long?
>>>>
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>> On 5/15/14 12:31 PM, Dmitry Samersoff wrote:
>>>>> Dan,
>>>>>
>>>>> Thank you for the review. Next version is:
>>>>>
>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8041435/webrev.02/
>>>>>
>>>>> please, see below.
>>>>>
>>>>> On 2014-05-15 19:58, Daniel D. Daugherty wrote:
>>>>>> On 5/15/14 5:50 AM, Dmitry Samersoff wrote:
>>>>>>> Hi Everyone,
>>>>>>>
>>>>>>> Please review the fix:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8041435/webrev.01/
>>>>>> src/share/transport/socket/socketTransport.c
>>>>>> line 200: // it lookups "localhost" and return 127.0.0.1 if
>>>>>> lookup
>>>>>> Typo: "it lookups" -> "it looks up"
>>>>>> Typo: "and return 127.0.0.1" -> "and returns 127.0.0.1"
>>>>> fixed. .
>>>>>
>>>>>> line 210: return dbgsysHostToNetworkLong(INADDR_LOOPBACK);
>>>>>> nit: indent should be 4-spaces (for this file)
>>>>> fixed.
>>>>>
>>>>>> line 235: u_short port = (u_short)atoi(address+10);
>>>>>> Will this crash on "localhost:"? (colon, but no port number)
>>>>>>
>>>>>> line 236: sa->sin_port = dbgsysHostToNetworkShort(port);
>>>>>> If atoi() on line 235 is passed a non-decimal, what will the
>>>>>> value of 'port' be and what will happen here?
>>>>>>
>>>>>> line 241: u_short port = (u_short)atoi(address+2);
>>>>>> line 242: sa->sin_port = dbgsysHostToNetworkShort(port);
>>>>>> Same questions about these two lines...
>>>>> Original code doesn't have any guards for invalid user input and I
>>>>> plan
>>>>> to add it later as a part of upcoming cleanup.
>>>>>
>>>>> But it's worth to do it now. I also add a test for it.
>>>>>
>>>>>
>>>>>> old line 440: err = parseAddress(addressString, &sa,
>>>>>> 0x7f000001);
>>>>>> Is it bad that my brain automatically translated that HEX
>>>>>> string into 127.0.0.1? I guess I still remember networking
>>>>>> code...
>>>>> Yes, you are correct. It's a bad way to specify loopback address. I
>>>>> replaced it with INADDR_LOOPBACK.
>>>>>
>>>>>
>>>>>> I checked out this doc:
>>>>>>
>>>>>> http://docs.oracle.com/javase/7/docs/technotes/guides/jpda/conninv.html
>>>>>>
>>>>> Thanks!
>>>>>
>>>>> -Dmitry
>>>>>
>>>>>> and the help message:
>>>>>>
>>>>>> $ java -agentlib:jdwp=help
>>>>>> Java Debugger JDWP Agent Library
>>>>>> --------------------------------
>>>>>>
>>>>>> (see http://java.sun.com/products/jpda for more information)
>>>>>>
>>>>>> jdwp usage: java -agentlib:jdwp=[help]|[<option>=<value>, ...]
>>>>>>
>>>>>> Option Name and Value Description Default
>>>>>> --------------------- ----------- -------
>>>>>> suspend=y|n wait on startup? y
>>>>>> transport=<name> transport spec
>>>>>> none
>>>>>> address=<listen/attach address> transport spec ""
>>>>>> server=y|n listen for debugger? n
>>>>>> launch=<command line> run debugger on event
>>>>>> none
>>>>>> onthrow=<exception name> debug on throw
>>>>>> none
>>>>>> onuncaught=y|n debug on any uncaught? n
>>>>>> timeout=<timeout value> for listen/attach in milliseconds n
>>>>>> mutf8=y|n output modified utf-8 n
>>>>>> quiet=y|n control over terminal messages n
>>>>>>
>>>>>> Obsolete Options
>>>>>> ----------------
>>>>>> strict=y|n
>>>>>> stdalloc=y|n
>>>>>>
>>>>>> Examples
>>>>>> --------
>>>>>> - Using sockets connect to a debugger at a specific address:
>>>>>> java
>>>>>> -agentlib:jdwp=transport=dt_socket,address=localhost:8000 ...
>>>>>> - Using sockets listen for a debugger to attach:
>>>>>> java -agentlib:jdwp=transport=dt_socket,server=y,suspend=y ...
>>>>>>
>>>>>> Notes
>>>>>> -----
>>>>>> - A timeout value of 0 (the default) is no timeout.
>>>>>>
>>>>>> Warnings
>>>>>> --------
>>>>>> - The older -Xrunjdwp interface can still be used, but will be
>>>>>> removed in
>>>>>> a future release, for example:
>>>>>> java -Xdebug -Xrunjdwp:[help]|[<option>=<value>, ...]
>>>>>>
>>>>>>
>>>>>> I don't think your change requires any doc changes...
>>>>>>
>>>>>> Dan
>>>>>>
>>>>>>
>>>>>>> After the fix, JDWP server attempts to guess localhost address
>>>>>>> and bind
>>>>>>> to it only if no address is specified in command line but user can
>>>>>>> explicitly bind server to all of available addresses by using *
>>>>>>> as an
>>>>>>> address.
>>>>>>>
>>
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
More information about the serviceability-dev
mailing list