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

8275640 (win) java.net.NetworkInterface issues with IPv6-only environments #6090

Closed
wants to merge 13 commits into from

Conversation

djelinski
Copy link
Member

@djelinski djelinski commented Oct 23, 2021

Clean up of various issues related to error handling and memory management


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8275640: (win) java.net.NetworkInterface issues with IPv6-only environments

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6090/head:pull/6090
$ git checkout pull/6090

Update a local copy of the PR:
$ git checkout pull/6090
$ git pull https://git.openjdk.java.net/jdk pull/6090/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6090

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6090.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 23, 2021

👋 Welcome back djelinski! 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 Oct 23, 2021
@openjdk
Copy link

openjdk bot commented Oct 23, 2021

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

  • net

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 net net-dev@openjdk.org label Oct 23, 2021
@mlbridge
Copy link

mlbridge bot commented Oct 23, 2021

@dfuch
Copy link
Member

dfuch commented Oct 27, 2021

Hi Daniel - it looks like your changeset is reverting all the changes that were made by JDK-8046500, to fix an issue where no interfaces were returned on machines that had only IPv6 interfaces (and no IPv4). See https://mail.openjdk.java.net/pipermail/net-dev/2018-November/011931.html ;

Were you able to test that NetworkInterface.getNetworkInterfaces() will not fail on a machine where IPv4 has been disabled with netsh interface ipv4 uninstall ?

@djelinski
Copy link
Member Author

djelinski commented Oct 27, 2021

Hi, I wasn't able to run the test yet; need to find a machine where I could run this test without worrying about restoring it to a working state.
However, looking at the code, I think it's broken already. JDK-8046500 says GetIpAddrTable fails on IPv6-only machines. This method is only called in lookupIPAddrTable, and getAllInterfacesAndAddresses ignores -2 result since 7f1f9a5.
Still, enumAddresses_win probably needs more love. I'll look into it.

@djelinski
Copy link
Member Author

djelinski commented Oct 28, 2021

I was able to find and fix the problems with IPv6-only operation; all of the following:
NetworkInterface.getNetworkInterfaces()
NetworkInterface.getByName
NetworkInterface.getByIndex
NetworkInterface.getByInetAddress
NetworkInterface#isUp
succeeded and returned correct data on a machine without IPv4 after f440a6e was applied.
While working on this I noticed that:

  • building on IPv6-only machine doesn't work; I got an error message mentioning Sjavac and No port file values materialized. I guess I'll revisit this later.
  • JTreg tests on IPv6-only machine don't work; I only get a cryptic TEST FAILURE message (but if the tests successfully passed earlier when IPv4 was enabled, I get TEST SUCCESS, probably cached somewhere). I don't even know how to start troubleshooting this.

@dfuch
Copy link
Member

dfuch commented Oct 28, 2021

Hi Daniel, thanks for the update - I will have a look.

Concerning IPv6-only environment - it's a bit of a whack a mole game at the moment. We have updated many tests to not assume that IPv4 was available, but unfortunately I suspect that we still have many tests that do depend on IPv4 - and that don't have the logic to either pick IPv6 or skip...

We had an openjdk contributor at one time who were interested in moving this forward but I believe they have moved to other interests...
Still - we are trying to make sure that new code (especially in networking area) doesn't make any assumption on the availability of either stack.

@djelinski
Copy link
Member Author

djelinski commented Oct 28, 2021

Just to clarify, no tests were run. The failure was reported by JTReg itself. As of now building Java on Windows requires IPv4.

On a side note, Microsoft created a completely new set of network interface-related APIs in Windows Vista. It looks like they could simplify our getByXXX methods, and possibly we could also use the same method to extract both IPv4 and IPv6 addresses. Do you think we could replace the pre-Vista code and start using the new functions?

@dfuch
Copy link
Member

dfuch commented Oct 28, 2021

Just to clarify, no tests were run. The failure was reported by JTReg itself. As of now building Java on Windows requires IPv4.

Understood. I'm satisfied that you manually tested the results on your machine - and if that change passes the tier1, tier2 in our CI I believe I'd be happy (I still need to review it though).

