Re: RFR: 8261031: Move some ClassLoader name checking to native/VM [v3]
On Fri, 12 Feb 2021 22:48:51 GMT, Mandy Chung wrote: >> This more limited cleanup looks good. > > This patch changes `JVM_FindLoadedClass` interface to only accept a binary > name. It used to accept both a binary name and internal form. Most, if not > all, JVM entry points take the name of internal name. So this change makes > this JVM entry point inconsistent with others. > > Looking closer each API that involves `fixClassName` or `verifyXXXClassName`, > the JVM entry points called expects the internal form except > `JVM_FindLoadedClass` (see details below). I think a better change is to > change the native `JVM_FindLoadedClass` to accept the internal form only and > have `findLoadedClass0` method to detect if the name contains '/' or '['. > > ClassLoader API does not allow loading of an array type whereas > `Class::forName` allows to find an array type. Perhaps `verifyFixClassName` > should be renamed like `binaryNameToInternalForm`. I think we don't need > `fixClassname`? > > ClassLoader::defineClass > - `preDefineClass` checks the name and throws if it contains '/' or '[' > - no name check in `JVM_DefineClassWithSource` and `JVM_LookupDefineClass` > which expects the name is of internal form > > native Class::forName0 > - converts the binary name to internal form (i.e. replace '.' with '/') > - throw if the name contains '/' > - no explicit name check in `JVM_FindClassFromCaller` > > ClassLoader::loadClass > - calls native `findLoadedClass0` that calls `JVM_FindLoadedClass` which > >accepts binary form and converts '.' to '/' but the current implementation >accepts both binary name and internal form > - calls `native findBootstrapClass` which converts '.' to '/' and pass the > internal >form to `JVM_FindBootstrapClass`. > > It'd be helpful to document the internal APIs and JVM entry points clearly > what it expects for example binary name vs internal form and where it does > the validation e.g. Class::forName0 allows array type and native library > methods do the name validation. Abandoning this. Sorry for wasting everyone's time. - PR: https://git.openjdk.java.net/jdk/pull/2378
Re: RFR: 8261031: Move some ClassLoader name checking to native/VM [v3]
On Fri, 12 Feb 2021 02:10:02 GMT, Coleen Phillimore wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Consolidate verifyClassname and verifyFixClassname > > This more limited cleanup looks good. This patch changes `JVM_FindLoadedClass` interface to only accept a binary name. It used to accept both a binary name and internal form. Most, if not all, JVM entry points take the name of internal name. So this change makes this JVM entry point inconsistent with others. Looking closer each API that involves `fixClassName` or `verifyXXXClassName`, the JVM entry points called expects the internal form except `JVM_FindLoadedClass` (see details below). I think a better change is to change the native `JVM_FindLoadedClass` to accept the internal form only and have `findLoadedClass0` method to detect if the name contains '/' or '['. ClassLoader API does not allow loading of an array type whereas `Class::forName` allows to find an array type. Perhaps `verifyFixClassName` should be renamed like `binaryNameToInternalForm`. I think we don't need `fixClassname`? ClassLoader::defineClass - `preDefineClass` checks the name and throws if it contains '/' or '[' - no name check in `JVM_DefineClassWithSource` and `JVM_LookupDefineClass` which expects the name is of internal form native Class::forName0 - converts the binary name to internal form (i.e. replace '.' with '/') - throw if the name contains '/' - no explicit name check in `JVM_FindClassFromCaller` ClassLoader::loadClass - calls native `findLoadedClass0` that calls `JVM_FindLoadedClass` which accepts binary form and converts '.' to '/' but the current implementation accepts both binary name and internal form - calls `native findBootstrapClass` which converts '.' to '/' and pass the internal form to `JVM_FindBootstrapClass`. It'd be helpful to document the internal APIs and JVM entry points clearly what it expects for example binary name vs internal form and where it does the validation e.g. Class::forName0 allows array type and native library methods do the name validation. - PR: https://git.openjdk.java.net/jdk/pull/2378
Re: RFR: 8261031: Move some ClassLoader name checking to native/VM [v3]
On Thu, 11 Feb 2021 12:44:54 GMT, Claes Redestad wrote: >> This patch moves some sanity checking done in ClassLoader.java to the >> corresponding endpoints in native or VM code. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Consolidate verifyClassname and verifyFixClassname This more limited cleanup looks good. - Marked as reviewed by coleenp (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2378
Re: RFR: 8261031: Move some ClassLoader name checking to native/VM [v3]
On Thu, 4 Feb 2021 13:14:16 GMT, Coleen Phillimore wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Consolidate verifyClassname and verifyFixClassname > > Changes requested by coleenp (Reviewer). I tried to consolidate all name checking into the native layer for the remaining methods, but there are places where we are calling the JNI code with internalized names directly through `JavaLangAccess.defineClass`, so we'd need a way to differentiate these. Seems simpler to leave the `checkName` in `preDefineClass` for now. For the JNI code consolidating verifyFixClassname and verifyClassname into a single method seems to be the most straightforward simplification possible, since these are currently called back to back. Since ASCII like `/` is never a component of a multibyte character in UTF-8 we can do the fix-up pass without validation, then do the full verification. This simplifies the code and might speed it up marginally. Also added some cleanup to the cleanup code as suggested by @tstuefe in #2407 - PR: https://git.openjdk.java.net/jdk/pull/2378
Re: RFR: 8261031: Move some ClassLoader name checking to native/VM [v3]
> This patch moves some sanity checking done in ClassLoader.java to the > corresponding endpoints in native or VM code. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Consolidate verifyClassname and verifyFixClassname - Changes: - all: https://git.openjdk.java.net/jdk/pull/2378/files - new: https://git.openjdk.java.net/jdk/pull/2378/files/727b2b37..6b8305e9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2378=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2378=01-02 Stats: 77 lines in 4 files changed: 13 ins; 40 del; 24 mod Patch: https://git.openjdk.java.net/jdk/pull/2378.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2378/head:pull/2378 PR: https://git.openjdk.java.net/jdk/pull/2378
Re: RFR: 8261031: Move some ClassLoader name checking to native/VM [v2]
> This patch moves some sanity checking done in ClassLoader.java to the > corresponding endpoints in native or VM code. Claes Redestad has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - Merge branch 'master' into checkName - Merge branch 'master' into checkName - Copyrights - Move class name checking for findBootstrapClass/findLoadedClass into native/VM - Changes: - all: https://git.openjdk.java.net/jdk/pull/2378/files - new: https://git.openjdk.java.net/jdk/pull/2378/files/f2fd1d1c..727b2b37 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2378=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2378=00-01 Stats: 28701 lines in 954 files changed: 16489 ins; 8085 del; 4127 mod Patch: https://git.openjdk.java.net/jdk/pull/2378.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2378/head:pull/2378 PR: https://git.openjdk.java.net/jdk/pull/2378
Re: RFR: 8261031: Move some ClassLoader name checking to native/VM
On Thu, 4 Feb 2021 12:54:43 GMT, Coleen Phillimore wrote:
>> This patch moves some sanity checking done in ClassLoader.java to the
>> corresponding endpoints in native or VM code.
>
> src/java.base/share/classes/java/lang/ClassLoader.java line 1259:
>
>> 1257: Class findBootstrapClassOrNull(String name) {
>> 1258: return findBootstrapClass(name);
>> 1259: }
>
> I'm confused why this would improve performance. Wouldn't avoiding the
> transition between Java to the VM be good? Or is checkName seldom false, so
> we're checking valid names for nothing?
It's practically never false, so the checking done here is just extra work. The
patch skips execution of a few thousand bytecode on startup as is, but I'm
reworking it to try and get rid of the last remaining checkName use clean up
the verifyFixClassName/fixClassName use to perhaps consolidate code there for a
bit.
-
PR: https://git.openjdk.java.net/jdk/pull/2378
Re: RFR: 8261031: Move some ClassLoader name checking to native/VM
On Wed, 3 Feb 2021 19:49:30 GMT, Mandy Chung wrote:
>> This patch moves some sanity checking done in ClassLoader.java to the
>> corresponding endpoints in native or VM code.
>
> src/java.base/share/native/libjava/ClassLoader.c line 291:
>
>> 289: }
>> 290: // disallow slashes in input, change '.' to '/'
>> 291: if (verifyFixClassname(clname)) {
>
> perhaps we should replace all use of `fixClassname` with `verifyFixClassname`
> and remove `fixClassname`.
This suggestion makes sense to me. verifyClassName is only used once in
Class.c passing false so you could remove that argument.
It's hard to see how fixClassName then verifyClassname is equivalent to
verifyFixClassname but verifyFixClassname makes more sense than verifyClassname.
I think this return:
return (p != 0 && p - name == (ptrdiff_t)length);
implies a non-utf8 character was found?
-
PR: https://git.openjdk.java.net/jdk/pull/2378
Re: RFR: 8261031: Move some ClassLoader name checking to native/VM
On Thu, 4 Feb 2021 13:11:47 GMT, Coleen Phillimore wrote:
>> src/java.base/share/native/libjava/ClassLoader.c line 291:
>>
>>> 289: }
>>> 290: // disallow slashes in input, change '.' to '/'
>>> 291: if (verifyFixClassname(clname)) {
>>
>> perhaps we should replace all use of `fixClassname` with
>> `verifyFixClassname` and remove `fixClassname`.
>
> This suggestion makes sense to me. verifyClassName is only used once in
> Class.c passing false so you could remove that argument.
> It's hard to see how fixClassName then verifyClassname is equivalent to
> verifyFixClassname but verifyFixClassname makes more sense than
> verifyClassname.
> I think this return:
> return (p != 0 && p - name == (ptrdiff_t)length);
> implies a non-utf8 character was found?
Actually I think replacing fixClassName with verifyFixClassname will be awkward
since the latter returns a value that's not checked in all the callers of
fixClassName. Maybe you could write fixClassName as:
void fixClassName() { verifyFixClassName(); with some assertion it passed? }
-
PR: https://git.openjdk.java.net/jdk/pull/2378
Re: RFR: 8261031: Move some ClassLoader name checking to native/VM
On Wed, 3 Feb 2021 12:21:30 GMT, Claes Redestad wrote:
> This patch moves some sanity checking done in ClassLoader.java to the
> corresponding endpoints in native or VM code.
Changes requested by coleenp (Reviewer).
src/java.base/share/classes/java/lang/ClassLoader.java line 1259:
> 1257: Class findBootstrapClassOrNull(String name) {
> 1258: return findBootstrapClass(name);
> 1259: }
I'm confused why this would improve performance. Wouldn't avoiding the
transition between Java to the VM be good? Or is checkName seldom false, so
we're checking valid names for nothing?
-
PR: https://git.openjdk.java.net/jdk/pull/2378
Re: RFR: 8261031: Move some ClassLoader name checking to native/VM
On Wed, 3 Feb 2021 12:21:30 GMT, Claes Redestad wrote:
> This patch moves some sanity checking done in ClassLoader.java to the
> corresponding endpoints in native or VM code.
I'm unsure the benefit of moving the check done by `checkName` to the VM but
`preDefineClass` still calls `checkName`. The overhead of `checkName` should
be fairly negligible?
src/java.base/share/native/libjava/ClassLoader.c line 291:
> 289: }
> 290: // disallow slashes in input, change '.' to '/'
> 291: if (verifyFixClassname(clname)) {
perhaps we should replace all use of `fixClassname` with `verifyFixClassname`
and remove `fixClassname`.
-
PR: https://git.openjdk.java.net/jdk/pull/2378
