8341094: Clean up relax_verify in ClassFileParser #21954

coleenp · 2024-11-07T13:38:28Z

Removed ClassFileParser::_relax_verify and cleaned up setting need_verify in the ClassFileStream code and ClassFileParser. Wrote up how I found control to flow to this setting with -Xverify:all/none/remote settings, which become BytecodeVerificationRemote/Local in the CR comments.

Tested with tier1-4. Valid class, field and method name testing is done through the JCK and there are CDS tests that test verification in runtime/cds/appcds/VerifierTest.java.

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-8341094: Clean up relax_verify in ClassFileParser (Enhancement - P4)

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Ioi Lam (@iklam - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21954

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-11-07T13:39:25Z

👋 Welcome back coleenp! 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 · 2024-11-07T13:39:49Z

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

8341094: Clean up relax_verify in ClassFileParser

Reviewed-by: dholmes, iklam

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

95a00f8: 8343875: Minor improvements of jpackage test library
90e9234: 8344074: RISC-V: C1: More accurate _exception_handler_size and _deopt_handler_size
3b28354: 8339288: Improve diagnostic logging runtime/cds/DeterministicDump.java
0dab920: 8343984: Fix Unsafe address overflow
168b18e: 8343958: Remove security manager impl in java.lang.Process and java.lang.Runtime.exec
5ac330b: 8344039: Remove security manager dependency in java.time
1eb38c8: 8343219: Manual clientlibs test failures after SM removal
dde6230: 8343416: CDS dump fails when unregistered class can also be loaded from system modules
ffea980: 8344023: Unnecessary Hashtable usage in LdapClient.defaultBinaryAttrs
5e01c40: 8343981: Remove usage of security manager from Thread and related classes
... and 159 more: https://git.openjdk.org/jdk/compare/8d6cfba37fe641e35886fdba536f5b2f1709e87b...master

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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-11-07T13:40:33Z

@coleenp The following label will be automatically applied to this pull request:

hotspot-runtime

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 · 2024-11-07T13:43:58Z

Webrevs

…ed on one call in ClassFileParser.

iklam · 2024-11-08T03:25:44Z

I am honestly very confused by this code:

We always create ClassFileStream with _need_verify = true
Sometimes we set it back to false in ClassFileParser::ClassFileParser()

  _need_verify = Verifier::should_verify_for(_loader_data->class_loader());
  stream->set_check_truncation(_need_verify);

Can we instead set ClassFileStream::_need_verifyonce and for all inside the ClassFileStream constructor, and use it as a constant afterwards (and propagate to ClassFilePaser, etc)? This can be done by passing a ClassLoader into the ClassFileStream constructor and call Verifier::should_verify_for in there.

Then, you can change _check_truncation back to _need_verify.

ClassFileStream::verify can be removed as it doesn't seem to serve any purpose anymore.

dholmes-ora

I won't pretend that I could follow your analysis in the full. At the end of the day we can relax verification (of all kinds) for trusted classes in the released product, but we should have a way to enforce all verification checks on trusted classes during development in case we introduce a bug and start producing invalid class files. I'm not sure that this latter part is actually in place unless "verify all" is explicitly enabled. Of course we will discover bad trusted class files one way or another, but enabling verification would make it more obvious as to the problem.

Doing additional checks on trusted classes is okay provided it doesn't affect startup or general performance - if verification (and related checks) were free we'd always do them, so there has to be a tipping point.

Thanks

dholmes-ora · 2024-11-08T04:35:53Z

src/hotspot/share/classfile/verifier.hpp

  // Return false if the class is loaded by the bootstrap loader,
  // or if defineClass was called requesting skipping verification
  // -Xverify:all overrides this value


This comment doesn't describe the new operation of this method.

You're right. There was an old change that made defineClass explicitly disable verification. That code is gone now.

dholmes-ora · 2024-11-08T04:37:11Z

src/hotspot/share/classfile/classFileStream.hpp

@@ -52,12 +52,12 @@ class ClassFileStream: public ResourceObj {
  const char* clone_source() const;

 public:
-  static const bool verify;
+  constexpr static bool verify = true;


Where, and how, is this used?

It's passed into the ClassFileStream constructor with default value true.

I removed this and fixed all the constructor calls.

coleenp · 2024-11-08T11:41:05Z

Ioi, I like your suggestion. That would make it even clearer. We could set the value of _need_verify once in ClassFileStream constructor, and propagate the value of _need_verify to ClassFileParser. ClassFileParser has a lot of checks for _need_verify so it's still better for ClassFileParser to have one.

David, there are three different concepts of "trusted" in this code which caused the duplication that I'm trying to resolve. One is for package access which includes boot/platform/system class loader.
Two is for this relax_verify to check the names of class/fields/methods which includes boot/platform class loader.
Three is for checking stream truncation, doing classfile format checks and running the Verifier itself which only includes the boot class loader.

My change removes #2 since checking names is a fast enough operation (doesn't call java unless < JDK 1.5), and most , and maybe all platform class loading is called with ClassFileStream::_need_verify == true anyway, so this code checked these names anyway.

Further, I'm trying to only check for when we want verification #3 in only one place as it was set and reset between ClassFileStream and ClassFileParser, and the same code is called again effectively ignoring the value of should_verify_class value passed into the Verifier but getting the same result as the last call.

coleenp · 2024-11-08T12:12:20Z

Ioi, your suggestion of passing class_loader to stream creation is really a cumbersome change. The Stream's version of need_verify is an internal flag used for determining whether we need to check for truncation and should be based on the ClassFileParser's call when we're about to parse the stream. But I did some cleanup to further unexpose this field.

…y based on what ClassFileParser gets for the answer based on the BytecodeVerificationnRemote/Local flags and the class loader.

dholmes-ora

I like the update so that CFS defaults to needing verification unless CFP sets it otherwise.

iklam · 2024-11-12T05:43:45Z

Why do we need ClassFileStream::_need_verify? Is it for performance? If so, we are making a trade off here -- boot classes will be marginally faster, but non-boot classes will be marginally slower. Assuming that large apps load many more non-boot classes than boot classes, this seems counter productive.

Maybe we should get rid of ClassFileStream::_need_verify altogether?

dholmes-ora · 2024-11-12T07:09:58Z

Maybe we should get rid of ClassFileStream::_need_verify altogether?

Get rid of it in what way? Implicitly always true, or implicitly always false? Or?

iklam · 2024-11-12T07:36:38Z

Maybe we should get rid of ClassFileStream::_need_verify altogether?

Get rid of it in what way? Implicitly always true, or implicitly always false? Or?

Always true. E.g.,

  u2 get_u2(TRAPS) const {
-   if (_need_verify) {
      guarantee_more(2, CHECK_0);
-   } else {
-     assert(2 <= _buffer_end - _current, "buffer overflow");
-   }
    return get_u2_fast();
  }

dholmes-ora · 2024-11-12T09:22:59Z

Always true.

We need to check performance here, particularly startup, as that is what would be most affected by always verifying boot classes.

coleenp · 2024-11-12T13:03:06Z

Yes, the classFileStream::_need_verify was added for performance. We had a one-pager design document (now gone) and everything for JDK-4990299. This was before CDS was default, but this isn't really the purpose of my making this change. I'm trying to fix another bug after this cleanup.

iklam

LGTM. This simplifies the code. There doesn't seem to be security risks as this PR only strengths the checks.

dholmes-ora

If the changes are now stable then I am fine with them.

Thanks

coleenp · 2024-11-14T12:07:54Z

Thanks Ioi and David for your reviews and improvements.
/integrate

openjdk · 2024-11-14T12:08:46Z

Going to push as commit 2145ace.
Since your change was applied there have been 182 commits pushed to the master branch:

5731ab7: 8335991: Implement Simple Source Files and Instance Main Methods (Fourth Preview)
81342ac: 8343752: The javadoc should contain a note about usages of requires transitive java.base;
8523880: 8342693: Use byte[] as parameter in a FDBigInteger constructor and as field
2b57f40: 8343426: ConcurrentSkipListMap.spliterator() can no longer split the stream
a8152bd: 8343941: IGV: dump graph at different register allocation steps
bd6152f: 8343855: HTTP/2 ConnectionWindowUpdateSender may miss some unprocessed DataFrames from closed streams
c3776db: 8342936: Enhance java.io.IO with parameter-less println() and readln()
b54bd82: 8344025: Remove unused ISO2022.Encoder.maximumDesignatorLength
abacece: 8344011: Remove usage of security manager from Class and reflective APIs
c977ef7: 8342047: Create Template Assertion Predicates with Halt nodes only instead of uncommon traps
... and 172 more: https://git.openjdk.org/jdk/compare/8d6cfba37fe641e35886fdba536f5b2f1709e87b...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-11-14T12:08:53Z

@coleenp Pushed as commit 2145ace.

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

8341094: Clean up relax_verify in ClassFileParser

6f74df4

openjdk bot added the rfr Pull request is ready for review label Nov 7, 2024

openjdk bot added the hotspot-runtime [email protected] label Nov 7, 2024

Rename ClassFileStream::_need_verify to _check_truncation and set bas…

c8597cb

…ed on one call in ClassFileParser.

dholmes-ora reviewed Nov 8, 2024

View reviewed changes

Unexpose the ClassFileStream's need_verify field. It's used internall…

8c1c3e3

…y based on what ClassFileParser gets for the answer based on the BytecodeVerificationnRemote/Local flags and the class loader.

dholmes-ora reviewed Nov 11, 2024

View reviewed changes

iklam approved these changes Nov 13, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 13, 2024

dholmes-ora approved these changes Nov 14, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Nov 14, 2024

openjdk bot closed this Nov 14, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 14, 2024

coleenp deleted the remove-relax-verify branch November 14, 2024 12:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8341094: Clean up relax_verify in ClassFileParser #21954

8341094: Clean up relax_verify in ClassFileParser #21954

coleenp commented Nov 7, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 7, 2024

openjdk bot commented Nov 7, 2024 •

edited

Loading

openjdk bot commented Nov 7, 2024

mlbridge bot commented Nov 7, 2024 •

edited

Loading

iklam commented Nov 8, 2024

dholmes-ora left a comment

dholmes-ora Nov 8, 2024

coleenp Nov 8, 2024

dholmes-ora Nov 8, 2024

coleenp Nov 8, 2024

coleenp Nov 8, 2024

coleenp commented Nov 8, 2024

coleenp commented Nov 8, 2024

dholmes-ora left a comment

iklam commented Nov 12, 2024

dholmes-ora commented Nov 12, 2024

iklam commented Nov 12, 2024

dholmes-ora commented Nov 12, 2024

coleenp commented Nov 12, 2024 •

edited

Loading

iklam left a comment

dholmes-ora left a comment

coleenp commented Nov 14, 2024

openjdk bot commented Nov 14, 2024

openjdk bot commented Nov 14, 2024

8341094: Clean up relax_verify in ClassFileParser #21954

8341094: Clean up relax_verify in ClassFileParser #21954

Conversation

coleenp commented Nov 7, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 7, 2024

openjdk bot commented Nov 7, 2024 • edited Loading

openjdk bot commented Nov 7, 2024

mlbridge bot commented Nov 7, 2024 • edited Loading

Webrevs

iklam commented Nov 8, 2024

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora Nov 8, 2024

Choose a reason for hiding this comment

coleenp Nov 8, 2024

Choose a reason for hiding this comment

dholmes-ora Nov 8, 2024

Choose a reason for hiding this comment

coleenp Nov 8, 2024

Choose a reason for hiding this comment

coleenp Nov 8, 2024

Choose a reason for hiding this comment

coleenp commented Nov 8, 2024

coleenp commented Nov 8, 2024

dholmes-ora left a comment

Choose a reason for hiding this comment

iklam commented Nov 12, 2024

dholmes-ora commented Nov 12, 2024

iklam commented Nov 12, 2024

dholmes-ora commented Nov 12, 2024

coleenp commented Nov 12, 2024 • edited Loading

iklam left a comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

coleenp commented Nov 14, 2024

openjdk bot commented Nov 14, 2024

openjdk bot commented Nov 14, 2024

coleenp commented Nov 7, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Nov 7, 2024 •

edited

Loading

mlbridge bot commented Nov 7, 2024 •

edited

Loading

coleenp commented Nov 12, 2024 •

edited

Loading