RFR(M): 8061228 Allow JDWP socket connector to accept connections from certain ip addresses only
Daniel D. Daugherty
daniel.daugherty at oracle.com
Thu Aug 31 21:51:53 UTC 2017
I compared the .2 and .3 patches. Everything good except for
this whitespace change that didn't show up in the webrev (IIRC):
- /*
+ /*
* Start the transport loop in a separate thread
*/
I'll wait to see how we resolve the new "exit code path" issue
before giving the official thumbs up!
Dan
On 8/31/17 3:35 PM, serguei.spitsyn at oracle.com wrote:
> On 8/31/17 11:54, Daniel D. Daugherty wrote:
>> On 8/29/17 2:44 AM, serguei.spitsyn at oracle.com wrote:
>>> Hi Dan,
>>>
>>> Please, find the updated webrev's here:
>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8061228-jdi-transport.2/
>>
>> src/jdk.jdwp.agent/share/native/include/jdwpTransport.h
>> L202: /* 12: SetTransportConfiguration */
>> I missed updating this comment also. Perhaps:
>>
>> /* 12: SetTransportConfiguration added in
>> JDWPTRANSPORT_VERSION_1_1 */
>>
>> and add this one before L262:
>>
>> /* SetTransportConfiguration added in
>> JDWPTRANSPORT_VERSION_1_1 */
>
> Fixed.
>
>
>> src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c
>> L414: "invalid ip address in allow option");
>> Should this "ip" be "IP"?
>
> Fixed.
>
>> L435: if (++_peers_cnt >= MAX_PEER_ENTRIES) {
>> L436: fprintf(stderr, "Error in allow option:
>> '%s'\n", allowed_peers);
>> L437: RETURN_ERROR(JDWPTRANSPORT_ERROR_ILLEGAL_ARGUMENT,
>> L438: "exceeded max number of
>> allowed peers (32)");
>> L439: }
>> I think this error block will execute if you happen to
>> have exactly 32 allowed peers, i.e., "*s == 0" and I
>> don't think that's what you want.
>>
>> I think you want the check to be:
>>
>> if (_peers_cnt >= MAX_PEER_ENTRIES) {
>>
>> and you want that check to be before L433. Basically, if the
>> current count has overflowed, error out. Of course, you'll
>> want the "++peer_cnt" increment just before "if (*s == 0)".
>
> Nice catch.
> Fixed as you suggested.
>
>> L438: "exceeded max number of allowed peers (32)");
>> That literal '32' is a maintenance problem when you have
>> MAX_PEER_ENTRIES available.
>
> Fixed.
> Now, it is: "exceeded max number of allowed peers: " MAX_PEER_ENTRIES);
>
>>
>> L444: // advance to next ip block
>> Should this "ip" be "IP"?
>
> Fixed.
>
>
>> L623: static int option_was_printed = 0;
>> Variable is not used.
>
> Removed.
>
>
>> L645: if (err) {
>> Not your bug, but this if-statement should be:
>>
>> if (err != JDWPTRANSPORT_ERROR_NONE) {
>
> Fixed.
> I saw it too but was trying minimize the volume of review.
>
>
>>
>> src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
>> L574: EXIT_ERROR(map2jvmtiError(serror), "JDWP Transport
>> failed to initialize");
>> This is a new exit code path. Previously the process
>> did not exit here. Why the change in behavior?
>
> This improves the diagnosability.
> I investigated a situation with this error in transport initialization
> and was puzzled why the test was passed.
> This line fixed the issue.
> But I see another message on this topic from you.
> Will continue this discussion in reply on it.
>
>
>> src/jdk.jdwp.agent/share/native/libjdwp/transport.c
>> L208: jint supported_versions[2] =
>> {JDWPTRANSPORT_VERSION_1_1, JDWPTRANSPORT_VERSION_1_0};
>> Please consider adding a comment above this line:
>>
>> /* If a new version is added here, update 'case JNI_EVERSION'
>> below. */
>
> Done.
>
>
>> L463: info->transportVersion = transportVersion;
>> Perhaps init name, address and allowed_peers fields to NULL
>> here. I don't think jvmtiAllocate() guarantees NULL init.
>
> Added the initialization lines and removed a couple of lines
> for isServer case as they became dups.
>
>
>> L529: cfg.allowed_peers = info->allowed_peers;
>> In the 'goto handlerError' case on L534, you are publishing the
>> info->allowed_peers in cfg.allowed_peers, but you're going to
>> free it. Do you want to NULL out cfg.allowed_peers in the
>> 'goto handlerError' case?
>
> No need to do it as the cfg is a local.
>
>> L602: if (err != JDWPTRANSPORT_ERROR_NONE) {
>> In this error block, you added the free of 'info'. Nice catch!
>> Perhaps add a comment that the name, address and allowed_peers
>> fields in 'info' are not allocated in the non-server case so
>> they do not need to be freed.
>
> Added a comment.
>
>
>> src/jdk.jdwp.agent/share/native/libjdwp/transport.h
>> No comments.
>>
>> test/com/sun/jdi/BasicJDWPConnectionTest.java
>> L173: // Bad mix of allow address value with allow option '*'
>> L174: String allowOpt = ",allow=allow=127.0.0.1+*";
>> Sorry, I'm still puzzled by this test case. With the
>> description on L173, I would expect L174 to be:
>>
>> String allowOpt = ",allow=127.0.0.1+allow=*";
>
> Ok, I fixed the comments like this:
> 167 // Bad mix of allow option '*' with address value
> 168 String allowOpt = ",allow=*+allow=127.0.0.1";
> . . .
> 173 // Bad mix of allow address value with '*'
> 174 String allowOpt = ",allow=allow=127.0.0.1+*";
>
>
> Please, updated webrevs:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8061228-jdi-transport.3/
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8061228-jdi-transport.3.inc/
>
> The last one is relative to the webrev.2, not the Dmitry's webrev.18.
>
>
> Thanks a lot, Dan!
> Serguei
>
>
>
>>
>> Dan
>>
>>
>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8061228-jdi-transport.2.inc/
>>>
>>> I think, I've resolved all you comments/suggestions.
>>> The list of allowed peers is still not printed in
>>> socketTransport_accept()
>>> in case of a rejected peer (not sure, if it is very necessary at
>>> this point).
>>> The issue is that the allow option is not available at this point.
>>> Regenerating it from the _peers array is non-trivial and error-prone.
>>> I'll try to implement it, if you think it is important.
>>>
>>> The nsk.jdi and JTreg jdk_jdi test runs are in progress.
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>> On 8/28/17 15:12, serguei.spitsyn at oracle.com wrote:
>>>> Hi Dan,
>>>>
>>>> Thank you a lot for review!
>>>>
>>>>
>>>> On 8/28/17 11:00, Daniel D. Daugherty wrote:
>>>>> Resending with Dmitry's e-mail address included.
>>>>>
>>>>> Please delete the previous version.
>>>>>
>>>>>
>>>>> On 8/22/17 5:22 PM, serguei.spitsyn at oracle.com wrote:
>>>>>> Please, review another revision of the fix for the enhancement:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8061228
>>>>>>
>>>>>> CSR:
>>>>>> https://bugs.openjdk.java.net/browse/CCC-8061228
>>>>>>
>>>>>> The SCR is in the DRAFT state.
>>>>>> Joe suggested to consider this CSR approved and gave a GO for
>>>>>> integration.
>>>>>> It will be moved to the right state later when the CSR tools
>>>>>> are ready.
>>>>>> I'm still asking at least one reviewer to look at this CSR and
>>>>>> give a thumbs up.
>>>>>> It is to ensure everything is going in a right direction.
>>>>>> I'll finalize the CSR after that.
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8061228-jdi-transport.1/
>>>>>
>>>>> >
>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8061228-jdi-transport.2/
>>>>
>>>> You seems to looked at8061228-jdi.transport.2 that I generated
>>>> temporarily for myself and which is obsolete now.
>>>> The 8061228-jdi.transport.1 was sent for review and needs to be used.
>>>> I will consider and fix all the comments that are still relevant
>>>> for v1.
>>>>
>>>>>
>>>>> src/jdk.jdwp.agent/share/native/include/jdwpTransport.h
>>>>> Needs copyright year update.
>>>>
>>>> It was fixed in the The 8061228-jdi.transport.1.
>>>>
>>>>>
>>>>> L150: const char* allowed_peers; /* Peers allowed
>>>>> for connection */
>>>>> Please consider adding the following comment above this line:
>>>>>
>>>>> /* Field added in JDWPTRANSPORT_VERSION_1_1: */
>>>>>
>>>>> That should provide a hint to future maintainers about
>>>>> how to add fields to jdwpTransportConfiguration.
>>>>>
>>>>> src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c
>>>>> Needs copyright year update.
>>>>
>>>> It was fixed in the The 8061228-jdi.transport.1.
>>>>
>>>>>
>>>>> L31: #include <netinet/in.h>
>>>>> L34: #include <netinet/in.h>
>>>>> Duplicated includes. Would be easier to spot if the includes
>>>>> were sorted, but that doesn't seem to be the style in the
>>>>> file.
>>>>> For the includes that you add, can you sort those? I don't
>>>>> recommend sorting the existing ones since that would make this
>>>>> patch messier.
>>>>
>>>> Nice catch.
>>>> Fixed.
>>>>
>>>>>
>>>>> L396: while(1) {
>>>>> Please add space before '('.
>>>>
>>>> Fixed.
>>>>
>>>>>
>>>>> L408: // Input is not consumed, something bad
>>>>> happens
>>>>> typo: 'happens' -> 'happened'
>>>>
>>>> Fixed.
>>>>
>>>>>
>>>>> L410: RETURN_ERROR(JDWPTRANSPORT_ERROR_ILLEGAL_ARGUMENT,
>>>>> You don't print the current value of 's' before this error
>>>>> return like you did in the previous error return. Why?
>>>>
>>>> This is printed/fixed in v1.
>>>>
>>>>>
>>>>> L421: _peers_cnt += 1;
>>>>> Why not ++_peers_cnt or _peers_cnt++?
>>>>
>>>> As it is minor, I did not want to fix it to minimize my incremental
>>>> webrev.
>>>> Fixed now.
>>>>
>>>>
>>>>>
>>>>> I don't see any checks for overflow of MAX_PEER_ENTRIES in
>>>>> parseAllowedPeers().
>>>>
>>>> Nice catch.
>>>> Fixed.
>>>>
>>>>
>>>>>
>>>>> L590: fprintf(stderr, "ERROR: Peer not allowed to
>>>>> connect, peers_cnt: %d\n", _peers_cnt);
>>>>> _peers_cnt is not particular interesting. It might be
>>>>> more interesting to print info about the peer that's
>>>>> trying to connect and maybe the list of allowed peers
>>>>> (one time).
>>>>
>>>> The _peers_cnt value is not printed in the webrev.1.
>>>> I agree, it is better to print
>>>> I'm not sure in what form to print the details about the peer
>>>> that's trying to connect
>>>> Should I use something like this:
>>>> char buffer[20] = { 0 };
>>>> inet_ntop(AF_INET, &(sa.sin_addr), buffer, len);
>>>>
>>>>
>>>>>
>>>>> socketTransport_accept() is executing a "do {...} while
>>>>> (socketFD < 0);"
>>>>> loop with various return points due to errors. Your new
>>>>> "if (_peers_cnt > 0)" block short circuits the logic in the
>>>>> "if (err) {" block that manages the acceptTimeout variable
>>>>> so the time we spent waiting for the connection won't be
>>>>> counted against the overall timeout specified by the
>>>>> caller.
>>>>>
>>>>> Example:
>>>>> - Say the caller asks for a 30 second timeout.
>>>>> - After 25 seconds we get a connection from an
>>>>> unapproved peer.
>>>>> - We won't update acceptTimeout (decrement by 25
>>>>> seconds) so we won't return from the
>>>>> socketTransport_accept() call for 55 seconds.
>>>>>
>>>>> I think acceptTimeout management has to be refactored
>>>>> to be common to both the not-allowed-peer path and
>>>>> the error path.
>>>>>
>>>>> L935: int err;
>>>>> Can move this variable decl to this line:
>>>>>
>>>>> L955: err = parseAllowedPeers(allowed_peers);
>>>>
>>>> Good suggestion - fixed.
>>>>
>>>>
>>>>>
>>>>> src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
>>>>> No comments.
>>>>>
>>>>> src/jdk.jdwp.agent/share/native/libjdwp/transport.c
>>>>> Needs copyright year update.
>>>>
>>>> It was fixed in the The 8061228-jdi.transport.1.
>>>>
>>>>
>>>>>
>>>>> L150: if (name == NULL) {
>>>>> You should add a check for parameter 'info' after this block.
>>>>> 'info' should not be NULL either.
>>>>
>>>> Nice catch - fixed.
>>>>
>>>>
>>>>>
>>>>> L203: size_t i;
>>>>> This decl can be moved to this line:
>>>>>
>>>>> L209 for (i = 0; i < sizeof(supported_versions);
>>>>> ++i) {
>>>>
>>>> It is not a C++ code, so the declaration can not be moved to the
>>>> line 203.
>>>> At least, some C compilers would not accept it.
>>>>
>>>>
>>>>>
>>>>> L210-214: four space indents should be used.
>>>>
>>>> Fixed.
>>>>
>>>>>
>>>>> L224: ERROR_MESSAGE(("transport doesn't
>>>>> recognize supported versions"));
>>>>> Perhaps you should also list the supported versions that were
>>>>> tried so there's more failure info.
>>>>
>>>> Nice suggestion.
>>>> Fixed.
>>>>
>>>>
>>>>>
>>>>> L241: * even if info is already dealocated.
>>>>> Typo: 'dealocated' -> 'deallocated'
>>>>
>>>> Fixed.
>>>>
>>>>>
>>>>> L507-511: four space indents should be used.
>>>>> L513-523: four space indents should be used.
>>>>
>>>> Fixed.
>>>>
>>>>>
>>>>> L527-541: four space indents should be used, but I don't think
>>>>> the switch statement is a good idea. That logic block should
>>>>> be something like:
>>>>>
>>>>> err = (*trans)->StartListening(trans, address, &retAddress);
>>>>> if (err != JDWPTRANSPORT_ERROR_NONE) {
>>>>> printLastError(trans, err);
>>>>> serror = JDWP_ERROR(TRANSPORT_INIT);
>>>>> goto handleError;
>>>>> }
>>>>>
>>>>> if (info->transportVersion >= JDWPTRANSPORT_VERSION_1_1) {
>>>>> config.allowed_peers = info->allowed_peers;
>>>>> err = (*trans)->SetTransportConfiguration(trans, &config);
>>>>> if (err != JDWPTRANSPORT_ERROR_NONE) {
>>>>> printLastError(trans, err);
>>>>> serror = JDWP_ERROR(TRANSPORT_INIT);
>>>>> goto handleError;
>>>>> }
>>>>> }
>>>>>
>>>>> The error checking block at L544-548 is now above. Note
>>>>> that I don't see a reason to error here if the version
>>>>> is newer than JDWPTRANSPORT_VERSION_1_1.
>>>>
>>>> Agreed - fixed.
>>>> I was thinking about the same refactoring but decided to keep the
>>>> original minimize my update.
>>>> Also, please, note that the order of calls to
>>>> SetTransportConfiguration and StartListening is different in the
>>>> webrev.1.
>>>>
>>>>
>>>>>
>>>>> src/jdk.jdwp.agent/share/native/libjdwp/transport.h
>>>>> Needs copyright year update.
>>>>
>>>> It was fixed in the The 8061228-jdi.transport.1.
>>>>
>>>>
>>>>>
>>>>> test/com/sun/jdi/BasicJDWPConnectionTest.java
>>>>> L32-34 - imports should be sorted.
>>>>
>>>> Fixed.
>>>>
>>>>>
>>>>> L165: // Bad mix of option '*' with other adress values
>>>>> Typo: 'adress' -> 'address'
>>>>>
>>>>> Not sure I like that description though. Perhaps:
>>>>>
>>>>> // Bad mix of option '*' with bad allow address value
>>>>
>>>> Fixed.
>>>> The address should not be bad, so I've put the 127.0.0.1 there.
>>>> It looks like this now:
>>>>
>>>> 167 // Bad mix of allow option '*' with allow address value
>>>> 168 String allowOpt = ",allow=*+allow=127.0.0.1";
>>>>
>>>>
>>>>
>>>>>
>>>>> L171: // Bad mix of option '*' with other adress values
>>>>> Typo: 'adress' -> 'address'
>>>>>
>>>>> Not sure I like that description though because you
>>>>> don't have a correctly formed '*' option there. Perhaps:
>>>>>
>>>>> // Bad mix of bad allow address values with option '*':
>>>>> String allowOpt = ",allow=allow=0.0.0.0+allow=*";
>>>> Fixed.
>>>> The address should not be bad, so I've put the 127.0.0.1 there.
>>>> It looks like this now:
>>>>
>>>> 173 // Bad mix of allow address value with allow option '*'
>>>> 174 String allowOpt = ",allow=allow=127.0.0.1+*";
>>>>
>>>>
>>>>> So you have two bad ones before the good option '*'. Not
>>>>> sure if that's what you were really looking for though...
>>>>
>>>> Right.
>>>> A good address must be there, a bad address was used by mistake.
>>>>
>>>>
>>>>>
>>>>> I think that's it. I still need to review the CSR...
>>>>
>>>> Wow!
>>>> Good catches and nice suggestions.
>>>>
>>>> The updated webrev is (one comment has not been resolved yet):
>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8061228-jdi-transport.2/
>>>>
>>>>
>>>> The original 8061228-jdi-transport.2 was moved to
>>>> 8061228-jdi-transport.2.old .
>>>>
>>>>
>>>> Thanks a lot, Dan!
>>>> Serguei
>>>>
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>>>
>>>>>> The lastest webrev from Dmitry:
>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.18/
>>>>>>
>>>>>> Incremental webrev vs the latest webrev from Dmitry:
>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8061228-jdi-transport.1.inc/
>>>>>>
>>>>>>
>>>>>> Summary:
>>>>>> This enhancement was developed by Dmitry who left the team.
>>>>>> I don't know what email address to use to CC him at this point.
>>>>>> I hope, Dmitry will find this discussion and reply accordingly.
>>>>>> The latest webrev revision from Dmitry was v18 (please, see above).
>>>>>>
>>>>>> This revision covers the following:
>>>>>> - Cleanup for versioning negotiation protocol (back up to the
>>>>>> original).
>>>>>> Now the transport library supports both versions 1_0 and
>>>>>> 1_1 (newly introduced).
>>>>>> - The transport native interface was changed.
>>>>>> The function SetTransportConfiguration() is introduced
>>>>>> instead of the
>>>>>> StartListeningWithAllow(). It allows to the same transport
>>>>>> library to support
>>>>>> both old and new version of the transport interface. At
>>>>>> this point, the
>>>>>> new structure jdwpTransportConfiguration includes only one
>>>>>> field:
>>>>>> const char* allowed_peers;
>>>>>> But it can be extended in the future if any other update in
>>>>>> configuration
>>>>>> will be required.
>>>>>> - The unit test was updated to provide better coverage of the
>>>>>> corner cases
>>>>>> for 'allow' option introduced by this enhancement.
>>>>>> - Fixes to improve diagnosability.
>>>>>> - A couple of bugs/regressions were fixed so that all the JDI
>>>>>> tests are passed now.
>>>>>> - A cleanup that includes some renaming and reformatting.
>>>>>>
>>>>>>
>>>>>> Testing:
>>>>>> Tested new agent flag (allow), with new test:
>>>>>> jdk/test/com/sun/jdi/BasicJDWPConnectionTest.java
>>>>>> Ran the nsk.jdi, nsk.jdwp and jtreg jdk_jdi for both release
>>>>>> and fastdebug builds.
>>>>>> All tests are passed.
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20170831/93b04a3f/attachment-0001.html>
More information about the serviceability-dev
mailing list