Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-8313804: JDWP support for -Djava.net.preferIPv6Addresses=system #15796

Closed
wants to merge 7 commits into from

Conversation

cushon
Copy link
Contributor

@cushon cushon commented Sep 18, 2023

Please consider this fix for JDK-8313804, which adds support to JDWP for -Djava.net.preferIPv6Addresses=system. Previously it only handled -Djava.net.preferIPv6Addresses=true and -Djava.net.preferIPv6Addresses=false.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8313804: JDWP support for -Djava.net.preferIPv6Addresses=system (Bug - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15796/head:pull/15796
$ git checkout pull/15796

Update a local copy of the PR:
$ git checkout pull/15796
$ git pull https://git.openjdk.org/jdk.git pull/15796/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15796

View PR using the GUI difftool:
$ git pr show -t 15796

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15796.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 18, 2023

👋 Welcome back cushon! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Sep 18, 2023
@openjdk
Copy link

openjdk bot commented Sep 18, 2023

@cushon The following label will be automatically applied to this pull request:

  • serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the serviceability serviceability-dev@openjdk.org label Sep 18, 2023
@mlbridge
Copy link

mlbridge bot commented Sep 18, 2023

@openjdk
Copy link

openjdk bot commented Sep 18, 2023

@cushon Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

@plummercj
Copy link
Contributor

Is the code starting at lines 748 and 970 still correct after your changes?

* Reads java.net.preferIPv6Addresses system value, sets preferredAddressFamily to
* - AF_INET6 if the property is "true";
* - AF_INET if the property is "false".
* - 0 if the property is "false".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you meant "system", not "false".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, updated to AF_UNSPEC if the property is "system"

} else if (strcmp(theValue, "false") == 0) {
preferredAddressFamily = AF_INET;
} else if (strcmp(theValue, "system") == 0) {
preferredAddressFamily = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would AF_UNSPEC (which is 0) be a better choice than a literal 0 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM, thanks!

@cushon
Copy link
Contributor Author

cushon commented Sep 18, 2023

Is the code starting at lines 748 and 970 still correct after your changes?

I think so, but scrutiny there is very welcome. I expected additional changes to be necessary.

  • For 748, it's handling a situation where IPv4 isn't specifically preferred. The implementation comment notes 'if preferredAddressFamily is AF_INET6 or not set'.
  • For 970 the first loop won't match, and on the second loop it will try in the order provided by getaddrinfo, which is what we want.

If that sounds right, suggestions for making it clearer / more explicit are welcome.

@dholmes-ora
Copy link
Member

Similar to @plummercj comment what about this code at L730:

// Try to find bind address of preferred address family first.
    for (ai = addrInfo; ai != NULL; ai = ai->ai_next) {
        if (ai->ai_family == preferredAddressFamily) {
            listenAddr = ai;
            break;
        }
    }

with AF_INET or AF_INET6 this would find a match, but with AF_UNSPEC then it seems it will just leave ai pointing to the last entry. ???

@cushon
Copy link
Contributor Author

cushon commented Sep 19, 2023

Similar to @plummercj comment what about this code at L730:

// Try to find bind address of preferred address family first.
    for (ai = addrInfo; ai != NULL; ai = ai->ai_next) {
        if (ai->ai_family == preferredAddressFamily) {
            listenAddr = ai;
            break;
        }
    }

with AF_INET or AF_INET6 this would find a match, but with AF_UNSPEC then it seems it will just leave ai pointing to the last entry. ???

I think that's covered by the logic that follows it on L738. If that loop fails to initialize listenAddr, it gets set to the first address, which I think is what we want:

    if (listenAddr == NULL) {
        // No address of preferred address family found, grab the first one.
        listenAddr = &(addrInfo[0]);
    }

@dholmes-ora
Copy link
Member

Okay I see now that ai is not important. We won't have set listenAddr so we will take the first addrInfo. But this seems somewhat random behaviour as the first addrInfo could be either V4 or V6 (what controls the order?). But we then execute the if at L748 regardless because the preferred value is not AF_INET even though the chosen address may still be a v4 address. This may all "just work" but I find it hard to discern what the correct/expected behaviour is if using system (which seems to mean "don't care").

@cushon
Copy link
Contributor Author

cushon commented Sep 19, 2023

The order of the addrInfos is determined by getaddrinfo(), which is supposed to sort the results according to RFC 6724. system is supposed to preserve that order instead of re-ordering them.

There's some related discussion in JDK-8170568 about use-cases that IPv4-first and IPv6-first don't support. The bug details some edge cases that still aren't well supported when using -Djava.net.preferIPv6Addresses=system, but in the cases I've seen system is usually fine, aside from this issue with JDWP ignoring that setting.

new ListenTest("localhost", ipv4Address)
.preferIPv4Stack(true)
.preferIPv6Addresses("system")
.run(TestResult.Success);
if (ipv6Address != null) {
// - only IPv4, so connection prom IPv6 should fail
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an existing typo here, I assume "prom" should be "from"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree: that is pre-existing, and it is a typo for 'from'. I am happy to fix it while I'm here if you like.

@plummercj
Copy link
Contributor

Similar to @plummercj comment what about this code at L730:

// Try to find bind address of preferred address family first.
    for (ai = addrInfo; ai != NULL; ai = ai->ai_next) {
        if (ai->ai_family == preferredAddressFamily) {
            listenAddr = ai;
            break;
        }
    }

with AF_INET or AF_INET6 this would find a match, but with AF_UNSPEC then it seems it will just leave ai pointing to the last entry. ???

Wouldn't ai be NULL?

@cushon
Copy link
Contributor Author

cushon commented Sep 19, 2023

Similar to @plummercj comment what about this code at L730:

// Try to find bind address of preferred address family first.
    for (ai = addrInfo; ai != NULL; ai = ai->ai_next) {
        if (ai->ai_family == preferredAddressFamily) {
            listenAddr = ai;
            break;
        }
    }

with AF_INET or AF_INET6 this would find a match, but with AF_UNSPEC then it seems it will just leave ai pointing to the last entry. ???

Wouldn't ai be NULL?

I think so, but does the value of ai when the loop exits matter? I don't see any reads of that value. There's another use of ai on L752, but it gets reinitialized first.

I could update this to something like the following, which I think will result in equivalent behavior but is possibly more readable, and avoids unnecessarily iterating over all the addresses:

    // Try to find bind address of preferred address family first.
    for (ai = addrInfo; ai != NULL; ai = ai->ai_next) {
-        if (ai->ai_family == preferredAddressFamily) {
+        if (ai->ai_family == preferredAddressFamily || preferredAddressFamily == AF_UNSPEC) {
            listenAddr = ai;
            break;
        }
    }

@alexmenkov
Copy link

The loop is not needed for AF_UNSPEC, just make it clear:

// Try to find bind address of preferred address family first (if java.net.preferIPv6Addresses != "system").
if (preferredAddressFamily != AF_UNSPEC) {
    for (ai = addrInfo; ai != NULL; ai = ai->ai_next) {
        if (ai->ai_family == preferredAddressFamily) {
            listenAddr = ai;
            break;
        }
    }
}

if (listenAddr == NULL) {
    // No address of preferred address family found or java.net.preferIPv6Addresses == "system",
    // use the first address returned by getaddrinfo.
    listenAddr = &(addrInfo[0]);
}

@alexmenkov
Copy link

1st pass in attach (line 970) is also not needed, can be something like

-    /* 1st pass - preferredAddressFamily (by default IPv4), 2nd pass - the rest */
+    // 1st pass - preferredAddressFamily (by default IPv4), 2nd pass - the rest;
+    // if java.net.preferIPv6Addresses == "system", only 2nd pass is needed
+  pass = preferredAddressFamily != AF_UNSPEC ? 0 : 1;
-    for (pass = 0; pass < 2 && socketFD < 0; pass++) {
+    for (; pass < 2 && socketFD < 0; pass++) {
        for (ai = addrInfo; ai != NULL; ai = ai->ai_next) {

@cushon
Copy link
Contributor Author

cushon commented Sep 19, 2023

Thanks @alexmenkov!

The loop is not needed for AF_UNSPEC, just make it clear

Done

1st pass in attach (line 970) is also not needed, can be something like

Done

@plummercj
Copy link
Contributor

plummercj commented Sep 19, 2023

For 748, it's handling a situation where IPv4 isn't specifically preferred. The implementation comment notes 'if preferredAddressFamily is AF_INET6 or not set'.

What does "specifically preferred" mean? By default it will be set to AF_INET. If it is set to AF_INET, you can't tell if java.net.preferIPv6Addresses=false was specified (which implies AF_INET), or if nothing was specified and it just defaulted to AF_INET. The comment and the code read as if preferredAddressFamily will be something other than AF_INET unless java.net.preferIPv6Addresses=false is used. Maybe the code is correct and the comment needs some cleaning up. TBH I don't understand what this code is suppose to do.

 743     // Binding to IN6ADDR_ANY allows to serve both IPv4 and IPv6 connections,
 744     // but binding to mapped INADDR_ANY (::ffff:0.0.0.0) allows to serve IPv4
 745     // connections only. Make sure that IN6ADDR_ANY is preferred over
 746     // mapped INADDR_ANY if preferredAddressFamily is AF_INET6 or not set.
 747 
 748     if (preferredAddressFamily != AF_INET) {
 749         inet_pton(AF_INET6, "::ffff:0.0.0.0", &mappedAny);
 750 
 751         if (isEqualIPv6Addr(listenAddr, mappedAny)) {
 752             for (ai = addrInfo; ai != NULL; ai = ai->ai_next) {
 753                 if (isEqualIPv6Addr(listenAddr, in6addr_any)) {
 754                     listenAddr = ai;
 755                     break;
 756                 }
 757             }
 758         }
 759     }

@plummercj
Copy link
Contributor

Okay I see now that ai is not important. We won't have set listenAddr so we will take the first addrInfo. But this seems somewhat random behaviour as the first addrInfo could be either V4 or V6 (what controls the order?). But we then execute the if at L748 regardless because the preferred value is not AF_INET even though the chosen address may still be a v4 address. This may all "just work" but I find it hard to discern what the correct/expected behaviour is if using system (which seems to mean "don't care").

The only time listenAddr is not set is when using AF_UNSPEC ("system"). In this case the proper thing to do is grab the first entry, since this is the entry that the "system" prefers that we use. It could be v4 or v6. I think Alex has commented that this can be made more obvious with a check for AF_UNSPEC.

@cushon
Copy link
Contributor Author

cushon commented Sep 19, 2023

I think Alex has commented that this can be made more obvious with a check for AF_UNSPEC.

Thanks, yes, I have applied that suggestion.

For 748, it's handling a situation where IPv4 isn't specifically preferred. The implementation comment notes 'if preferredAddressFamily is AF_INET6 or not set'.

What does "specifically preferred" mean? By default it will be set to AF_INET. If it is set to AF_INET, you can't tell if java.net.preferIPv6Addresses=false was specified (which implies AF_INET), or if nothing was specified and it just defaulted to AF_INET. The comment and the code read as if preferredAddressFamily will be something other than AF_INET unless java.net.preferIPv6Addresses=false is used. Maybe the code is correct and the comment needs some cleaning up. TBH I don't understand what this code is suppose to do.

I agree 'preferredAddressFamily is AF_INET6 or not set' is unclear, since preferredAddressFamily was only ever AF_INET or AF_INET6. I wondered if the use of if (preferredAddressFamily != AF_INET) { instead of if (preferredAddressFamily == AF_INET6) { and the comment about 'not set' was referring to the possibility of AF_UNSPEC. The history for this logic is in

The bug mentions it fixed an issue on Alpine Linux, and I don't have convenient access to a test environment for that.

I could update L750 to if (preferredAddressFamily == AF_INET6) { to prevent this logic from re-ordering address when java.net.preferIPv6Addresses=system is set.

What do you think?

@alexmenkov
Copy link

I think Alex has commented that this can be made more obvious with a check for AF_UNSPEC.

Thanks, yes, I have applied that suggestion.

For 748, it's handling a situation where IPv4 isn't specifically preferred. The implementation comment notes 'if preferredAddressFamily is AF_INET6 or not set'.

What does "specifically preferred" mean? By default it will be set to AF_INET. If it is set to AF_INET, you can't tell if java.net.preferIPv6Addresses=false was specified (which implies AF_INET), or if nothing was specified and it just defaulted to AF_INET. The comment and the code read as if preferredAddressFamily will be something other than AF_INET unless java.net.preferIPv6Addresses=false is used. Maybe the code is correct and the comment needs some cleaning up. TBH I don't understand what this code is suppose to do.

I agree 'preferredAddressFamily is AF_INET6 or not set' is unclear, since preferredAddressFamily was only ever AF_INET or AF_INET6. I wondered if the use of if (preferredAddressFamily != AF_INET) { instead of if (preferredAddressFamily == AF_INET6) { and the comment about 'not set' was referring to the possibility of AF_UNSPEC. The history for this logic is in

* https://bugs.openjdk.org/browse/JDK-8250630

* https://mail.openjdk.org/pipermail/serviceability-dev/2020-August/thread.html#32673

The bug mentions it fixed an issue on Alpine Linux, and I don't have convenient access to a test environment for that.

I could update L750 to if (preferredAddressFamily == AF_INET6) { to prevent this logic from re-ordering address when java.net.preferIPv6Addresses=system is set.

What do you think?

I took some time to understand what this code for.
I think the condition is not correct. it should be

if (!allowOnlyIPv4) {

allowOnlyIPv4 (reflects "java.net.preferIPv4Stack" sys.prop) means that only IPv4 connections are allowed (even if IPv6 available)

@dholmes-ora
Copy link
Member

The only time listenAddr is not set is when using AF_UNSPEC ("system").

@plummercj no it can also be unset if there are no listed addresses for the preferred family e.g. you prefer v6 but they are all v4, or vice-versa. The new code makes the UNSPEC case much clearer anyway.

@cushon
Copy link
Contributor Author

cushon commented Sep 20, 2023

I took some time to understand what this code for. I think the condition is not correct. it should be

if (!allowOnlyIPv4) {

allowOnlyIPv4 (reflects "java.net.preferIPv4Stack" sys.prop) means that only IPv4 connections are allowed (even if IPv6 available)

I have tentatively updated it to do that

Copy link
Contributor

@plummercj plummercj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like the "IPv6-only system" part of the test should include more test cases, both passing and failing. I'm not sure why it was lmitted to just the one:

            // IPv6-only system - expected to fail on IPv4 address
            new ListenTest("localhost", ipv6Address)
                    .preferIPv4Stack(true)
                    .run(TestResult.ListenFailed);

test/jdk/com/sun/jdi/JdwpNetProps.java Show resolved Hide resolved
test/jdk/com/sun/jdi/JdwpNetProps.java Show resolved Hide resolved
@cushon
Copy link
Contributor Author

cushon commented Sep 21, 2023

It seems like the "IPv6-only system" part of the test should include more test cases, both passing and failing. I'm not sure why it was lmitted to just the one:

I have added additional test cases

Copy link
Contributor

@plummercj plummercj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think overall the changes look good. Before approving I'd like to run it on our internal test system, which has a very large number of hosts and platform types, but I'll wait to make sure there are no other changes before doing that.

@openjdk
Copy link

openjdk bot commented Sep 22, 2023

@cushon This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8313804: JDWP support for -Djava.net.preferIPv6Addresses=system

Reviewed-by: cjplummer, amenkov

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 99 new commits pushed to the master branch:

  • 68a9c81: 8316711: SEGV in LoaderConstraintTable::find_loader_constraint after JDK-8310874
  • e015e6c: 8315825: Open some swing tests
  • 9aaac2e: 8301639: JDI and JDWP specs should clarify potential deadlock issues with method invocation
  • 6b8261b: 8315464: Uncouple AllClassesIndexWriter from IndexBuilder
  • 9b65b7d: 8316305: Initial buffer size of StackWalker is too small caused by JDK-8285447
  • 53516ae: 8304956: Update KeyStore.getDefaultType​() specification to return pkcs12 as fallback
  • 373cdf2: 8315960: test/jdk/java/io/File/TempDirDoesNotExist.java leaves test files behind
  • f7578e8: 8316540: StoreReproducibilityTest fails on some locales
  • b66ded9: 8316688: Widen allowable error bound of Math.hypot
  • 6c61bc1: 8316514: Better diagnostic header for VtableStub
  • ... and 89 more: https://git.openjdk.org/jdk/compare/4421951d8f1c6fb16255851a803252fe96a453e5...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Sep 22, 2023
Copy link
Contributor

@plummercj plummercj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, and the changes passed all my testing.

@cushon
Copy link
Contributor Author

cushon commented Sep 25, 2023

/integrate

@openjdk
Copy link

openjdk bot commented Sep 25, 2023

Going to push as commit 9291b46.
Since your change was applied there have been 111 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Sep 25, 2023
@openjdk openjdk bot closed this Sep 25, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 25, 2023
@openjdk
Copy link

openjdk bot commented Sep 25, 2023

@cushon Pushed as commit 9291b46.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated serviceability serviceability-dev@openjdk.org
5 participants