Skip to content

8339414: Fix JDK-8202369 incorrect backport for 8u #661

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fitzsim
Copy link
Contributor

@fitzsim fitzsim commented Jun 4, 2025

There is a large #ifdef in JDK 8's Inet4AddressImpl.c:

#if defined(__GLIBC__) || (defined(__FreeBSD__) && (__FreeBSD_version >= 601104))
#define HAS_GLIBC_GETHOSTBY_R   1
#endif

#if defined(_ALLBSD_SOURCE) && !defined(HAS_GLIBC_GETHOSTBY_R)
[... 261 lines ...]
#else /* defined(_ALLBSD_SOURCE) && !defined(HAS_GLIBC_GETHOSTBY_R) */
[... ~ 244 similar lines ...]
#endif /* _ALLBSD_SOURCE */

When the fix for https://bugs.openjdk.org/browse/JDK-8201369 was backported to 8, the second branch of the large #ifdef in Inet4AddressImpl.c was not changed at the same time, which is incorrect, since reverse lookup should only ever happen on Solaris, according to the description of JDK-8201369.

I used the JDK 11 unix/native/libnet/Inet4AddressImpl.c implementation; after this change, the only difference between 11's Java_java_net_Inet4AddressImpl_getLocalHostName and the defined(HAS_GLIBC_GETHOSTBY_R) one will be the use of JVM_GetHostName in 8 versus gethostname in 11.

I decided to update the hostlen argument to getnameinfo from NI_MAXHOST to sizeof(hostname) to fix https://bugs.openjdk.org/browse/JDK-8202181 for this function. I do not have a Solaris machine to test on, but this matches 11's getnameinfo calls, and is correct given hostname[NI_MAXHOST] = '\0'.

The resulting behaviour should be the same, other than the reverse lookup only being performed on Solaris.

The implementations of Java_java_net_Inet4AddressImpl_getLocalHostName in each of the #ifdef branches diverge already because the fix for https://bugs.openjdk.org/browse/JDK-7112670 (a62dd62) was only applied to the second branch.

Probably the upper Java_java_net_Inet4AddressImpl_getLocalHostName implementation could be made identical to the lower, but I do not want to do that, just in case NI_MAXHOST is preferred to sizeof(hostname) on !defined(HAS_GLIBC_GETHOSTBY_R) operating systems.


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
  • JDK-8339414 needs maintainer approval

Issue

  • JDK-8339414: Fix JDK-8202369 incorrect backport for 8u (Task - P4 - Requested)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev.git pull/661/head:pull/661
$ git checkout pull/661

Update a local copy of the PR:
$ git checkout pull/661
$ git pull https://git.openjdk.org/jdk8u-dev.git pull/661/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 661

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/661.diff

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 4, 2025

👋 Welcome back fitzsim! 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
Copy link

openjdk bot commented Jun 4, 2025

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@openjdk openjdk bot changed the title 8339414: Fix JDK-8202369 incorrect backport 8339414: Fix JDK-8202369 incorrect backport for 8u Jun 5, 2025
@fitzsim fitzsim force-pushed the fix-jdk-8339414 branch from 7400935 to 633bdb6 Compare June 5, 2025 14:01
@fitzsim fitzsim force-pushed the fix-jdk-8339414 branch from 633bdb6 to 8d8bef8 Compare June 5, 2025 14:18
@fitzsim fitzsim marked this pull request as ready for review June 5, 2025 15:02
@openjdk openjdk bot added the rfr Pull request is ready for review label Jun 5, 2025
@mlbridge
Copy link

mlbridge bot commented Jun 5, 2025

Webrevs

Copy link
Member

@gnu-andrew gnu-andrew left a comment

Choose a reason for hiding this comment

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

Yes, this seems like a clear case of the duplication of getLocalHostName(JNIEnv *env, jobject this) being missed in backporting from 11u (which has a much simpler addition of the #ifdef and the #elsecase). There's a lot of noise in removing the stored error value both in the original 8u backport and this change, but the overall change amounts to the same as 11u.

To that end, I don't think a partial backport of JDK-8202181 belongs here. Even if we agree not to alter the branch without HAS_GLIBC_GETHOSTBY_R (which I think has merit as we don't define NI_MAXHOST ourselves there), we should fix getHostByAddr and the IPv6 implementation. So I'd rather see that handled in full as a separate PR, rather than deviating from the 8202369 backport here.

@fitzsim fitzsim requested a review from gnu-andrew June 6, 2025 00:08
Copy link
Member

@gnu-andrew gnu-andrew left a comment

Choose a reason for hiding this comment

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

Thanks. Happy for this to go in now it's just the backport.

Please apply for approval

@openjdk
Copy link

openjdk bot commented Jun 6, 2025

⚠️ @fitzsim This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

@fitzsim
Copy link
Contributor Author

fitzsim commented Jun 6, 2025

/approval request This fix is ready.

@openjdk
Copy link

openjdk bot commented Jun 6, 2025

@fitzsim
8339414: The approval request has been created successfully.

@openjdk openjdk bot added the approval label Jun 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval rfr Pull request is ready for review
Development

Successfully merging this pull request may close these issues.

2 participants