RFR(S): JDK-8041435 Make JDWP socket connector accept only local connections by default

[Dmitry Samersoff]

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.