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

Thu May 15 23:19:21 UTC 2014

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
>>>                 --------------------------------
>>>
>>>    (seehttp://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.
>>>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20140515/d767db29/attachment.html>