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