Update the error message for invalid class name #8513

ChengJin01 · 2020-02-05T23:27:42Z

The change is to add code so as to improve the readability
of the error message being thrown out when an invalid class
name stored in the constant pool is captured during the bytecode
verification.

Related: #7684

Signed-off-by: Cheng Jin [email protected]

ChengJin01 · 2020-02-05T23:30:42Z

Already verified in the single test (loading the specified class file with a corrupted constant pool) and personal builds.

passed-in the class file with a class name:

 ../jdk/bin/java  -cp  .  GregorSamsa_22613
Error: LinkageError occurred while loading main class GregorSamsa_22613
        java.lang.ClassFormatError: 
The class name (errorNo=#1) in the constant pool entry at index (#122) is invalid; 
utf8name=(Ljava/lang/Object;ILcom/sun/org/apache/xalan/internal/xsltc/DOM;)Ljava/lang/String;, 
class=GregorSamsa_22613, offset=0

loaded in the class file without a class name:
LoadClassTest.java

class FileClassLoader extends ClassLoader {
     public Class findClass(String name) {
                File file = new File(name);
                byte[] classBytes = null;
                try {classBytes = Files.readAllBytes(file.toPath()); } catch(Exception e) {return null;}
        return defineClass(null, classBytes, 0, classBytes.length); <---- passed null for the class name
     }
}

public class LoadClassTest {
    public static void main(String[] args)  {
     FileClassLoader fcl = new FileClassLoader();
     Class<?> clazz = fcl.findClass("GregorSamsa_22613.class");
    }
}

 ../jdk/bin/java  -cp  . LoadClassTest
Exception in thread "main" java.lang.ClassFormatError: 
The class name (errorNo=#1) in the constant pool entry at index (#122) is invalid; 
utf8name=(Ljava/lang/Object;ILcom/sun/org/apache/xalan/internal/xsltc/DOM;)Ljava/lang/String;, 
class=die/verwandlung/GregorSamsa, offset=0
        at java.base/java.lang.ClassLoader.defineClassInternal(ClassLoader.java:476)
        at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:437)
        at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:389)
        at FileClassLoader.findClass(LoadClassTest.java:10)
        at LoadClassTest.main(LoadClassTest.java:17)

ChengJin01 · 2020-02-05T23:34:22Z

Reviewer: @DanHeidinga

DanHeidinga

Overall, I like what you're doing here - making it easier to debug and for users to understand what's wrong.

I'm a little concerned that the implementation is brittle and may be easy to break / crash if the incoming class is malformed in the wrong way.

