RFR: 8261031: Move some ClassLoader name checking to native/VM

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8261031: Move some ClassLoader name checking to native/VM

Claes Redestad-2
This patch moves some sanity checking done in ClassLoader.java to the corresponding endpoints in native or VM code.

-------------

Commit messages:
 - Copyrights
 - Move class name checking for findBootstrapClass/findLoadedClass into native/VM

Changes: https://git.openjdk.java.net/jdk/pull/2378/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2378&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261031
  Stats: 20 lines in 3 files changed: 11 ins; 4 del; 5 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261031: Move some ClassLoader name checking to native/VM

Coleen Phillimore-3
On Wed, 3 Feb 2021 12:21:30 GMT, Claes Redestad <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261031: Move some ClassLoader name checking to native/VM

Coleen Phillimore-3
In reply to this post by Claes Redestad-2
On Wed, 3 Feb 2021 19:49:30 GMT, Mandy Chung <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261031: Move some ClassLoader name checking to native/VM

Coleen Phillimore-3
On Thu, 4 Feb 2021 13:11:47 GMT, Coleen Phillimore <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261031: Move some ClassLoader name checking to native/VM

Claes Redestad-2
In reply to this post by Coleen Phillimore-3
On Thu, 4 Feb 2021 12:54:43 GMT, Coleen Phillimore <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261031: Move some ClassLoader name checking to native/VM [v2]

Claes Redestad-2
In reply to this post by Claes Redestad-2
> 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&pr=2378&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2378&range=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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261031: Move some ClassLoader name checking to native/VM [v3]

Claes Redestad-2
In reply to this post by Coleen Phillimore-3
On Thu, 4 Feb 2021 13:14:16 GMT, Coleen Phillimore <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261031: Move some ClassLoader name checking to native/VM [v3]

Claes Redestad-2
In reply to this post by Claes Redestad-2
> 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&pr=2378&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2378&range=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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261031: Move some ClassLoader name checking to native/VM [v3]

Coleen Phillimore-3
On Thu, 11 Feb 2021 12:44:54 GMT, Claes Redestad <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261031: Move some ClassLoader name checking to native/VM [v3]

Mandy Chung-2
On Fri, 12 Feb 2021 02:10:02 GMT, Coleen Phillimore <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261031: Move some ClassLoader name checking to native/VM [v3]

Claes Redestad-2
On Fri, 12 Feb 2021 22:48:51 GMT, Mandy Chung <[hidden email]> 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