Re: RFR: JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame [v3]

2022-03-03 Thread Tim Prinzing
On Wed, 2 Mar 2022 22:21:03 GMT, Mandy Chung  wrote:

>> Tim Prinzing has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   suggested changes
>
> test/jdk/java/util/ResourceBundle/exeNullCallerResourceBundle/exeNullCallerResourceBundle.c
>  line 28:
> 
>> 26: 
>> 27: #include "jni.h"
>> 28: #undef NDEBUG
> 
> is this line unintended?

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

-

PR: https://git.openjdk.java.net/jdk/pull/7663


Re: RFR: JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame [v3]

2022-03-02 Thread Mandy Chung
On Wed, 2 Mar 2022 21:53:01 GMT, Tim Prinzing  wrote:

>> 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.
>
> Tim Prinzing has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   suggested changes

Thanks for the update.

src/java.base/share/classes/java/util/ResourceBundle.java line 1567:

> 1565:  * module will be used.
> 1566:  * @param caller
> 1567:  * @return

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

src/java.base/share/classes/java/util/ResourceBundle.java line 2251:

> 2249: @CallerSensitive
> 2250: public static final void clearCache() {
> 2251: final Module callerModule = 
> getCallerModule(Reflection.getCallerClass());

nit: final can be dropped here.

test/jdk/java/util/ResourceBundle/exeNullCallerResourceBundle/NullCallerResourceBundle.java
 line 52:

> 50: import java.nio.file.Paths;
> 51: import java.util.Properties;
> 52: import java.util.ResourceBundle;

nit: good to keep the imports in alphabetic order.

test/jdk/java/util/ResourceBundle/exeNullCallerResourceBundle/NullCallerResourceBundle.java
 line 74:

> 72: 
> 73: // set up shared library path
> 74: final String sharedLibraryPathEnvName = 
> Platform.sharedLibraryPathVariableName();

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

test/jdk/java/util/ResourceBundle/exeNullCallerResourceBundle/NullCallerResourceBundle.java
 line 89:

> 87: 
> 88: 
> 89: private static void makePropertiesFile() throws IOException {

This can be removed?

test/jdk/java/util/ResourceBundle/exeNullCallerResourceBundle/exeNullCallerResourceBundle.c
 line 28:

> 26: 
> 27: #include "jni.h"
> 28: #undef NDEBUG

is this line unintended?

-

PR: https://git.openjdk.java.net/jdk/pull/7663


Re: RFR: JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame [v3]

2022-03-02 Thread Tim Prinzing
> 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.

Tim Prinzing has updated the pull request incrementally with one additional 
commit since the last revision:

  suggested changes

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7663/files
  - new: https://git.openjdk.java.net/jdk/pull/7663/files/9e8fb193..eeb2d0fa

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=7663=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7663=01-02

  Stats: 48 lines in 1 file changed: 6 ins; 41 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7663.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7663/head:pull/7663

PR: https://git.openjdk.java.net/jdk/pull/7663