8372155: ReleaseInfoPlugin doesn't handle input file as UTF-8 properly

[jerboaa]

Please review this simple fix to jlink's --release-info plugin to handle non-ASCII in vendor strings. The JDK build uses UTF-8 encoding for the produced release file that is being passed to jlink at build-time via the --release-info plugin. However, the plugin internally uses java.util.Properties.load(InputStream) API which assumes ISO-8859-1 encoding of the input stream. The proposed fix is to use the java.util.Prorperties.load(Reader) API instead and pass it an InputStreamReader with UTF-8 encoding.

Testing:

• GHA
• test/jdk/tools/jlink tests including the new reg-test which fails prior and passes after the fix.

Thoughts?

---

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-8372155: ReleaseInfoPlugin doesn't handle input file as UTF-8 properly (Bug - P3)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28399

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back sgehwolf! 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]

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

[openjdk]

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

[mlbridge]

Webrevs

• 03: Full - Incremental (65e57118)
• 02: Full - Incremental (580b4b13)
• 01: Full - Incremental (2f48c81b)
• 00: Full (2059e527)

[jaikiran]

Hello Severin, would it better to (even) specify the expectations of the --release-info jlink plugin and what encoding it expects for the file content?

I might be wrong, but even with the use of UTF-8 for reading the given file (like proposed in this PR), it may not guarantee that it is the right Charset to use for that file.

[jerboaa]

Thanks for the review.

Hello Severin, would it better to (even) specify the expectations of the --release-info jlink plugin and what encoding it expects for the file content?

IIUYC, you'd like to have the help text amended for --release-info to say it expects the input file in UTF-8? Is that what you are saying?

IMO it would be best for --release-info to not only require a file as an option but also the encoding. It could perhaps default to UTF-8. @AlanBateman Any thoughts on this?

I might be wrong, but even with the use of UTF-8 for reading the given file (like proposed in this PR), it may not guarantee that it is the right Charset to use for that file.

True. If somebody passes an ISO-8859-1 (or any non-utf8) encoded file to --release-info it would break for any non-ASCII characters.

[jaikiran]

Hello Severin, would it better to (even) specify the expectations of the --release-info jlink plugin and what encoding it expects for the file content?

IIUYC, you'd like to have the help text amended for --release-info to say it expects the input file in UTF-8? Is that what you are saying?

Correct, but not necessarily UTF-8. I am not familiar where all this release-info gets used during jlink and then in the generated image. So whatever encoding is appropriate for such content, I think we should specify it, in the help text and man page of jlink.

[jerboaa]

Correct, but not necessarily UTF-8.

It expects the input file to be UTF-8 now. So I've added that the plugin help text in 2f48c81

I am not familiar where all this release-info gets used during jlink and then in the generated image.

It's the plugin that creates the $JDK/release file. That's it.

So whatever encoding is appropriate for such content, I think we should specify it, in the help text and man page of jlink.

I've added it to the --list-plugin help text now. Plugins aren't mentioned in the jlink man page, so it doesn't seem appropriate to mention it the jlink man page. Here is how it looks like when using jlink --list-plugins with the latest update:

  --release-info <file>|add:<key1>=<value1>:<key2>=<value2>:...|del:<key list>
                            <file> option is to load release properties from
                            the supplied file. The specified file is expected
                            to be encoded in UTF-8.
                            add: is to add properties to the release file.
                            Any number of <key>=<value> pairs can be passed.
                            del: is to delete the list of keys in release file.

[AlanBateman]

IMO it would be best for --release-info to not only require a file as an option but also the encoding. It could perhaps default to UTF-8. @AlanBateman Any thoughts on this?

It's very likely that tools that read the release file aren't using Properties API so I think having it UTF-8 is best.

[jerboaa]

IMO it would be best for --release-info to not only require a file as an option but also the encoding. It could perhaps default to UTF-8. @AlanBateman Any thoughts on this?

It's very likely that tools that read the release file aren't using Properties API so I think having it UTF-8 is best.

The output $JDK/release file will be UTF-8 since the ReleaseInfoPlugin uses PrintWriter(BytearrayOutputStream) to write the content bytes to the ResourcePoolEntry. That in-turn uses UTF-8 since JEP 400 (by means of Charset.defaultCharset().

Question is if it's worth supporting arbitrary input encondings for --release-info <file> usages. <file> is currently in UTF-8 in the JDK build, so went with that expectation. But arguably it could be any other encoding a user chooses. So to make it generic, it's conceivable to allow --release-info=<file>,<encoding> or some such to allow non-UTF-8 as input and still do the right thing (Note: output would still be UTF-8). Perhaps it's not worth the trouble?

[AlanBateman]

Question is if it's worth supporting arbitrary input encondings for --release-info <file> usages. <file> is currently in UTF-8 in the JDK build, so went with that expectation. But arguably it could be any other encoding a user chooses. So to make it generic, it's conceivable to allow --release-info=<file>,<encoding> or some such to allow non-UTF-8 as input and still do the right thing (Note: output would still be UTF-8). Perhaps it's not worth the trouble?

That seems unnecessary complexity, I think we should just keep it as UTF-8.

[RogerRiggs]

Please fix the typo in the PR title. (I corrected the issue title).