On a side note, Microsoft created a completely new set of network interface-related APIs in Windows Vista. It looks like they could simplify our getByXXX methods, and possibly we could also use the same method to extract both IPv4 and IPv6 addresses. Do you think we could replace the pre-Vista code and start using the new functions?

Yes - I saw that there were newer versions of some of these APIs - I was wondering too if using them would simplify our code. On the one hand it would be very good to simplify the code and use more modern API. On the other hand that would probably deserve more testing - possibly setting up some machines with special configurations to make sure we're not introducing new regressions. Let me think on it and seek other's opinions...

I assume we're talking about using GetIfTable2 and GetIfEntry2 or are there even newer APIs? But that wouldn't dispense us of calling GetAdaptersAddresses too - would it?

@djelinski
Copy link
Member Author

djelinski commented Oct 28, 2021

I still need to review it though

Each commit tries to fix one function (memory allocation / deallocation / return values); it may be easier to review them one by one.

I assume we're talking about using GetIfTable2 and GetIfEntry2

Yes, that's what I was talking about.
And yes, looks like GetAdaptersAddresses is still the best method to retrieve adapter-to-address mapping. For a moment I thought GetUnicastIpAddressEntry might be able to find adapter by IP address, but apparently that is not the case.

@djelinski
Copy link
Member Author

djelinski commented Nov 22, 2021

@dfuch were you able to review this? Anything I could help with?

@dfuch
Copy link
Member

dfuch commented Nov 29, 2021

Hi Daniel, I believe this is a good fix and we definitely want it in.
However, it is hard to test - and we don't have a regression test for that that would work in our CI.
I have talked with the other members of the team - and it was suggested that it could be more prudent to push such changes in the very early development stages of a release rather than at the end. So I would suggest we wait a few days more and integrate this for JDK 19 after the RDP1 fork. This would give this fix some more time to bake.
The same applies to your other PR (JDK-8262442).
Does that make sense to you?

@djelinski
Copy link
Member Author

djelinski commented Nov 29, 2021

(..) wait a few days more and integrate this for JDK 19 after the RDP1 fork. (..) Does that make sense to you?

Makes perfect sense. At least now I know what I'm waiting for :) Thanks!

@djelinski
Copy link
Member Author

djelinski commented Dec 14, 2021

RDP1 is out now; can I get this reviewed? Thank you!

@msheppar
Copy link

msheppar commented Jan 8, 2022

I'm working through a number of related issues in this area .... I have applied your changes and run InetAddress and NetworkInterface sanity checks, and encountered some problems with these changes - as such we're working through the failures analysis - will provide approriate feedback in due course

Copy link
Member Author

@djelinski djelinski left a comment

Thanks for the review! Let me know if I can assist with the failure analysis.

