From 8de158aefe64d493e107ef310f510bab57beb251 Mon Sep 17 00:00:00 2001 From: David Holmes Date: Mon, 25 Nov 2024 19:55:26 +0000 Subject: [PATCH] 8339134: Callers of Exceptions::fthrow should ensure exception message lengths avoid the INT_MAX limits of os::vsnprintf Reviewed-by: coleenp, jsjolen --- src/hotspot/share/classfile/classFileError.cpp | 4 ++++ src/hotspot/share/classfile/classFileParser.cpp | 17 +++++++++++++++++ src/hotspot/share/interpreter/linkResolver.cpp | 6 ++++++ src/hotspot/share/oops/constantPool.cpp | 1 + src/hotspot/share/oops/instanceKlass.cpp | 3 +++ src/hotspot/share/runtime/reflection.cpp | 1 + .../share/services/diagnosticArgument.cpp | 17 +++++++++++------ .../share/services/diagnosticArgument.hpp | 1 + src/hotspot/share/utilities/exceptions.cpp | 5 +++++ 9 files changed, 49 insertions(+), 6 deletions(-) diff --git a/src/hotspot/share/classfile/classFileError.cpp b/src/hotspot/share/classfile/classFileError.cpp index 4de5c288955..f6bac278e60 100644 --- a/src/hotspot/share/classfile/classFileError.cpp +++ b/src/hotspot/share/classfile/classFileError.cpp @@ -34,6 +34,10 @@ PRAGMA_DIAG_PUSH PRAGMA_FORMAT_NONLITERAL_IGNORED +// None of the error routines below take in a free-form, potentially unbounded +// string, and names are all limited to < 64K, so we know that all formatted +// strings passed to fthrow will not be excessively large. + void ClassFileParser::classfile_parse_error(const char* msg, TRAPS) const { assert(_class_name != nullptr, "invariant"); ResourceMark rm(THREAD); diff --git a/src/hotspot/share/classfile/classFileParser.cpp b/src/hotspot/share/classfile/classFileParser.cpp index 12e7cf1ae9d..f0586cd7bcc 100644 --- a/src/hotspot/share/classfile/classFileParser.cpp +++ b/src/hotspot/share/classfile/classFileParser.cpp @@ -1794,6 +1794,7 @@ void ClassFileParser::throwIllegalSignature(const char* type, assert(sig != nullptr, "invariant"); ResourceMark rm(THREAD); + // Names are all known to be < 64k so we know this formatted message is not excessively large. Exceptions::fthrow(THREAD_AND_LOCATION, vmSymbols::java_lang_ClassFormatError(), "%s \"%s\" in class %s has illegal signature \"%s\"", type, @@ -4073,6 +4074,8 @@ void ClassFileParser::check_super_class_access(const InstanceKlass* this_klass, char* msg = Reflection::verify_class_access_msg(this_klass, InstanceKlass::cast(super), vca_result); + + // Names are all known to be < 64k so we know this formatted message is not excessively large. if (msg == nullptr) { bool same_module = (this_klass->module() == super->module()); Exceptions::fthrow( @@ -4121,6 +4124,8 @@ void ClassFileParser::check_super_interface_access(const InstanceKlass* this_kla char* msg = Reflection::verify_class_access_msg(this_klass, k, vca_result); + + // Names are all known to be < 64k so we know this formatted message is not excessively large. if (msg == nullptr) { bool same_module = (this_klass->module() == k->module()); Exceptions::fthrow( @@ -4217,6 +4222,8 @@ static void check_illegal_static_method(const InstanceKlass* this_klass, TRAPS) // if m is static and not the init method, throw a verify error if ((m->is_static()) && (m->name() != vmSymbols::class_initializer_name())) { ResourceMark rm(THREAD); + + // Names are all known to be < 64k so we know this formatted message is not excessively large. Exceptions::fthrow( THREAD_AND_LOCATION, vmSymbols::java_lang_VerifyError(), @@ -4236,6 +4243,7 @@ void ClassFileParser::verify_legal_class_modifiers(jint flags, TRAPS) const { assert(_major_version >= JAVA_9_VERSION || !is_module, "JVM_ACC_MODULE should not be set"); if (is_module) { ResourceMark rm(THREAD); + // Names are all known to be < 64k so we know this formatted message is not excessively large. Exceptions::fthrow( THREAD_AND_LOCATION, vmSymbols::java_lang_NoClassDefFoundError(), @@ -4259,6 +4267,7 @@ void ClassFileParser::verify_legal_class_modifiers(jint flags, TRAPS) const { (is_interface && major_gte_1_5 && (is_super || is_enum)) || (!is_interface && major_gte_1_5 && is_annotation)) { ResourceMark rm(THREAD); + // Names are all known to be < 64k so we know this formatted message is not excessively large. Exceptions::fthrow( THREAD_AND_LOCATION, vmSymbols::java_lang_ClassFormatError(), @@ -4295,6 +4304,7 @@ void ClassFileParser::verify_class_version(u2 major, u2 minor, Symbol* class_nam } if (major > max_version) { + // Names are all known to be < 64k so we know this formatted message is not excessively large. Exceptions::fthrow( THREAD_AND_LOCATION, vmSymbols::java_lang_UnsupportedClassVersionError(), @@ -4310,6 +4320,7 @@ void ClassFileParser::verify_class_version(u2 major, u2 minor, Symbol* class_nam if (minor == JAVA_PREVIEW_MINOR_VERSION) { if (major != max_version) { + // Names are all known to be < 64k so we know this formatted message is not excessively large. Exceptions::fthrow( THREAD_AND_LOCATION, vmSymbols::java_lang_UnsupportedClassVersionError(), @@ -4362,6 +4373,7 @@ void ClassFileParser::verify_legal_field_modifiers(jint flags, if (is_illegal) { ResourceMark rm(THREAD); + // Names are all known to be < 64k so we know this formatted message is not excessively large. Exceptions::fthrow( THREAD_AND_LOCATION, vmSymbols::java_lang_ClassFormatError(), @@ -4445,6 +4457,7 @@ void ClassFileParser::verify_legal_method_modifiers(jint flags, if (is_illegal) { ResourceMark rm(THREAD); + // Names are all known to be < 64k so we know this formatted message is not excessively large. Exceptions::fthrow( THREAD_AND_LOCATION, vmSymbols::java_lang_ClassFormatError(), @@ -4686,6 +4699,7 @@ void ClassFileParser::verify_legal_class_name(const Symbol* name, TRAPS) const { if (!legal) { ResourceMark rm(THREAD); assert(_class_name != nullptr, "invariant"); + // Names are all known to be < 64k so we know this formatted message is not excessively large. Exceptions::fthrow( THREAD_AND_LOCATION, vmSymbols::java_lang_ClassFormatError(), @@ -4719,6 +4733,7 @@ void ClassFileParser::verify_legal_field_name(const Symbol* name, TRAPS) const { if (!legal) { ResourceMark rm(THREAD); assert(_class_name != nullptr, "invariant"); + // Names are all known to be < 64k so we know this formatted message is not excessively large. Exceptions::fthrow( THREAD_AND_LOCATION, vmSymbols::java_lang_ClassFormatError(), @@ -4756,6 +4771,7 @@ void ClassFileParser::verify_legal_method_name(const Symbol* name, TRAPS) const if (!legal) { ResourceMark rm(THREAD); assert(_class_name != nullptr, "invariant"); + // Names are all known to be < 64k so we know this formatted message is not excessively large. Exceptions::fthrow( THREAD_AND_LOCATION, vmSymbols::java_lang_ClassFormatError(), @@ -5527,6 +5543,7 @@ void ClassFileParser::parse_stream(const ClassFileStream* const stream, if (_class_name != class_name_in_cp) { if (_class_name != vmSymbols::unknown_class_name()) { ResourceMark rm(THREAD); + // Names are all known to be < 64k so we know this formatted message is not excessively large. Exceptions::fthrow(THREAD_AND_LOCATION, vmSymbols::java_lang_NoClassDefFoundError(), "%s (wrong name: %s)", diff --git a/src/hotspot/share/interpreter/linkResolver.cpp b/src/hotspot/share/interpreter/linkResolver.cpp index 6300b660092..ca8a85c8bba 100644 --- a/src/hotspot/share/interpreter/linkResolver.cpp +++ b/src/hotspot/share/interpreter/linkResolver.cpp @@ -323,6 +323,9 @@ void LinkResolver::check_klass_accessibility(Klass* ref_klass, Klass* sel_klass, char* msg = Reflection::verify_class_access_msg(ref_klass, InstanceKlass::cast(base_klass), vca_result); + + // Names are all known to be < 64k so we know this formatted message is not excessively large. + bool same_module = (base_klass->module() == ref_klass->module()); if (msg == nullptr) { Exceptions::fthrow( @@ -615,6 +618,7 @@ void LinkResolver::check_method_accessability(Klass* ref_klass, print_nest_host_error_on(&ss, ref_klass, sel_klass); } + // Names are all known to be < 64k so we know this formatted message is not excessively large. Exceptions::fthrow(THREAD_AND_LOCATION, vmSymbols::java_lang_IllegalAccessError(), "%s", @@ -968,6 +972,7 @@ void LinkResolver::check_field_accessability(Klass* ref_klass, if (fd.is_private()) { print_nest_host_error_on(&ss, ref_klass, sel_klass); } + // Names are all known to be < 64k so we know this formatted message is not excessively large. Exceptions::fthrow(THREAD_AND_LOCATION, vmSymbols::java_lang_IllegalAccessError(), "%s", @@ -1187,6 +1192,7 @@ Method* LinkResolver::linktime_resolve_special_method(const LinkInfo& link_info, ss.print(" %s(", resolved_method->name()->as_C_string()); resolved_method->signature()->print_as_signature_external_parameters(&ss); ss.print(")' not found"); + // Names are all known to be < 64k so we know this formatted message is not excessively large. Exceptions::fthrow( THREAD_AND_LOCATION, vmSymbols::java_lang_NoSuchMethodError(), diff --git a/src/hotspot/share/oops/constantPool.cpp b/src/hotspot/share/oops/constantPool.cpp index 015ec32700a..31644f33797 100644 --- a/src/hotspot/share/oops/constantPool.cpp +++ b/src/hotspot/share/oops/constantPool.cpp @@ -1266,6 +1266,7 @@ oop ConstantPool::resolve_constant_at_impl(const constantPoolHandle& this_cp, cp_index, callee->is_interface() ? "CONSTANT_MethodRef" : "CONSTANT_InterfaceMethodRef", callee->is_interface() ? "CONSTANT_InterfaceMethodRef" : "CONSTANT_MethodRef"); + // Names are all known to be < 64k so we know this formatted message is not excessively large. Exceptions::fthrow(THREAD_AND_LOCATION, vmSymbols::java_lang_IncompatibleClassChangeError(), "%s", ss.as_string()); save_and_throw_exception(this_cp, cp_index, tag, CHECK_NULL); } diff --git a/src/hotspot/share/oops/instanceKlass.cpp b/src/hotspot/share/oops/instanceKlass.cpp index d2761667f64..9cf48da91b6 100644 --- a/src/hotspot/share/oops/instanceKlass.cpp +++ b/src/hotspot/share/oops/instanceKlass.cpp @@ -899,6 +899,7 @@ bool InstanceKlass::link_class_impl(TRAPS) { // if we are executing Java code. This is not a problem for CDS dumping phase since // it doesn't execute any Java code. ResourceMark rm(THREAD); + // Names are all known to be < 64k so we know this formatted message is not excessively large. Exceptions::fthrow(THREAD_AND_LOCATION, vmSymbols::java_lang_NoClassDefFoundError(), "Class %s, or one of its supertypes, failed class initialization", @@ -919,6 +920,7 @@ bool InstanceKlass::link_class_impl(TRAPS) { if (super_klass != nullptr) { if (super_klass->is_interface()) { // check if super class is an interface ResourceMark rm(THREAD); + // Names are all known to be < 64k so we know this formatted message is not excessively large. Exceptions::fthrow( THREAD_AND_LOCATION, vmSymbols::java_lang_IncompatibleClassChangeError(), @@ -3286,6 +3288,7 @@ InstanceKlass* InstanceKlass::compute_enclosing_class(bool* inner_is_member, TRA // If the outer class is not an instance klass then it cannot have // declared any inner classes. ResourceMark rm(THREAD); + // Names are all known to be < 64k so we know this formatted message is not excessively large. Exceptions::fthrow( THREAD_AND_LOCATION, vmSymbols::java_lang_IncompatibleClassChangeError(), diff --git a/src/hotspot/share/runtime/reflection.cpp b/src/hotspot/share/runtime/reflection.cpp index 97fc26a599f..7739cff03bf 100644 --- a/src/hotspot/share/runtime/reflection.cpp +++ b/src/hotspot/share/runtime/reflection.cpp @@ -697,6 +697,7 @@ void Reflection::check_for_inner_class(const InstanceKlass* outer, const Instanc // 'inner' not declared as an inner klass in outer ResourceMark rm(THREAD); + // Names are all known to be < 64k so we know this formatted message is not excessively large. Exceptions::fthrow( THREAD_AND_LOCATION, vmSymbols::java_lang_IncompatibleClassChangeError(), diff --git a/src/hotspot/share/services/diagnosticArgument.cpp b/src/hotspot/share/services/diagnosticArgument.cpp index 5fd565a605a..a36b8e853b8 100644 --- a/src/hotspot/share/services/diagnosticArgument.cpp +++ b/src/hotspot/share/services/diagnosticArgument.cpp @@ -115,12 +115,13 @@ template <> void DCmdArgument::parse_value(const char* str, || sscanf(str, JLONG_FORMAT "%n", &_value, &scanned) != 1 || (size_t)scanned != len) { - const int maxprint = 64; Exceptions::fthrow(THREAD_AND_LOCATION, vmSymbols::java_lang_IllegalArgumentException(), - "Integer parsing error in command argument '%s'. Could not parse: %.*s%s.\n", _name, - MIN2((int)len, maxprint), - (str == nullptr ? "" : str), - (len > maxprint ? "..." : "")); + "Integer parsing error in command argument '%.*s'. Could not parse: %.*s%s.\n", + maxprint, + _name, + maxprint > len ? (int)len : maxprint, + (str == nullptr ? "" : str), + (len > maxprint ? "..." : "")); } } @@ -160,7 +161,11 @@ PRAGMA_DIAG_POP buf[len] = '\0'; Exceptions::fthrow(THREAD_AND_LOCATION, vmSymbols::java_lang_IllegalArgumentException(), - "Boolean parsing error in command argument '%s'. Could not parse: %s.\n", _name, buf); + "Boolean parsing error in command argument '%.*s'. Could not parse: %.*s.\n", + maxprint, + _name, + maxprint, + buf); } } } diff --git a/src/hotspot/share/services/diagnosticArgument.hpp b/src/hotspot/share/services/diagnosticArgument.hpp index 1451ea34f86..639330d6a0e 100644 --- a/src/hotspot/share/services/diagnosticArgument.hpp +++ b/src/hotspot/share/services/diagnosticArgument.hpp @@ -62,6 +62,7 @@ public: class GenDCmdArgument : public ResourceObj { protected: + static const int maxprint = 64; GenDCmdArgument* _next; const char* const _name; const char* const _description; diff --git a/src/hotspot/share/utilities/exceptions.cpp b/src/hotspot/share/utilities/exceptions.cpp index 0f87f76a2e8..86961039c7c 100644 --- a/src/hotspot/share/utilities/exceptions.cpp +++ b/src/hotspot/share/utilities/exceptions.cpp @@ -258,6 +258,10 @@ void Exceptions::throw_stack_overflow_exception(JavaThread* THREAD, const char* _throw(THREAD, file, line, exception); } +// All callers are expected to have ensured that the incoming expanded format string +// will be within reasonable limits - specifically we will never hit the INT_MAX limit +// of os::vsnprintf when it tries to report how big a buffer is needed. Even so we +// further limit the formatted output to 1024 characters. void Exceptions::fthrow(JavaThread* thread, const char* file, int line, Symbol* h_name, const char* format, ...) { const int max_msg_size = 1024; va_list ap; @@ -273,6 +277,7 @@ void Exceptions::fthrow(JavaThread* thread, const char* file, int line, Symbol* // have a truncated UTF-8 sequence. Similarly, if the buffer was too small and ret >= max_msg_size // we may also have a truncated UTF-8 sequence. In such cases we need to fix the buffer so the UTF-8 // sequence is valid. + assert(ret != -1, "Caller should have ensured the incoming format string is size limited!"); if (ret == -1 || ret >= max_msg_size) { int len = (int) strlen(msg); if (len > 0) {