8275319 java.net.NetworkInterface throws java.lang.Error instead of SocketException #5956

djelinski · 2021-10-14T17:29:16Z

Per Java documentation, "Error [..] indicates serious problems that a reasonable application should not try to catch". Failure to enumerate network interfaces or addresses is not a serious enough situation; many applications can recover from this pretty easily.

All native methods (except init()) in NetworkInterface are declared with throws SocketException, so throwing SocketExceptions instead of Errors will match the declared interface.

Unix version of NetworkInterface already throws SocketExceptions in similar situations, and does not throw Error under any circumstances.

I searched the bug database and mail archives, but could not find any discussion on the topic; if there's a reason to keep throwing Errors, I couldn't find it.

tier1 tests pass, java.net tests pass. No new regression tests as I couldn't find any list of steps to reproduce the problem.

I don't have write access to JBS. I could use some help with creating a ticket.

Progress

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

Issue

JDK-8275319: java.net.NetworkInterface throws java.lang.Error instead of SocketException

Reviewers

Alan Bateman (@AlanBateman - Reviewer)
Daniel Fuchs (@dfuch - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5956

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

Using diff file

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

bridgekeeper · 2021-10-14T17:29:51Z

👋 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 · 2021-10-14T17:32:20Z

@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.

jaikiran · 2021-10-15T03:30:49Z

Hello @djelinski,

I don't have write access to JBS. I could use some help with creating a ticket.

I have created https://bugs.openjdk.java.net/browse/JDK-8275319 to track this. For this PR to trigger the PR review process, you will now have to update the PR title to match the bug id and bug title (and I think even the commit message should refer to the same).

mlbridge · 2021-10-15T05:08:58Z

Webrevs

AlanBateman · 2021-10-15T06:31:08Z

I suspect most of these cases need a closer look as there may be cases that can never happen, meaning Error or InternalError would be more appropriate, e.g. if MultiByteToWideChar fails with ERROR_INVALID_FLAGS or ERROR_INVALID_PARAMETER for example.

dfuch · 2021-10-15T08:53:43Z

I have to agree with Alan on this. Is there any concrete case where an Error was observed, or is this pure speculation?
If there are concrete cases then the proper fix would probably be to examine the error code or condition that triggered the Error - and investigate if the native source code can be updated to take remediation actions in those cases.

djelinski · 2021-10-15T10:01:19Z

@jaikiran thanks for the JBS. Editing PR title was sufficient.

Is there any concrete case where an Error was observed?

It happens occasionally, as evidenced by JDK-8217298, JDK-8046500 (fixed), JDK-8165665, JDK-8066931, JDK-8057900, JDK-8065559(closed), JDK-8040229, JDK-8065078(fixed), JDK-8068597(fixed), numerous reports on Google (search for "error IP Helper Library"), and an ancient note found in my company's codebase, possibly no longer relevant.

if MultiByteToWideChar fails

I started checking how such failures are handled elsewhere in JDK codebase, and the results are pretty inconsistent: ignore result, normal error handling, native assert, and Java error in NetworkInterface. Some consistency here would be nice, but then, I don't know which option I would pick. Google couldn't find any cases of "Error Cannot get multibyte", so I guess that doesn't happen too often.

Error or InternalError

There are only 2 places in Java codebase where Error is thrown, one is NetworkInterface, the other one is here. JNU_ThrowInternalError is definitely the more popular option.

Still, I'd argue that even if an unhandled error in IP helper library indicates a bug in JDK, throwing a checked exception is a better option here, because:

these errors do not indicate that the JVM is broken / unusable, at least not any more than it was before the function was called. They do not affect global JVM state in any way
a checked exception is less likely to go unnoticed (uncaught Error will usually terminate one thread, and let other threads continue; checked exception needs to be handled by user code)

dfuch · 2021-10-15T11:22:23Z

It happens occasionally, as evidenced by JDK-8217298, JDK-8046500 (fixed), JDK-8165665, JDK-8066931, JDK-8057900, JDK-8065559(closed), JDK-8040229, JDK-8065078(fixed), JDK-8068597(fixed), numerous reports on Google (search for "error IP Helper Library"), and an ancient note found in my company's codebase, possibly no longer relevant.

So these Errors do actually happen - thanks for the confirmation, and double thanks for the archeology research. This is very useful to this discussion.

I see that some of the issue you mentioned are suggesting some fixes - like - for instance JDK-8165665.
It may be that we need to examine each of these on a case-by-case basis. It seems that GetAdaptersAddresses, for instance, has more error codes than those we specifically handle - and maybe we need more specific error handling for some of these.
Maybe throwing SocketException, as you suggest, is appropriate for some of these cases.
Maybe just skipping the interface would be the right choice for others?

If some conditions are not supposed to happen - then I still think that throwing InternalError is more appropriate.
I would be afraid that blindly changing all Error into SocketException would be hiding issues under the carpet, as they would appear as a regular failure...

AlanBateman · 2021-10-15T11:40:18Z

It happens occasionally, as evidenced by JDK-8217298, JDK-8046500 (fixed), JDK-8165665, JDK-8066931, JDK-8057900, JDK-8065559(closed), JDK-8040229, JDK-8065078(fixed), JDK-8068597(fixed), numerous reports on Google (search for "error IP Helper Library"), and an ancient note found in my company's codebase, possibly no longer relevant.

There was churn in the Windows APIs and some of these issues may be historical. I think it would be useful if we can find any recent reports of issues and try to identify if they are issues in specific environments/configurations, JDK issues (not handling some cases), or something else. I'd prefer not rush in and change all these exceptions without going through them one by one.

djelinski · 2021-10-15T16:13:51Z

we need to examine each of these on a case-by-case basis

Makes sense.

MultiByteToWideChar case: we can change this to InternalError, as it's not supposed to happen and apparently not happening
GetIfTable case:

as JDK-8165665 states, our handling of ERROR_INSUFFICIENT_BUFFER leaves something to be desired. We could keep allocating larger buffers a few more times, similar to what we have here.
ERROR_INVALID_PARAMETER qualifies as InternalError to me,
ERROR_NOT_SUPPORTED is either a SocketException or an empty list.
I'm a bit concerned about the "Other" catch-all qualifier on MSDN page, but I'd use SocketException for these, possibly with error code / name embedded in exception message. Since they are not explicitly enumerated by Microsoft, I don't see how they could qualify as Java bugs.

GetIpAddrTable case: documentation specifies exactly the same list of errors as above, so we could handle them in the same way
GetAdaptersAddresses case (winxp only - is the file really used only on WinXP?):

ERROR_ADDRESS_NOT_ASSOCIATED: either return no data or throw SocketException to make it different from ERROR_NO_DATA
ERROR_BUFFER_OVERFLOW: our handling looks fine here
ERROR_INVALID_PARAMETER: InternalError
ERROR_NOT_ENOUGH_MEMORY: OOME?
ERROR_NO_DATA: return no data
other: JDK-8217298 reports ERROR_ACCESS_DENIED (5), JDK-8231811 reports ERROR_NOT_FOUND (1168); I'd use SocketException for error codes in this category.

What do you think?

AlanBateman · 2021-10-16T08:03:27Z

JDK-8217298 hints that one scenario where an exception is thrown is when there are no network adapters configured. I don't know how easy it would be to create that scenario and to see how current JDK releases behave.

MultiByteToWideChar - yes, makes sense.

GetIfTable -yes, I agree this needs to cleaned up. I don't see ERROR_BUFFER_OVERFLOW listed as a possible error.

GetAdaptersAddresses. It would be nice if this NetworkInterface_winXP.c could be removed but it is still used for the -Djava.net.preferIPv4Stack=true case. A more useful exercise may be to see if we could move to one implementation that works with preferIPv4Stack set to true or false.

djelinski · 2021-10-18T16:15:10Z

Updated the PR with more specific exception types. I'm still doing some printf-based testing / debugging. Observations so far:

My assumption that WinXP file contained some ancient code was incorrect. The WinXP functions are used when NOT in java.net.preferIPv4Stack mode, and are the preferred / more modern way. Cost me a few sanity points when I first realized.
GetAdaptersAddresses failures are generating correct exception types, but error message generated by JNU_ThrowByNameWithMessageAndLastError is not what I expected. Example:

java.net.SocketException: An address has not yet been associated with the network endpoint (IP Helper Library GetAdaptersAddresses function failed)

I expected the Windows error code to appear after my message, not before.

GetIpAddrTable failures are working correctly in java.net.preferIPv4Stack mode; in default mode they crash the JVM (access violation in net.dll). Apparently some of the callers attempt to access result pointer even when lookupIPAddrTable fails.

I'll update when I know more.

FWIW, my test code:

		try {
			Collections.list(NetworkInterface.getNetworkInterfaces());
			System.out.println("getIfs succeeded");
		} catch (Throwable t) {
			t.printStackTrace();
		}

djelinski · 2021-10-18T16:59:40Z

The crash reason was a bit more mundane. We assigned the (int) result of lookupIPAddrTable to a (DWORD / unsigned int) variable, then checked if the variable value is negative (which it cannot be).
Wonder why the compiler didn't warn about this.

djelinski · 2021-10-18T19:07:23Z

GetIfTable failures are handled somewhat correctly; in IPv4 mode the exception is propagated as expected.

In default mode the exception is swallowed here, and then SocketException ("No network interfaces configured") is thrown in Java code.
The code that swallows exception appears incorrect; when exception is swallowed, netifPP is null, last is null and the code that could add an IPv6 interface would crash.

Should I clean that up here or leave it for future PRs?

AlanBateman · 2021-10-19T06:42:43Z

src/java.base/windows/native/libnet/NetworkInterface_winXP.c

+            case ERROR_NO_DATA:
+                // not an error
+                *adapters = NULL;
+                return ERROR_SUCCESS;


Mapping ERROR_NO_DATA to ERROR_SUCCESS is probably correct here. Could this explain bug reports that seem to be from configurations with no IP addresses plumbed?

I suppose it could. But in order to get ERROR_NO_DATA here one would need to disable loopback interface. I couldn't find how to do that.

There has been one or two reports from configurations that appear to have no interface or no IP addresses. I don't think we've been able to create those configurations to duplicate. In any case, treat NO_DATA as SUCCESS seems correct here.

AlanBateman · 2021-10-19T06:45:58Z

src/java.base/windows/native/libnet/NetworkInterface_winXP.c

@@ -237,7 +229,7 @@ static int ipinflen = 2048;
 */
 int getAllInterfacesAndAddresses (JNIEnv *env, netif **netifPP)
 {
-    DWORD ret, flags;
+    int ret, flags;


This looks right but I can't relate this to your comment about the crash.

let me try to explain this clang-style:

tableP contains random garbage

lookupIPAddrTable returns error, does not touch tableP

ret is unsigned, so this is never true

uninitialized tableP is passed to enumAddresses_win_ipaddrtable

enumAddresses_win_ipaddrtable dereferences tableP and crashes

Thanks for this, I hadn't spotted the broken code that tests if loopupIPAddrTable fails.

AlanBateman

I think you've got this to a good place. It would be helpful if you could trim down some of the really long lines to keep to consistent with the existing code if possible. It's clear that there is a lot of cleanup needed in this area but that can be done in future PRs as needed.

dfuch · 2021-10-19T10:43:28Z

Hi Daniel, thanks for persisting with this. I had a look at your changes and I believe you have hit the right balance. I also like the fact plain java.lang.Error has been transformed into java.lang.InternalError. Also thanks for sanitizing the error code checks to match what GetIfTable and GetAdaptersAddresses are documented to return. I believe we are ending in a much better place. Please give me some time to import your patch and give it a round of testing.

dfuch · 2021-10-20T11:21:35Z

Hi - I have run your current patch through our CI and the good news is that we got no failure.
However - we should probably go the extra mile and fix the case where -2 is returned.

In default mode the exception is swallowed here, and then SocketException ("No network interfaces configured") is thrown in Java code.
The code that swallows exception appears incorrect; when exception is swallowed, netifPP is null, last is null and the code that could add an IPv6 interface would crash.

I believe you are correct and there are still some issues in that case. In particular I also see -that if netaddrCount == -2 there:

jdk/src/java.base/windows/native/libnet/NetworkInterface_winXP.c

Line 573 in a6c89e2

if (netaddrCount == -2) {

we're going to try and allocate arrays of size -2 a few lines below. That doesn't sound right.

dfuch · 2021-10-20T12:12:21Z

Hmm... JDK-8225239 might have introduced a bug.
I see that with the current code neither enumAddresses_win nor enumAddresses_win_ipaddrtable will return -2 - so there appears to be some dead code. I suspect that this line here is wrong:

jdk/src/java.base/windows/native/libnet/NetworkInterface.c

Line 516 in a6c89e2

return NULL;

it should be returning ret not NULL

AlanBateman

The current patch looks good and it's good to have caught the DWORD->int issue. We should add information about this to the JBS issue.
I don't mind if you want to iterate further with Daniel's suggestion, okay if it's a separate PR too. In general I think there is quite a bit of cleanup to do in this code.

openjdk · 2021-10-20T12:59:03Z

⚠️ @djelinski the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout networkinterface-socketexception
$ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name'
$ git push

openjdk · 2021-10-20T12:59:06Z

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

8275319: java.net.NetworkInterface throws java.lang.Error instead of SocketException

Reviewed-by: alanb, 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 108 new commits pushed to the master branch:

a91a0a5: 8233724: Remove -Wc++14-compat warning suppression in operator_new.cpp
1271fbf: 8248584: Enable CHECK_UNHANDLED_OOPS for Windows fastdebug builds
135cf3c: 8275439: Remove PrintVtableStats
50a5723: 8274910: Compile in G1 evacuation failure injection code based on define
5454a76: 8275273: Add missing HtmlStyle documentation
bd0bed7: 8273317: crash in cmovP_cmpP_zero_zeroNode::bottom_type()
77b2789: 7124287: [macosx] JTableHeader doesn't get focus after pressing F8 key
c24fb85: 8275512: Upgrade required version of jtreg to 6.1
926966b: 8275003: Suppress warnings on non-serializable non-transient instance fields in windows mscapi
e63c148: 8264849: Add KW and KWP support to PKCS11 provider
... and 98 more: https://git.openjdk.java.net/jdk/compare/8657f77608f37d7ff5254032858f2f16c7c204d5...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 (@AlanBateman, @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).

djelinski · 2021-10-20T13:38:17Z

Thanks @dfuch and @AlanBateman for the review and all your help.

@dfuch I'd rather follow up on the remaining issues in a separate PR. This one is already doing more than the PR title advertises.

djelinski · 2021-10-20T13:38:26Z

/integrate

openjdk · 2021-10-20T13:39:15Z

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

dfuch · 2021-10-20T13:53:21Z

@djelinski I have no objection in handling that in a followup PR.
I have logged https://bugs.openjdk.java.net/browse/JDK-8275640.
Would you be willing to continue with that?

dfuch · 2021-10-20T14:04:53Z

/sponsor

openjdk · 2021-10-20T14:06:19Z

Going to push as commit 043cde2.
Since your change was applied there have been 108 commits pushed to the master branch:

a91a0a5: 8233724: Remove -Wc++14-compat warning suppression in operator_new.cpp
1271fbf: 8248584: Enable CHECK_UNHANDLED_OOPS for Windows fastdebug builds
135cf3c: 8275439: Remove PrintVtableStats
50a5723: 8274910: Compile in G1 evacuation failure injection code based on define
5454a76: 8275273: Add missing HtmlStyle documentation
bd0bed7: 8273317: crash in cmovP_cmpP_zero_zeroNode::bottom_type()
77b2789: 7124287: [macosx] JTableHeader doesn't get focus after pressing F8 key
c24fb85: 8275512: Upgrade required version of jtreg to 6.1
926966b: 8275003: Suppress warnings on non-serializable non-transient instance fields in windows mscapi
e63c148: 8264849: Add KW and KWP support to PKCS11 provider
... and 98 more: https://git.openjdk.java.net/jdk/compare/8657f77608f37d7ff5254032858f2f16c7c204d5...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-10-20T14:06:37Z

@dfuch @djelinski Pushed as commit 043cde2.

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

djelinski · 2021-10-20T14:10:32Z

@dfuch yessir. I'll continue the cleanup, should have something for review later this week.

Throw SocketExceptions instead of Errors in NetworkInterface

2322f15

openjdk bot added the net label Oct 14, 2021

djelinski marked this pull request as ready for review October 14, 2021 18:00

djelinski changed the title ~~Throw SocketExceptions instead of Errors in NetworkInterface~~ 8275319 java.net.NetworkInterface throws java.lang.Error instead of SocketException Oct 15, 2021

openjdk bot added the rfr label Oct 15, 2021

Throw more specific exceptions

79c74b8

Fix crash on GetIpAddrTable error

a6c89e2

AlanBateman reviewed Oct 19, 2021

View reviewed changes

Adapt line length to 80 column display

9e9ea0a

AlanBateman approved these changes Oct 20, 2021

View reviewed changes

openjdk bot added the ready label Oct 20, 2021

openjdk bot added the sponsor label Oct 20, 2021

dfuch approved these changes Oct 20, 2021

View reviewed changes

openjdk bot closed this Oct 20, 2021

openjdk bot added integrated and removed ready rfr sponsor labels Oct 20, 2021

djelinski deleted the networkinterface-socketexception branch January 12, 2022 14:49

8275319 java.net.NetworkInterface throws java.lang.Error instead of SocketException #5956

8275319 java.net.NetworkInterface throws java.lang.Error instead of SocketException #5956

Conversation

djelinski commented Oct 14, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Oct 14, 2021

openjdk bot commented Oct 14, 2021

jaikiran commented Oct 15, 2021

mlbridge bot commented Oct 15, 2021 • edited Loading

Webrevs

AlanBateman commented Oct 15, 2021

dfuch commented Oct 15, 2021

djelinski commented Oct 15, 2021

dfuch commented Oct 15, 2021

AlanBateman commented Oct 15, 2021

djelinski commented Oct 15, 2021

AlanBateman commented Oct 16, 2021

djelinski commented Oct 18, 2021 • edited Loading

djelinski commented Oct 18, 2021

djelinski commented Oct 18, 2021

AlanBateman Oct 19, 2021

Choose a reason for hiding this comment

djelinski Oct 19, 2021

Choose a reason for hiding this comment

AlanBateman Oct 19, 2021

Choose a reason for hiding this comment

AlanBateman Oct 19, 2021

Choose a reason for hiding this comment

djelinski Oct 19, 2021

Choose a reason for hiding this comment

AlanBateman Oct 19, 2021

Choose a reason for hiding this comment

AlanBateman left a comment

Choose a reason for hiding this comment

dfuch commented Oct 19, 2021

dfuch commented Oct 20, 2021 • edited Loading

dfuch commented Oct 20, 2021

AlanBateman left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 20, 2021

openjdk bot commented Oct 20, 2021 • edited Loading

djelinski commented Oct 20, 2021

djelinski commented Oct 20, 2021

openjdk bot commented Oct 20, 2021

dfuch commented Oct 20, 2021

dfuch commented Oct 20, 2021

openjdk bot commented Oct 20, 2021

openjdk bot commented Oct 20, 2021

djelinski commented Oct 20, 2021

djelinski commented Oct 14, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 15, 2021 •

edited

Loading

djelinski commented Oct 18, 2021 •

edited

Loading

dfuch commented Oct 20, 2021 •

edited

Loading

openjdk bot commented Oct 20, 2021 •

edited

Loading