JDK-8341094 : Clean up relax_verify in ClassFileParser
  • Type: Enhancement
  • Component: hotspot
  • Sub-Component: runtime
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2024-09-27
  • Updated: 2024-11-27
  • Resolved: 2024-11-14
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 24
24 b25Fixed
Related Reports
Relates :  
JDK-8148854 - Class names "SomeClass" and "LSomeClass;" treated by JVM as an equivalent
Relates :  
JDK-8344922 - Redefinition verifies the new klass when verification is disabled
Relates :  
JDK-8330606 - Redefinition doesn't but should verify the new klass
Description
I'm revisiting fix for JDK-8148854 because I want to clean up some duplicated code.  There's no bug in this fix so nothing to change.  This fix added the function:

static bool relax_format_check_for(ClassLoaderData* loader_data) {
  bool trusted = loader_data->is_boot_class_loader_data() ||
                 loader_data->is_platform_class_loader_data();
  bool need_verify =
    // verifyAll
    (BytecodeVerificationLocal && BytecodeVerificationRemote) ||
    // verifyRemote
    (!BytecodeVerificationLocal && BytecodeVerificationRemote && !trusted);
  return !need_verify;
}

To call instead of

bool Verifier::relax_access_for(oop loader) {
  bool trusted = java_lang_ClassLoader::is_trusted_loader(loader);
  bool need_verify =
    // verifyAll
    (BytecodeVerificationLocal && BytecodeVerificationRemote) ||
    // verifyRemote      
    (!BytecodeVerificationLocal && BytecodeVerificationRemote && !trusted);
  return !need_verify;   
} 

The essential difference in these two functions is java_lang_ClassLoader::is_trusted_loader() returns true for the "system" class loader, also known as the app class loader.  The bug in this report should verify the name for the app class loader and only elide verification for the boot and platform class loader.   Eliding the name verification for the platform class loader seems like an unimportant optimization and not necessary for correctness.  Therefore, we could use the value of _need_verify in classFileParser to decide name verification, which is already tested, eg:

