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

8278185: Custom JRE cannot find non-ASCII named module inside #6693

Closed
wants to merge 4 commits into from

Conversation

toshiona
Copy link
Contributor

@toshiona toshiona commented Dec 3, 2021

Could you review this fix?

Problem:
Custom JRE generated by jlink cannot find non-ASCII named modules included inside the JRE.

Cause and fix:
If module or package name was composed by ASCII then non-ASCII characters,
ImageStringsReader:stringFromByteBufferMatches() miscalculated the length of matched string. The first part of ASCII characters was missing. This patch corrected the value.

Testing:
tier1 and tier2 on Linux have no regression.
I wasn't able to create an automate test for this issue. I appreciate any advice.


Progress

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

Issue

  • JDK-8278185: Custom JRE cannot find non-ASCII named module inside

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6693

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 3, 2021

👋 Welcome back tnakamura! 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 openjdk bot commented Dec 3, 2021

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

  • core-libs

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 core-libs label Dec 3, 2021
@toshiona toshiona marked this pull request as ready for review Dec 3, 2021
@openjdk openjdk bot added the rfr label Dec 3, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 3, 2021

Webrevs

Copy link
Member

@JimLaskey JimLaskey left a comment

Change looks reasonable. A regression test should be provided. Contact me directly for advice.

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Dec 6, 2021

Yes, I think we would be good to have a test. If it's too hard using the tools then a test that uses the image reader API directly would be okay. @naotoj may have some suggestions too.

@naotoj
Copy link
Member

@naotoj naotoj commented Dec 6, 2021

Regression tests for ResourceBundle class have tests that dynamically generate modules. Although I have not used non-ASCII module names, tests under test/jdk/java/util/ResourceBundle/modules may shed some light on the subject.

@toshiona
Copy link
Contributor Author

@toshiona toshiona commented Dec 7, 2021

Thank you so much for the valuable advices, Jim, Alan, and Naoto.

The created test calls ImageStringsReader.stringFromByteBufferMatches() directly.
It's a package private method, and I tried to use library style.

The upper public method, BasicImageReader.match(), is hard to prepare data.
Calling jlink is also complex for me. So, I choose this way.

@naotoj
Copy link
Member

@naotoj naotoj commented Dec 7, 2021

There is a jlink plugin test case that dynamically creates custom jimages with locale data variations. That might be of your interest: test/jdk/tools/jlink/plugins/IncludeLocalesPluginTest.java.

@toshiona
Copy link
Contributor Author

@toshiona toshiona commented Dec 8, 2021

Thank you again for valuable information, Sato-san.
I've replaced the testcase with the one using helper classes.

One tricky part is using InMemory methods, because non-ASCII name file may not be created.
Non-ASCII name file was packed in a jar file directly.

naotoj
naotoj approved these changes Dec 8, 2021
Copy link
Member

@naotoj naotoj left a comment

Thanks for updating the test. Looks good to me.
I might be tempted to change the test case name, as Strings is too generic. Maybe to JImageNonAsciiNameTest.java or similar.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 8, 2021

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

8278185: Custom JRE cannot find non-ASCII named module inside

Reviewed-by: naoto

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

  • 83e6a4c: 8255409: Support the new C_GetInterfaceList, C_GetInterface, and C_SessionCancel APIs in PKCS#11 v3.0
  • 3cec700: 8278463: [test] Serialization WritePrimitive test revised for readFully test fails
  • 03bf55f: 8277980: ObjectMethods::bootstrap throws NPE when lookup is null
  • 3893f4f: 8278330: dump stack trace if the jvmti test nsk/jvmti/GetThreadState/thrstat002 is failed with wrong thread state
  • b3faecf: 8276116: C2: optimize long range checks in int counted loops
  • fe2ae8e: 8276904: Optional.toString() is unnecessarily expensive
  • 6e7b7f3: 8278251: Enable "missing-explicit-ctor" check in the jdk.unsupported.desktop module
  • c83b781: 8278459: ProblemList javax/swing/JTree/4908142/bug4908142.java on macosx-aarch64
  • 8e8fadf: 8278428: ObjectInputStream.readFully range check incorrect
  • 5a80abf: 8272944: Use snippets in jdk.javadoc documentation
  • ... and 102 more: https://git.openjdk.java.net/jdk/compare/473ececaf5df8392e76d022d7126770526be31f7...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 (@JimLaskey, @naotoj) 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 label Dec 8, 2021
@toshiona
Copy link
Contributor Author

@toshiona toshiona commented Dec 8, 2021

/integrate

@openjdk openjdk bot added the sponsor label Dec 8, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Dec 8, 2021

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

@toshiona
Copy link
Contributor Author

@toshiona toshiona commented Dec 8, 2021

@naotoj
Thank you very much for the review.
Could you sponsor the change please?

@naotoj
Copy link
Member

@naotoj naotoj commented Dec 9, 2021

Sure, I can. But I would like @JimLaskey to have a final look at it.

@JimLaskey
Copy link
Member

@JimLaskey JimLaskey commented Dec 9, 2021

Sure, I can. But I would like @JimLaskey to have a final look at it.

Standing by

@naotoj
Copy link
Member

@naotoj naotoj commented Dec 9, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Dec 9, 2021

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

  • abebde7: 8278421: G1: Remove unused HeapRegion::verify
  • bc31ccc: 8278312: Update SimpleSSLContext keystore to use SANs for localhost IP addresses
  • 01b30bf: 8253860: PPC: Relocation::pd_set_data_value conflates compressed oops and klasses
  • 09831e7: 8273146: Start of release updates for JDK 19
  • 6dae52f: 8276985: AArch64: [vectorapi] Backend support of VectorMaskToLongNode
  • 08aad85: 8211230: JFR: internal events
  • 965ea8d: 8274298: JFR Thread Sampler thread must not acquire malloc lock after suspending a thread because of possible deadlock
  • 84baea7: 8278433: Use snippets in jdk.incubator.foreign documentation
  • 1ea766d: 8278419: JFR jcmd option contract "This value cannot be changed once JFR has been initialized" is not enforced
  • 62a7f5d: 8254318: Remove .hgtags
  • ... and 115 more: https://git.openjdk.java.net/jdk/compare/473ececaf5df8392e76d022d7126770526be31f7...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Dec 9, 2021
@openjdk openjdk bot added integrated and removed ready labels Dec 9, 2021
@openjdk openjdk bot removed rfr sponsor labels Dec 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Dec 9, 2021

@naotoj @toshiona Pushed as commit fcd67a5.

💡 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
core-libs integrated
4 participants