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

[serguei.spitsyn at oracle.com]

Dmitry,

I've reviewed the .01 webrev.

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.
>>>
>

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