RFR(S): JDK-8041435 Make JDWP socket connector accept only local connections by default
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Fri May 16 18:24:58 UTC 2014
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.
>>>>>>
>
More information about the serviceability-dev
mailing list