Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8341094: Clean up relax_verify in ClassFileParser #21954

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 6 additions & 32 deletions src/hotspot/share/classfile/classFileParser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4659,7 +4659,7 @@ const char* ClassFileParser::skip_over_field_signature(const char* signature,

// Checks if name is a legal class name.
void ClassFileParser::verify_legal_class_name(const Symbol* name, TRAPS) const {
if (!_need_verify || _relax_verify) { return; }
if (!_need_verify) { return; }

assert(name->refcount() > 0, "symbol must be kept alive");
char* bytes = (char*)name->bytes();
Expand Down Expand Up @@ -4699,7 +4699,7 @@ void ClassFileParser::verify_legal_class_name(const Symbol* name, TRAPS) const {

// Checks if name is a legal field name.
void ClassFileParser::verify_legal_field_name(const Symbol* name, TRAPS) const {
if (!_need_verify || _relax_verify) { return; }
if (!_need_verify) { return; }

char* bytes = (char*)name->bytes();
unsigned int length = name->utf8_length();
Expand Down Expand Up @@ -4732,7 +4732,7 @@ void ClassFileParser::verify_legal_field_name(const Symbol* name, TRAPS) const {

// 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; }
if (!_need_verify) { return; }

assert(name != nullptr, "method name is null");
char* bytes = (char*)name->bytes();
Expand Down Expand Up @@ -5236,17 +5236,6 @@ void ClassFileParser::update_class_name(Symbol* new_class_name) {
_class_name->increment_refcount();
}

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;
}

ClassFileParser::ClassFileParser(ClassFileStream* stream,
Symbol* name,
ClassLoaderData* loader_data,
Expand Down Expand Up @@ -5303,7 +5292,6 @@ ClassFileParser::ClassFileParser(ClassFileStream* stream,
_itfs_len(0),
_java_fields_count(0),
_need_verify(false),
_relax_verify(false),
_has_nonstatic_concrete_methods(false),
_declares_nonstatic_concrete_methods(false),
_has_localvariable_table(false),
Expand All @@ -5324,24 +5312,10 @@ ClassFileParser::ClassFileParser(ClassFileStream* stream,
assert(0 == _access_flags.as_int(), "invariant");

// 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);
_need_verify = Verifier::should_verify_for(_loader_data->class_loader());

// Check if verification needs to be relaxed for this class file
// Do not restrict it to jdk1.0 or jdk1.1 to maintain backward compatibility (4982376)
_relax_verify = relax_format_check_for(_loader_data);
// synch back verification state to stream to check for truncation.
stream->set_check_truncation(_need_verify);

parse_stream(stream, CHECK);

Expand Down
1 change: 0 additions & 1 deletion src/hotspot/share/classfile/classFileParser.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,6 @@ class ClassFileParser {
u2 _java_fields_count;

bool _need_verify;
bool _relax_verify;

bool _has_nonstatic_concrete_methods;
bool _declares_nonstatic_concrete_methods;
Expand Down
10 changes: 4 additions & 6 deletions src/hotspot/share/classfile/classFileStream.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1997, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1997, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -28,22 +28,20 @@
#include "classfile/vmSymbols.hpp"
#include "memory/resourceArea.hpp"

const bool ClassFileStream::verify = true;

void ClassFileStream::truncated_file_error(TRAPS) const {
THROW_MSG(vmSymbols::java_lang_ClassFormatError(), "Truncated class file");
}

ClassFileStream::ClassFileStream(const u1* buffer,
int length,
const char* source,
bool verify_stream,
bool check_truncation,
bool from_boot_loader_modules_image) :
_buffer_start(buffer),
_buffer_end(buffer + length),
_current(buffer),
_source(source),
_need_verify(verify_stream),
_check_truncation(check_truncation),
_from_boot_loader_modules_image(from_boot_loader_modules_image) {
assert(buffer != nullptr, "caller should throw NPE");
}
Expand Down Expand Up @@ -72,6 +70,6 @@ const ClassFileStream* ClassFileStream::clone() const {
return new ClassFileStream(new_buffer_start,
length(),
clone_source(),
need_verify(),
check_truncation(),
from_boot_loader_modules_image());
}
18 changes: 9 additions & 9 deletions src/hotspot/share/classfile/classFileStream.hpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1997, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1997, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -43,7 +43,7 @@ class ClassFileStream: public ResourceObj {
const u1* const _buffer_end; // Buffer top (one past last element)
mutable const u1* _current; // Current buffer position
const char* const _source; // Source of stream (directory name, ZIP/JAR archive name)
bool _need_verify; // True if verification is on for the class file
bool _check_truncation; // True if we should check truncation of stream bytes.
bool _from_boot_loader_modules_image; // True if this was created by ClassPathImageEntry.
void truncated_file_error(TRAPS) const ;

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

public:
static const bool verify;
constexpr static bool verify = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where, and how, is this used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this and fixed all the constructor calls.


ClassFileStream(const u1* buffer,
int length,
const char* source,
bool verify_stream = verify, // to be verified by default
bool check_truncation = verify,
bool from_boot_loader_modules_image = false);

virtual const ClassFileStream* clone() const;
Expand All @@ -76,8 +76,8 @@ class ClassFileStream: public ResourceObj {
return (juint)(_current - _buffer_start);
}
const char* source() const { return _source; }
bool need_verify() const { return _need_verify; }
void set_verify(bool flag) { _need_verify = flag; }
bool check_truncation() const { return _check_truncation; }
void set_check_truncation(bool flag) { _check_truncation = flag; }
bool from_boot_loader_modules_image() const { return _from_boot_loader_modules_image; }

void check_truncated_file(bool b, TRAPS) const {
Expand All @@ -97,7 +97,7 @@ class ClassFileStream: public ResourceObj {
return *_current++;
}
u1 get_u1(TRAPS) const {
if (_need_verify) {
if (_check_truncation) {
guarantee_more(1, CHECK_0);
} else {
assert(1 <= _buffer_end - _current, "buffer overflow");
Expand All @@ -112,7 +112,7 @@ class ClassFileStream: public ResourceObj {
return res;
}
u2 get_u2(TRAPS) const {
if (_need_verify) {
if (_check_truncation) {
guarantee_more(2, CHECK_0);
} else {
assert(2 <= _buffer_end - _current, "buffer overflow");
Expand All @@ -136,7 +136,7 @@ class ClassFileStream: public ResourceObj {

// Skip length elements from stream
void skip_u1(int length, TRAPS) const {
if (_need_verify) {
if (_check_truncation) {
guarantee_more(length, CHECK);
}
skip_u1_fast(length);
Expand Down
2 changes: 0 additions & 2 deletions src/hotspot/share/classfile/classLoader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1198,8 +1198,6 @@ InstanceKlass* ClassLoader::load_class(Symbol* name, PackageEntry* pkg_entry, bo
return nullptr;
}

stream->set_verify(ClassLoaderExt::should_verify(classpath_index));

ClassLoaderData* loader_data = ClassLoaderData::the_null_class_loader_data();
Handle protection_domain;
ClassLoadInfo cl_info(protection_domain);
Expand Down
7 changes: 1 addition & 6 deletions src/hotspot/share/classfile/classLoaderExt.hpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2014, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2014, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -33,11 +33,6 @@ class ClassListParser;

class ClassLoaderExt: public ClassLoader { // AllStatic
public:
static bool should_verify(int classpath_index) {
CDS_ONLY(return (classpath_index >= _app_class_paths_start_index);)
NOT_CDS(return false;)
}

#if INCLUDE_CDS
private:
enum SomeConstants {
Expand Down
4 changes: 2 additions & 2 deletions src/hotspot/share/classfile/klassFactory.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2015, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2015, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -158,7 +158,7 @@ static ClassFileStream* check_class_file_load_hook(ClassFileStream* stream,
stream = new ClassFileStream(ptr,
pointer_delta_as_int(end_ptr, ptr),
stream->source(),
stream->need_verify());
stream->check_truncation());
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/classfile/systemDictionary.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -825,7 +825,7 @@ InstanceKlass* SystemDictionary::resolve_hidden_class_from_stream(
loader_data = register_loader(class_loader, create_mirror_cld);

assert(st != nullptr, "invariant");
assert(st->need_verify(), "invariant");
assert(st->check_truncation(), "invariant");

// Parse stream and create a klass.
InstanceKlass* k = KlassFactory::create_from_stream(st,
Expand Down
6 changes: 3 additions & 3 deletions src/hotspot/share/classfile/verifier.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,8 @@ static verify_byte_codes_fn_t verify_byte_codes_fn() {

// Methods in Verifier

bool Verifier::should_verify_for(oop class_loader, bool should_verify_class) {
return (class_loader == nullptr || !should_verify_class) ?
bool Verifier::should_verify_for(oop class_loader) {
return class_loader == nullptr ?
BytecodeVerificationLocal : BytecodeVerificationRemote;
}

Expand Down Expand Up @@ -276,7 +276,7 @@ bool Verifier::verify(InstanceKlass* klass, bool should_verify_class, TRAPS) {
bool Verifier::is_eligible_for_verification(InstanceKlass* klass, bool should_verify_class) {
Symbol* name = klass->name();

return (should_verify_for(klass->class_loader(), should_verify_class) &&
return (should_verify_class &&
// return if the class is a bootstrapping class
// or defineClass specified not to verify by default (flags override passed arg)
// We need to skip the following four for bootstraping
Expand Down
4 changes: 2 additions & 2 deletions src/hotspot/share/classfile/verifier.hpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1998, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1998, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -52,7 +52,7 @@ class Verifier : AllStatic {
// Return false if the class is loaded by the bootstrap loader,
// or if defineClass was called requesting skipping verification
// -Xverify:all overrides this value
Comment on lines 52 to 54
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

static bool should_verify_for(oop class_loader, bool should_verify_class);
static bool should_verify_for(oop class_loader);

// Relax certain access checks to enable some broken 1.1 apps to run on 1.2.
static bool relax_access_for(oop class_loader);
Expand Down