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 18:54:07 UTC 2017
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/de25272d/attachment-0001.html>
More information about the serviceability-dev
mailing list