JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame #7663

tprinzing · 2022-03-02T18:56:40Z

The caller class returned by Reflection::getCallerClass was used to gain access to it's module in most cases and class loader in one case. I added a method to translate the caller class to caller module so that the decision of what module represents the caller with no stack frame is made in a single place. Calls made to caller.getModule() were replaced with getCallerModule(caller) which returns the system class loader unnamed module if the caller is null.

The one place a class loader was produced from the caller in getBundleImpl it was rewritten to route through the getCallerModule method:

    final ClassLoader loader = (caller != null) ?
            caller.getClassLoader() : getLoader(getCallerModule(caller));

A JNI test was added which calls getBundle to fetch a test bundle from a location added to the classpath, fetches a string out of the bundle and verifies it, and calls clearCache.

The javadoc was updated for the caller sensitive methods changed.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed
Change requires a CSR request to be approved

Issues

JDK-8280902: ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame
JDK-8282575: ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame (CSR)

Reviewers

Naoto Sato (@naotoj - Reviewer) ⚠️ Review applies to 3077806
Mandy Chung (@mlchung - Reviewer) ⚠️ Review applies to 45f9b37
Magnus Ihse Bursie (@magicus - Reviewer) ⚠️ Review applies to 45f9b37

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7663

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

Using diff file

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

…NI code with no caller frame

boot class loader.

bridgekeeper · 2022-03-02T19:09:46Z

👋 Welcome back tprinzing! 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 · 2022-03-02T19:12:33Z

@tprinzing The following labels will be automatically applied to this pull request:

build
core-libs
i18n

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-03-02T19:16:24Z

Webrevs

naotoj

Looks good to me. I believe a CSR is needed for this change.

naotoj · 2022-03-02T19:52:15Z

/csr

openjdk · 2022-03-02T19:53:23Z

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

8280902: ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame

Reviewed-by: naoto, mchung, ihse

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

12693a6: 8282432: Optimize masked "test" Vector API with predicate feature
4924513: 8265263: AArch64: Combine vneg with right shift count
ea19114: 8282832: Update file path for HostnameMatcher/cert5.crt in test sun/security/util/Pem/encoding.sh

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 (@naotoj, @mlchung, @magicus) 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 · 2022-03-02T19:53:49Z

@naotoj has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@tprinzing please create a CSR request for issue JDK-8280902. This pull request cannot be integrated until the CSR request is approved.

mlchung

Instead of sprinkling the null caller text in the javadoc in each getBundle method, we can specify that that in the class specification, for example, after the "Resource bundles in other modules and class path" section.

