Serguei, 1. New webrev with couple of issues addressed (see Robbin's e-mails):
http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.15 2. We have to keep transport version in global variable because old transport doesn't initialize reserved1 field and we are loosing version information after first disconnect. i.e. if I remove this "dancing" debugger is not able to connect second time. I'd reorganized code a bit for better readability. 3. ccc tool doesn't have JDK10 so we can't go forward. -Dmitry On 2017-03-17 12:20, serguei.spit...@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.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 -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