@@ -274,20 +267,23 @@ int getAllInterfacesAndAddresses (JNIEnv *env, netif **netifPP)
// Retrieve IPv4 addresses with the IP Helper API
curr = *netifPP;
ret = lookupIPAddrTable(env, &tableP);
if (ret < 0) {
if (ret == -1) {
free_netif(*netifPP);
Copy link
Member

@dfuch dfuch Jan 26, 2022

Choose a reason for hiding this comment

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

*netifPP might be null here.

Copy link
Member Author

@djelinski djelinski Jan 26, 2022

Choose a reason for hiding this comment

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

free_netif (and all other free functions) handle null input just fine.

if ((*env)->ExceptionCheck(env)) {
(*env)->ExceptionClear(env);
}
tableP = NULL;
}
while (curr != NULL) {
Copy link
Member

@dfuch dfuch Jan 26, 2022

Choose a reason for hiding this comment

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

Maybe we should not enter here if tableP == null. Looks like it should be while ( curr != NULL && tableP != null) - either that or guard the block with an if (tableP != NULL) ?

@@ -301,7 +297,8 @@ int getAllInterfacesAndAddresses (JNIEnv *env, netif **netifPP)
flags |= GAA_FLAG_INCLUDE_PREFIX;
Copy link
Member

@dfuch dfuch Jan 26, 2022

Choose a reason for hiding this comment

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

tableP could be null at lime 293 (new file - line 297 old file) - free(tableP) should be guarded by a null check

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 7, 2022

@djelinski This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@djelinski
Copy link
Member Author

djelinski commented Mar 7, 2022

Hi!
Would it help with the review if I split this patch into smaller portions? It contains 20-ish independent fixes for memory leaks, invalid free, and incorrect return value handling; I could easily put each of these changes in a separate PR if that helps.

@openjdk
Copy link

openjdk bot commented Mar 8, 2022

@djelinski 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:

8275640: (win) java.net.NetworkInterface issues with IPv6-only environments

Reviewed-by: msheppar, dfuchs

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 636 new commits pushed to the master branch:

  • 3e4dfc6: 8282295: SymbolPropertyEntry::set_method_type fails with assert
  • 0cbc4b8: 8281266: [JVMCI] MetaUtil.toInternalName() doesn't handle hidden classes correctly
  • 0f88fc1: 8282769: BSD date cannot handle all ISO 8601 formats
  • c6d743f: 8282770: Set source date in jib profiles from buildId
  • 5fab27e: 8282144: RandomSupport.convertSeedBytesToLongs sign extension overwrites previous bytes
  • 65ca0a5: 8276333: jdk/jfr/event/oldobject/TestLargeRootSet.java failed "assert(!contains(edge->reference())) failed: invariant"
  • e607287: 8282728: Serial: Remove unused BlockOffsetArray::Action
  • 8b45dbd: 8282312: Minor corrections to evbroadcasti32x4 intrinsic on x86
  • 3f0684d: 8275775: Add jcmd VM.classes to print details of all classes
  • cde923d: 8282690: runtime/CommandLine/VMDeprecatedOptions.java fails after JDK-8281181
  • ... and 626 more: https://git.openjdk.java.net/jdk/compare/9f30ec174faae10484766308996cab136a779658...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@msheppar, @dfuch) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 8, 2022
dfuch
dfuch approved these changes Mar 8, 2022
@djelinski
Copy link
Member Author

djelinski commented Mar 8, 2022

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 8, 2022
@openjdk
Copy link

openjdk bot commented Mar 8, 2022

@djelinski
Your change (at version cdb86b5) is now ready to be sponsored by a Committer.

@dfuch
Copy link
Member

dfuch commented Mar 8, 2022

/sponsor

@openjdk
Copy link

openjdk bot commented Mar 8, 2022

Going to push as commit 2549e55.
Since your change was applied there have been 636 commits pushed to the master branch:

  • 3e4dfc6: 8282295: SymbolPropertyEntry::set_method_type fails with assert
  • 0cbc4b8: 8281266: [JVMCI] MetaUtil.toInternalName() doesn't handle hidden classes correctly
  • 0f88fc1: 8282769: BSD date cannot handle all ISO 8601 formats
  • c6d743f: 8282770: Set source date in jib profiles from buildId
  • 5fab27e: 8282144: RandomSupport.convertSeedBytesToLongs sign extension overwrites previous bytes
  • 65ca0a5: 8276333: jdk/jfr/event/oldobject/TestLargeRootSet.java failed "assert(!contains(edge->reference())) failed: invariant"
  • e607287: 8282728: Serial: Remove unused BlockOffsetArray::Action
  • 8b45dbd: 8282312: Minor corrections to evbroadcasti32x4 intrinsic on x86
  • 3f0684d: 8275775: Add jcmd VM.classes to print details of all classes
  • cde923d: 8282690: runtime/CommandLine/VMDeprecatedOptions.java fails after JDK-8281181
  • ... and 626 more: https://git.openjdk.java.net/jdk/compare/9f30ec174faae10484766308996cab136a779658...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Mar 8, 2022
@openjdk openjdk bot closed this Mar 8, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Mar 8, 2022
@openjdk
Copy link

openjdk bot commented Mar 8, 2022

@dfuch @djelinski Pushed as commit 2549e55.

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

@djelinski djelinski deleted the interface-cleanup branch May 10, 2022
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 net net-dev@openjdk.org
3 participants