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 20:40:07 UTC 2017
> 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?
I think this new exit code path is going to be a problem.
Here's where startTransport is called:
L722: transport_initialize();
L723: (void)bagEnumerateOver(transports, startTransport, &arg);
L724:
L725: /*
L726: * Exit with an error only if
L727: * 1) none of the transports was successfully started, and
L728: * 2) the application has not yet started running
L729: */
L730: if ((arg.error != JDWP_ERROR(NONE)) &&
L731: (arg.startCount == 0) &&
L732: initOnStartup) {
L733: EXIT_ERROR(map2jvmtiError(arg.error), "No transports
initialized");
L734: }
Suppose you have more than one transport in the 'transports' bag.
If you have an error in the first one's call to 'startTransport',
then you're going to exit at L574 and you won't get to the next
transport in the bag.
The intended error exit point for startTransport is L733.
Dan
On 8/31/17 12:54 PM, 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 */
>
> src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c
> L414: "invalid ip address in allow option");
> Should this "ip" be "IP"?
>
> 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)".
>
> L438: "exceeded max number of allowed
> peers (32)");
> That literal '32' is a maintenance problem when you have
> MAX_PEER_ENTRIES available.
>
> L444: // advance to next ip block
> Should this "ip" be "IP"?
>
> L623: static int option_was_printed = 0;
> Variable is not used.
>
> L645: if (err) {
> Not your bug, but this if-statement should be:
>
> if (err != JDWPTRANSPORT_ERROR_NONE) {
>
> 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?
>
> 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. */
>
> L463: info->transportVersion = transportVersion;
> Perhaps init name, address and allowed_peers fields to NULL
> here. I don't think jvmtiAllocate() guarantees NULL init.
>
> 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?
>
> 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.
>
> 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=*";
>
> 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/5f22d04b/attachment-0001.html>
More information about the serviceability-dev
mailing list