The class name (errorNo=#1) in the constant pool entry at index (#122) is invalid; 
utf8name=(Ljava/lang/Object;ILcom/sun/org/apache/xalan/internal/xsltc/DOM;)Ljava/lang/String;, 
class=die/verwandlung/GregorSamsa, offset=0

What is errorNo=#1? Is that our internal error code or something from the spec? What other values can it take?

Is constant pool entry at index (#122) an index in the original constantpool or in the ROMClass's cp? If the former, this is good. If the later it won't match what the user sees in the their classfile.

DanHeidinga · 2020-02-06T05:01:33Z

runtime/bcverify/staticverify.c

@@ -1635,9 +1640,12 @@ j9bcv_verifyClassStructure (J9PortLibrary * portLib, J9CfrClassFile * classfile,
 	J9CfrConstantPoolInfo *info;
 	J9CfrConstantPoolInfo *utf8;
 	J9CfrMethod *method;
+	U_32 thisClassIndex = classfile->constantPool[classfile->thisClass].slot1;


Suggested change

U_32 thisClassIndex = classfile->constantPool[classfile->thisClass].slot1;

const U_32 thisClassIndex = classfile->constantPool[classfile->thisClass].slot1;

const was added as suggested above.

DanHeidinga · 2020-02-06T05:03:52Z

runtime/bcverify/staticverify.c

@@ -1894,6 +1908,12 @@ j9bcv_verifyClassStructure (J9PortLibrary * portLib, J9CfrClassFile * classfile,

 	goto _leaveProc;			/* All is well */

+  _classNameError:
+	Trc_STV_j9bcv_verifyClassStructure_BadClassNameError(J9NLS_CFR_ERR_CP_BAD_CLASS_NAME__ID, classNameErrNo, cpIndex, className->slot1, className->bytes);


What if className is never assigned? Won't this crash dereferencing the null pointer?

The className is initialized with NULL and assigned in the case of CFR_CONSTANT_Class in j9bcv_verifyClassStructure.

case CFR_CONSTANT_Class: /* Must be a UTF8. */ utf8 = &classfile->constantPool[info->slot1]; className = utf8;

So It jumps to _classNameError if anything wrong happens to className (assigned by utf8), which means if className is never assigned with utf8, then there is no problem to the class name in the constant pool. As a result, there is no way to reach _classNameError and the tracepoint here.

Considering that any change in the future might unintentionally screws things up by dereferencing a null pointer in the tracepoint, I agree to do the initialization first with J9CfrConstantPoolInfo classNameInfo = {0, 0, 0, 0, 0, NULL, 0} (all code there was updated accordingly), in which case it will end up with an empty class name if the class name is literally NULL in the constant pool (which in theory should never happen / will crash in bcvCheckClassName(utf8) prior to _classNameError .

DanHeidinga · 2020-02-06T05:08:55Z

runtime/nls/cfre/cfrerr.nls

 J9NLS_CFR_ERR_UNKNOWN_CONSTANT.system_action=The JVM will throw a verification or classloading-related exception such as java.lang.ClassFormatError.
 J9NLS_CFR_ERR_UNKNOWN_CONSTANT.user_response=Contact the provider of the classfile for a corrected version.

 # END NON-TRANSLATABLE

-J9NLS_CFR_ERR_BAD_CLASS_NAME=class name is invalid
+#Example: JVMCFRE068 The class name (errorNo=#1) in the constant pool entry at index (#120) is invalid; utf8name=([Ljava/lang/String;)V, class=Foo, offset=0


We don't usually change the NLS messages. Rather we add new ones to the end.

The reason for this is the NLS messages are periodically translated and all the translations need to match, especially with respect to the format strings.

Agreed and moved the NLS messages to the end of the file.

DanHeidinga · 2020-02-06T05:09:34Z

runtime/verutil/cfrerr.c

+			"The class name (errorNo=#%u) in the constant pool entry at index (#%u) is invalid; utf8name=%.*s, class=%.*s, offset=%u");
+
+	allocSize = strlen(template) + utf8NameLength + thisClassNameLength + (MAX_INT_SIZE * 3);
+	errorString = j9mem_allocate_memory(allocSize, OMRMEM_CATEGORY_VM);


Where is this freed?

In our case, the calling path is as follows:

createROMClassFromClassFile (/runtime/bcutil/defineclass.c /calling j9mem_free_memory) |--> buildVerifyErrorString |---> getJ9CfrErrorDetailMessageNoMethod |---> getJ9CfrErrorClassNameMessage (calling j9mem_allocate_memory)

Following the existing implementation in allocating/releasing memory, the memory allocated in getJ9CfrErrorClassNameMessage will eventually be released in createROMClassFromClassFile after use as follows:

createROMClassFromClassFile(...) { ... case BCT_ERR_GENERIC_ERROR_CUSTOM_MSG: { /* default value for exceptionNumber (J9VMCONSTANTPOOL_JAVALANGCLASSFORMATERROR) assigned before switch */ ----> errorUTF = (U_8 *)buildVerifyErrorString(vm, (J9CfrError *)vm->dynamicLoadBuffers->classFileError, className, classNameLength); break; } ... } else { if (errorUTF == NULL) { J9_VM_FUNCTION(currentThread, setCurrentException)(currentThread, exceptionNumber, NULL); } else { PORT_ACCESS_FROM_JAVAVM(vm); J9_VM_FUNCTION(currentThread, setCurrentExceptionUTF)(currentThread, exceptionNumber, (const char*)errorUTF); --------> j9mem_free_memory(errorUTF); } }

The change is to add code so as to improve the readability of the error message being thrown out when an invalid class name stored in the constant pool is captured during the bytecode verification. Related: eclipse-openj9#7684 Signed-off-by: Cheng Jin <[email protected]>

ChengJin01 · 2020-02-07T17:59:32Z

What is errorNo=#1? Is that our internal error code or something from the spec? What other values can it take?

The errorNo is an internal error code added by me for us to quickly identify the issue with the class name stored in the constant pool without checking the dump files.

/runtime/bcverify/staticverify.c

#define CLASS_NAME_ERR_UTF8             1
#define CLASS_NAME_ERR_OBJECT_ARRAY     2
#define CLASS_NAME_ERR_BASE_TYPE_ARRAY  3
#define CLASS_NAME_ERR_DEFAULT          4

		case CFR_CONSTANT_Class:
...
			if (arity < 0) {
				errorType = J9NLS_CFR_ERR_CP_BAD_CLASS_NAME__ID;
  ------> classNameErrNo = CLASS_NAME_ERR_UTF8;
				goto _classNameError;
			}
...
				switch (utf8->bytes[arity]) {
				case 'L':		/* object array */
					if (utf8->bytes[--end] != ';') {
						errorType = J9NLS_CFR_ERR_CP_BAD_CLASS_NAME__ID;
  ------> classNameErrNo = CLASS_NAME_ERR_OBJECT_ARRAY;
						goto _classNameError;
					}
					break;
...
					if (--end != (UDATA) arity) {
						errorType = J9NLS_CFR_ERR_CP_BAD_CLASS_NAME__ID;
  ------> classNameErrNo = CLASS_NAME_ERR_BASE_TYPE_ARRAY;
						goto _classNameError;
					}
					break;
				default:
					errorType = J9NLS_CFR_ERR_CP_BAD_CLASS_NAME__ID;
  ------> classNameErrNo = CLASS_NAME_ERR_DEFAULT;
					goto _classNameError;
				}
			}

			break;

Is constant pool entry at index (#122) an index in the original constantpool or in the ROMClass's cp? If the former, this is good. If the later it won't match what the user sees in the their classfile.

The intention of the PR is to identify the issue with the class name detected in j9bcv_verifyClassStructure during the static verification, in which case the constant pool comes from a class file rather than a ROM class ( is there any class file generated from a ROM class ? )

IDATA 
j9bcv_verifyClassStructure (J9PortLibrary * portLib, 
J9CfrClassFile * classfile, <-----------
U_8 * segment,
)
{... }

ChengJin01 · 2020-02-07T18:08:43Z

I'm a little concerned that the implementation is brittle and may be easy to break / crash if the incoming class is malformed in the wrong way.

I totally understand your concern as this is the first time for verification to capture a corrupted constant pool of a class file. So we need to carefully evaluate all situations to ensure they are safely addressed without any new problem introduced unexpectedly.

genie-openj9 · 2021-05-17T14:19:01Z

Can one of the admins verify this patch?

ChengJin01 force-pushed the bcv_classformaterror_check branch from 5d83232 to de92c1b Compare February 5, 2020 23:33

ChengJin01 requested a review from DanHeidinga February 5, 2020 23:34

DanHeidinga reviewed Feb 6, 2020

View reviewed changes

DanHeidinga added the comp:vm label Feb 6, 2020

ChengJin01 force-pushed the bcv_classformaterror_check branch from de92c1b to 51f5c7b Compare February 7, 2020 17:19

ChengJin01 closed this Mar 17, 2022

ChengJin01 deleted the bcv_classformaterror_check branch March 17, 2022 16:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the error message for invalid class name #8513

Update the error message for invalid class name #8513

ChengJin01 commented Feb 5, 2020 •

edited

Loading

ChengJin01 commented Feb 5, 2020 •

edited

Loading

ChengJin01 commented Feb 5, 2020

DanHeidinga left a comment

DanHeidinga Feb 6, 2020

ChengJin01 Feb 7, 2020

DanHeidinga Feb 6, 2020

ChengJin01 Feb 7, 2020 •

edited

Loading

DanHeidinga Feb 6, 2020

ChengJin01 Feb 7, 2020 •

edited

Loading

DanHeidinga Feb 6, 2020

ChengJin01 Feb 7, 2020 •

edited

Loading

ChengJin01 commented Feb 7, 2020 •

edited

Loading

ChengJin01 commented Feb 7, 2020 •

edited

Loading

genie-openj9 commented May 17, 2021

	U_32 thisClassIndex = classfile->constantPool[classfile->thisClass].slot1;
	const U_32 thisClassIndex = classfile->constantPool[classfile->thisClass].slot1;

Update the error message for invalid class name #8513

Update the error message for invalid class name #8513

Conversation

ChengJin01 commented Feb 5, 2020 • edited Loading

ChengJin01 commented Feb 5, 2020 • edited Loading

ChengJin01 commented Feb 5, 2020

DanHeidinga left a comment

Choose a reason for hiding this comment

DanHeidinga Feb 6, 2020

Choose a reason for hiding this comment

ChengJin01 Feb 7, 2020

Choose a reason for hiding this comment

DanHeidinga Feb 6, 2020

Choose a reason for hiding this comment

ChengJin01 Feb 7, 2020 • edited Loading

Choose a reason for hiding this comment

DanHeidinga Feb 6, 2020

Choose a reason for hiding this comment

ChengJin01 Feb 7, 2020 • edited Loading

Choose a reason for hiding this comment

DanHeidinga Feb 6, 2020

Choose a reason for hiding this comment

ChengJin01 Feb 7, 2020 • edited Loading

Choose a reason for hiding this comment

ChengJin01 commented Feb 7, 2020 • edited Loading

ChengJin01 commented Feb 7, 2020 • edited Loading

genie-openj9 commented May 17, 2021

ChengJin01 commented Feb 5, 2020 •

edited

Loading

ChengJin01 commented Feb 5, 2020 •

edited

Loading

ChengJin01 Feb 7, 2020 •

edited

Loading

ChengJin01 Feb 7, 2020 •

edited

Loading

ChengJin01 Feb 7, 2020 •

edited

Loading

ChengJin01 commented Feb 7, 2020 •

edited

Loading

ChengJin01 commented Feb 7, 2020 •

edited

Loading