In cases where the {@code getBundle} factory method is called from a context 
where there is no caller frame on the stack (e.g. when called directly from
a JNI attached thread), the caller module is default to the unnamed module for the
{@linkplain ClassLoader#getSystemClassLoader system class loader}.

What do you think?

src/java.base/share/classes/java/util/ResourceBundle.java

Co-authored-by: Mandy Chung <mandy.chung@oracle.com>

mlchung

Thanks for the update.

mlchung · 2022-03-02T22:16:52Z

src/java.base/share/classes/java/util/ResourceBundle.java

+     * stack frame the caller will be null, so the system class loader unnamed
+     * module will be used.
+     * @param caller
+     * @return


Maybe you intended to make this /* .... */ instead of javadoc? no need to have @param and @return for this simple method.

mlchung · 2022-03-02T22:17:51Z

src/java.base/share/classes/java/util/ResourceBundle.java

    @CallerSensitive
    public static final void clearCache() {
-        Class<?> caller = Reflection.getCallerClass();
+        final Module callerModule = getCallerModule(Reflection.getCallerClass());


nit: final can be dropped here.

mlchung · 2022-03-02T22:21:03Z

test/jdk/java/util/ResourceBundle/exeNullCallerResourceBundle/exeNullCallerResourceBundle.c

+#include <stdlib.h>
+
+#include "jni.h"
+#undef NDEBUG


is this line unintended?

Forcing the assert() seems like a good idea in a test

mlchung · 2022-03-02T22:22:29Z

test/jdk/java/util/ResourceBundle/exeNullCallerResourceBundle/NullCallerResourceBundle.java

+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Properties;
+import java.util.ResourceBundle;


nit: good to keep the imports in alphabetic order.

mlchung · 2022-03-02T22:36:16Z

test/jdk/java/util/ResourceBundle/exeNullCallerResourceBundle/NullCallerResourceBundle.java

+    }
+
+
+    private static void makePropertiesFile() throws IOException {


This can be removed?

mlchung · 2022-03-02T22:40:05Z

test/jdk/java/util/ResourceBundle/exeNullCallerResourceBundle/NullCallerResourceBundle.java

+        final String vmDir = Platform.jvmLibDir().toString();
+
+        // set up shared library path
+        final String sharedLibraryPathEnvName = Platform.sharedLibraryPathVariableName();


Nit: final adds noise but no other value to the test. Can consider dropping it.

mlchung · 2022-03-03T18:55:42Z

src/java.base/share/classes/java/util/ResourceBundle.java

+    private static Module getCallerModule(Class<?> caller) {
+        Module callerModule = (caller != null) ? caller.getModule()
+                : ClassLoader.getSystemClassLoader().getUnnamedModule();
+        return callerModule;


nit: callerModule variable is not needed. It can simply do:

return (caller != null) ? caller.getModule() : ClassLoader.getSystemClassLoader().getUnnamedModule();

magicus

Build changes look good. Can't say anything about the rest.

tprinzing · 2022-03-09T01:59:34Z

/integrate

openjdk · 2022-03-09T02:01:06Z

@tprinzing
Your change (at version 6600e1f) is now ready to be sponsored by a Committer.

mlchung · 2022-03-09T04:00:31Z

/sponsor

openjdk · 2022-03-09T04:02:25Z

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

12693a6: 8282432: Optimize masked "test" Vector API with predicate feature
4924513: 8265263: AArch64: Combine vneg with right shift count
ea19114: 8282832: Update file path for HostnameMatcher/cert5.crt in test sun/security/util/Pem/encoding.sh

Your commit was automatically rebased without conflicts.

openjdk · 2022-03-09T04:02:46Z

@mlchung @tprinzing Pushed as commit 31ad80a.

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

tprinzing added 2 commits March 1, 2022 16:16

JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by J…

957295d

…NI code with no caller frame

Use the unnamed module defined to the system class loader instead of the

3077806

boot class loader.

openjdk bot added the rfr Pull request is ready for review label Mar 2, 2022

openjdk bot added build build-dev@openjdk.org core-libs core-libs-dev@openjdk.org i18n i18n-dev@openjdk.org labels Mar 2, 2022

naotoj approved these changes Mar 2, 2022

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated csr Pull request needs approved CSR before integration labels Mar 2, 2022

openjdk bot removed the ready Pull request is ready to be integrated label Mar 2, 2022

mlchung reviewed Mar 2, 2022

View reviewed changes

src/java.base/share/classes/java/util/ResourceBundle.java Outdated Show resolved Hide resolved

tprinzing and others added 2 commits March 2, 2022 15:27

Update src/java.base/share/classes/java/util/ResourceBundle.java

9e8fb19

Co-authored-by: Mandy Chung <mandy.chung@oracle.com>

suggested changes

eeb2d0f

mlchung reviewed Mar 2, 2022

View reviewed changes

more suggested changes

45f9b37

mlchung approved these changes Mar 3, 2022

View reviewed changes

magicus approved these changes Mar 8, 2022

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Mar 8, 2022

tprinzing added 2 commits March 8, 2022 19:29

remove unnecessary variable

711d24b

Merge branch 'master' into JDK-8280902

6600e1f

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 9, 2022

openjdk bot added the integrated Pull request has been integrated label Mar 9, 2022

openjdk bot closed this Mar 9, 2022

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 9, 2022

		}


		private static void makePropertiesFile() throws IOException {

JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame #7663

JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame #7663

Uh oh!

Conversation

tprinzing commented Mar 2, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Mar 2, 2022

Uh oh!

openjdk bot commented Mar 2, 2022

Uh oh!

mlbridge bot commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

naotoj left a comment

Choose a reason for hiding this comment

Uh oh!

naotoj commented Mar 2, 2022

Uh oh!

openjdk bot commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Mar 2, 2022

Uh oh!

mlchung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mlchung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magicus left a comment

Choose a reason for hiding this comment

Uh oh!

tprinzing commented Mar 9, 2022

Uh oh!

openjdk bot commented Mar 9, 2022

Uh oh!

mlchung commented Mar 9, 2022

Uh oh!

openjdk bot commented Mar 9, 2022

Uh oh!

openjdk bot commented Mar 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

tprinzing commented Mar 2, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 2, 2022 •

edited

Loading

openjdk bot commented Mar 2, 2022 •

edited

Loading