// Checks if name is a legal method name.
void ClassFileParser::verify_legal_method_name(const Symbol* name, TRAPS) const {
  if (!_need_verify || _relax_verify) { return; }
Comments
Changeset: 2145ace3 Branch: master Author: Coleen Phillimore <coleenp@openjdk.org> Date: 2024-11-14 12:08:42 +0000 URL: https://git.openjdk.org/jdk/commit/2145ace384137b1c028a68dc34a8800577c7a43e
14-11-2024
A pull request was submitted for review. Branch: master URL: https://git.openjdk.org/jdk/pull/21954 Date: 2024-11-07 13:38:28 +0000
07-11-2024
Notes from tracing through the logic of when we set need_verify and trigger when to do class file format checking in the class file parser and doing calling the verifier. ClassFileStream::need_verify -> checks for truncated file void guarantee_more(int size, TRAPS) const { size_t remaining = (size_t)(_buffer_end - _current); unsigned int usize = (unsigned int)size; check_truncated_file(usize > remaining, CHECK); } Created by default using the ClassFileStream::verify constant which equals true, except in ClassLoader::load_class it's set like this: ClassLoaderExt::should_verify() static bool should_verify(int classpath_index) { CDS_ONLY(return (classpath_index >= _app_class_paths_start_index);) // essentially boot & platform class loader NOT_CDS(return false;) <= this looks wrong but since the class loader is the null class loader, it should be false. } stream->set_verify(ClassLoaderExt::should_verify()); BUT, this calls KlassFactory::create_from_stream() with the NULL class loader_data, so when it gets to the should_verify_for() function below, it'll always return false. This doesn't do anything! Also, stream->need_verify() is never false for the PlatformClassLoader, it is set to true every where else. Then this value is passed to ClassFileParser to set the value of need_verify. ClassFileParser::need_verify -> checks name formats, ClassFormatError -> then sets InstanceKlass::should_verify_class() initialized like this: // Figure out whether we can skip format checking (matching classic VM behavior) if (CDSConfig::is_dumping_static_archive()) { // verify == true means it's a 'remote' class (i.e., non-boot class) // Verification decision is based on BytecodeVerificationRemote flag // for those classes. _need_verify = (stream->need_verify()) ? BytecodeVerificationRemote : BytecodeVerificationLocal; } else { _need_verify = Verifier::should_verify_for(_loader_data->class_loader(), stream->need_verify()); } // synch back verification state to stream stream->set_verify(_need_verify); Verifier::should_verify_for() is again similar code (not sure why there's a special CDS case in ClassFileParser), but it passes false for boot and platform c lass loader, so false is sticky. platform class loader classes aren't verified. bool Verifier::should_verify_for(oop class_loader, bool should_verify_class) { return (class_loader == nullptr || !should_verify_class) ? BytecodeVerificationLocal : BytecodeVerificationRemote; } Then it's used to set InstanceKlass::should_verify_class(); But the Verifier is called like: return Verifier::verify(this, should_verify_class(), THREAD); Which calls: Verifier::is_eligible_for_verification(InstanceKlass* klass, bool should_verify_class). which calls Verifier::should_verify_for() again, when we already know the answer! It also checks for some special cases, some historical, and some CDS. // Can not verify the bytecodes for shared classes because they have // already been rewritten to contain constant pool cache indices, // which the verifier can't understand. // Shared classes shouldn't have stackmaps either. // However, bytecodes for shared old classes can be verified because // they have not been rewritten. !(klass->is_shared() && klass->is_rewritten())); There is also a similar function in the Verifier that's used to perform package access checks in Reflection::verify_class_access (modules etc). static bool relax_access_for(oop loader) { bool trusted = java_lang_ClassLoader::is_trusted_loader(loader); bool need_verify = // verifyAll (BytecodeVerificationLocal && BytecodeVerificationRemote) || // verifyRemote (!BytecodeVerificationLocal && BytecodeVerificationRemote && !trusted); return !need_verify; Here the trusted loader includes the app/system class loader, but -Xverify=none allows all access (which is a bit strange actually and doesn't seem correct, but we've always done it this way). The original bug in https://bugs.openjdk.org/browse/JDK-8148854 was that we were calling this function from the class file parser and not checking app/system class names. So the proposal is: 1. Remove ClassFileParser::_relax_verify since it's == ClassFileParser::_need_verify + sometimes includes classes loaded with the platform loader. relax_verify only verifies that the names of methods, fields and classes are in the right format. Including this verification for the platform class loader loaded classes won't fail, since these are assumed to be valid names, and the extra time to validate some of them is not concerning for performance. static bool relax_format_check_for(ClassLoaderData* loader_data) { bool trusted = loader_data->is_boot_class_loader_data() || loader_data->is_platform_class_loader_data(); bool need_verify = // verifyAll (BytecodeVerificationLocal && BytecodeVerificationRemote) || // verifyRemote (!BytecodeVerificationLocal && BytecodeVerificationRemote && !trusted); return !need_verify; } _need_verify = relax_format_check_for(); // same as should_verify_for except for the platform loader case. 2. Remove ClassLoaderExt::should_verify() and always pass false for boot class loader, in ClassLoader::load_class. Then Verifier:should_verify_for() will be true if BytecodeVerificationLocal (that is, -Xverify:all) 3. Remove the CDS dumping case in ClassFileParser. For LambdaFunctions in create_from_stream, the default of CFS->need_verify() is true but the class loader data is the null class loader data. So we would verify these during CDS dumping but not for running. This appears unintentional. 4. Lastly in a follow on PR, fix the bug where we don't verify bytecodes for redefined classes (and we should), When we pass Verifier::verify(this, should_verify_class()), the last parameter is the result of calling Verifier::should_verify_for() so gets the same answer. For redefinition, we pass true, but when passing true to Verifier::should_verify_for() it may override to false if the class in the bootclass loader. But we really want to force verifying classes for boot class loader as well in this case!
06-11-2024
> bool trusted = java_lang_ClassLoader::is_trusted_loader(loader); is_trusted_loader has always considered the system/app loader as "trusted", yet it was added to control the name checks and the refactoring placed it in Verifier::relax_access_for instead of relax_format_check_for. So it does look like a bug was introduced with that refactoring - not that anyone apparently ever noticed given this was addressing a change in naming rules between JDK 1.1 and 1.2.
30-09-2024