RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

Thu Apr 6 19:57:39 UTC 2017

Serguei,

(resending lost e-mail)

Please, see:

http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.15/

Nits fixed.

> It is not clear why do you
> need the static variable transportVersion and this dance with it.

We have to keep transport version in the global variable because

void *reserved1; (_extra_data in new code)

is not initialized by old transport and we can't rely on it's content
but have to keep transport version between subsequent connections.

I'd reorganized this part of code a bit for better readability.

-Dmitry

On 2017-03-17 12:20, serguei.spitsyn at oracle.com wrote:
> Dmitry,
> 
> 
> Some quick comments on the webrev.12.
> 
> 
> The style of comments must be /* */, not //.
> 
> 
> http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.12/src/jdk.jdwp.agent/share/native/libjdwp/transport.c.frames.html
> 
> 
> 205 // Pass MAXIMAL supported version, expect actual transport version in
> 
>    Would it better to replace 'MAXIMAL' with 'the latest' ?
> 
> 
> 516 // interface version 1.0 doesn't support versioning, so we have to
> 517 // a use global variable and set the version artifically.
> 518 // Use (*t)->extra_data->version directly when 1.0 is gone
> 
>   516: interface => Interface
>   517: Typo: a use => use
>   518: Dot is missed at the end.
> 
> 
> 33 static unsigned transportVersion = JDWPTRANSPORT_VERSION_1_0; ... 207
> ver = (*onLoad)(jvm, &callback, JDWPTRANSPORT_VERSION_1_1, &t);
> 208 if (ver == JNI_EVERSION) {
>  209             ver = (*onLoad)(jvm, &callback,
> JDWPTRANSPORT_VERSION_1_0, &t);
> 210 // Special handling for versionless transports
> 211 info->transportVersion = JDWPTRANSPORT_VERSION_1_0;
> 212 }
> 213 else {
> 214 info->transportVersion = (*t)->extra_data->version;
> 215 } ...263 transportVersion = pTransportVersion; ... 459
> info->transportVersion = transportVersion; It is not clear why do you
> need the static variable transportVersion and this dance with it. It
> seems, the transport version is always enforced by the TransportOnLoad
> function anyway. At the line 459, you could just have:
> 459 info->transportVersion = JDWPTRANSPORT_VERSION_1_0; Thanks, Serguei
> 
> On 3/17/17 00:03, serguei.spitsyn at oracle.com wrote:
>> Hi Dmitry, On 3/15/17 00:56, Dmitry Samersoff wrote:
>>> Serguei,
>>>> I still see the C .vs. C++ related change in the jdwpTransport.h. 
>>> done. http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.12/ 
>> Good. Thank you for the update!
>>> see also inline. On 2017-03-15 00:40, serguei.spitsyn at oracle.com wrote:
>>>> Hi Dmitry, We already had a big review thread back in 2014 on this.
>>>> I think, it is again going in the wrong order. First, I think, it is
>>>> better to start from a CCC (or its equivalent, CSR). This will allow
>>>> to focus on and sort out the changes in interface/behavior first
>>>> before going into the implementation details. 
>>> CCC was filed and approved. The only reason I withdraw it - the fix
>>> didn't go to jdk9 but CCC tool doesn't have jdk10. CSR process is
>>> also not yet implemented. 
>> The CSR preview message from Joe is attached. My understanding is that
>> we can continue to use CCC. At some points the CCC process will be
>> moved to the CSR. The CCC is out of date now as it does not match the
>> webrev 12. Also, I do not like the addition of new function
>> StartListening11() next to StartListening(). Does it mean, that for
>> transport version 1.2 we might have more function clones with 12
>> suffix? Perhaps, we need something smarter here but I'm unsure yet
>> what it has to be.
>>>> Second, I'd suggest to separate a couple of other things. I still
>>>> see the C .vs. C++ related change in the jdwpTransport.h. It is
>>>> better to leave it along for now as it was suggested in early review
>>>> rounds. If you still want to fix it then it is better to file a
>>>> separate bug that should include the JNI as well (as it was
>>>> discussed with Alan before). 
>>> Do you mean?    39 #ifdef __cplusplus    40 extern "C" {    41 #endif
>>> I'll add it back to avoid any confusion. 
>> Yes. I see you added it back in new version of webrev.
>>>> Also, I'm thinking if it is a good idea to separate the transport
>>>> versioning to an RFE. It would allow to focus on this aspect as
>>>> necessary. In this case, the 8061228 will depend on the versioning.
>>>> However, it is much more simple and can be resolved faster. 
>>> It's hard to test versioning code without implementation of new,
>>> VERSION_1_1 transport. Network part of 8061228 is simple and clear
>>> separated from versioning, so I would prefer to keep it together in
>>> one CR/one push. 
>> No pressure. I've got your point above. Thanks, Serguei
>>> Restriction turned off by default (I'll file separate CR to enable it
>>> later), so we have enough time to fix any issues that might come.
>>> -Dmitry
>>>> Thanks, Serguei On 2/28/17 01:41, Dmitry Samersoff wrote:
>>>>> Everybody, Please review:
>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.10/ These
>>>>> changes introduce new parameter[1] of the socket transport - allow.
>>>>> Users can explicitly specify a list of hosts that allowed to
>>>>> connect to jdwp server and it's the second part of JDWP
>>>>> hardening[2]. No restrictions are applied by default now but I'll
>>>>> file a separate CR to restrict list of allowed peers to localhost
>>>>> by default. Also these changes implement versioning for jdwp
>>>>> transport and therefor simplify feature development of jdwp. 1.
>>>>> Example command line:
>>>>> -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,
>>>>> address=*,allow="127.0.0.0/8;192.168.0.0/24" Possible values for
>>>>> allow parameter:     *           - accept connections from
>>>>> everywhere.     N.N.N.N     - accept connections from this IP
>>>>> address only     N.N.N.N/nn  - accept connections from particular
>>>>> ip subnet 2. JDK-8052136 JDWP hardening -Dmitry 

-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.