Reject illegal entries in the bootstrap argument array #2700

ChengJin01 · 2018-08-24T17:16:49Z

The changes are to validate the entries stored in
the bootstrap argument array to ensure they are
valid constant pool index required in the JVM Spec.

Signed-off-by: CHENGJin [email protected]

ChengJin01 · 2018-08-24T17:17:09Z

Code Reviewer: @DanHeidinga

runtime/bcutil/cfreader.c

DanHeidinga · 2018-08-28T01:51:32Z

runtime/bcutil/cfreader.c

+						 * CONSTANT_Double_info, CONSTANT_MethodHandle_info, or CONSTANT_MethodType_info structure.
+						 */
+						cpValueTag = cpBase[value].tag;
+						if ((CFR_CONSTANT_String != cpValueTag)


Would this be clearer as a switch statement?

Already updated against suggestion.

DanHeidinga · 2018-08-28T01:51:57Z

runtime/bcutil/cfreader.c

+						&& (CFR_CONSTANT_Float != cpValueTag)
+						&& (CFR_CONSTANT_Double != cpValueTag)
+						&& (CFR_CONSTANT_MethodHandle != cpValueTag)
+						&& (CFR_CONSTANT_MethodType != cpValueTag)


Also, for JDK11, what about Constant_Dynamic? It should be allowed as a BSM arg as well

Already double-checked the Spec at http://openjdk.java.net/jeps/309 and added Constant_Dynamic as valid structure.

DanHeidinga · 2018-08-28T01:56:07Z

runtime/nls/cfre/cfrerr.nls

@@ -1362,3 +1362,11 @@ J9NLS_CFR_ERR_CP_ENTRY_INVALID_BEFORE_V55.explanation=Please consult the Java Vi
 J9NLS_CFR_ERR_CP_ENTRY_INVALID_BEFORE_V55.system_action=The JVM will throw a verification or classloading-related exception such as java.lang.ClassFormatError.
 J9NLS_CFR_ERR_CP_ENTRY_INVALID_BEFORE_V55.user_response=Contact the provider of the classfile for a corrected version.
 # END NON-TRANSLATABLE
+
+J9NLS_CFR_ERR_BAD_BOOTSTRAP_ARGUMENT_ENTRY=The bootstrap argument array contains illegal indices to the constant pool


Can the message be updated to something like:
BootstrapMethod (%d) arguments contain invalid constantpool entry at index (%d) of type (%d)

So that it contains the BSM index, the cp entry and the cp entries type? It makes it much easier for users to understand what went wrong.

I originally introduced 2 parameters (bsmMethodIndex and bsmArgumentIndex) for this message but didn't move forward because the modification of the existing method buildError() incurs a bunch of unnecessary updating at all places.

From the user perspective, I am customizing a new method buildBootstrapMethodError() (still in coding) which only addresses this issue. Will see whether it works good in this case.

Got a crash with my new changes. Still investigating to see what happens to my code.

Already fixed the bug in the new changes.

ChengJin01 · 2018-08-30T15:26:24Z

Already finished the new changes and it works good as expected:

java.lang.ClassFormatError: JVMCFRE154 BootstrapMethod (0) arguments contain invalid constantpool entry at index (#20) of type (1); class=Foo, offset=492

DanHeidinga · 2018-09-11T02:51:06Z

runtime/verutil/cfrerr.c

+	/* J9NLS_CFR_ERR_BAD_BOOTSTRAP_ARGUMENT_ENTRY=BootstrapMethod (%1$d) arguments contain invalid constantpool entry at index (#%2$u) of type (%3$u); class=%5$.*4$s, offset=%6$u */
+	template = j9nls_lookup_message(J9NLS_ERROR | J9NLS_DO_NOT_APPEND_NEWLINE, J9NLS_CFR_ERR_BAD_BOOTSTRAP_ARGUMENT_ENTRY, "(%d)(#%u)(%u);%.*s,%u");
+
+	allocSize = strlen(template) + classNameLength + MAX_INT_SIZE * 4;


Please add brackets to the MAX_INT_SIZE * 4 so it's immediately obvious that you intended to multiple the max int size by 4, and not the entire amount

Already fixed as suggested.

DanHeidinga · 2018-09-11T02:51:52Z

runtime/verutil/cfrerr.c

+	PORT_ACCESS_FROM_PORT(portLib);
+	UDATA allocSize = 0;
+	char *errorString = NULL;
+	const char *template;


Either assign j9nls_lookup_message()'s return value to this variable directly or initialize it to null

Already fixed as suggested.

runtime/bcutil/cfreader.c

DanHeidinga · 2018-09-11T17:45:57Z

Jenkins test sanity xlinux jdk8,jdk11

DanHeidinga · 2018-09-11T18:08:28Z

@ChengJin01 the build is failing - can you take a look?

13:49:25 cfrerr.c: In function ‘getJ9CfrErrorDetailMessageNoMethod’:
13:49:25 cfrerr.c:87:15: error: assignment discards ‘const’ qualifier from pointer target type [-Werror]
13:49:25    errorString = getJ9CfrErrorBsmMessage(portLib, error, className, classNameLength);
13:49:25                ^
13:49:25 cfrerr.c:90:15: error: assignment discards ‘const’ qualifier from pointer target type [-Werror]
13:49:25    errorString = getJ9CfrErrorNormalMessage(portLib, error, className, classNameLength);

ChengJin01 · 2018-09-11T18:28:56Z

Will investigate to see what happened in there.

The changes are to validate the entries stored in the bootstrap argument array to ensure they are valid constant pool index required in the JVM Spec. Signed-off-by: CHENGJin <[email protected]>

ChengJin01 · 2018-09-11T19:02:39Z

Jenkins test sanity xlinux jdk8,jdk11

DanHeidinga · 2018-09-11T19:16:16Z

Jenkins test sanity xlinux jdk8,jdk11

DanHeidinga · 2018-09-13T15:09:57Z

I'm OK with merging this now but would like to see some tests added for this. In particular, to validate that the error cases print reasonable messages and don't crash :)

Opened #2856 for adding tests.

DanHeidinga added the comp:vm label Aug 28, 2018

DanHeidinga reviewed Aug 28, 2018

View reviewed changes

ChengJin01 force-pushed the bcv_classformat_boostrap_args branch 2 times, most recently from 898c8e4 to 8e5c77e Compare August 30, 2018 15:24

ChengJin01 force-pushed the bcv_classformat_boostrap_args branch from 8e5c77e to 1bd2033 Compare August 30, 2018 16:33

DanHeidinga reviewed Sep 11, 2018

View reviewed changes

DanHeidinga self-assigned this Sep 11, 2018

ChengJin01 force-pushed the bcv_classformat_boostrap_args branch from 1bd2033 to 7572bf4 Compare September 11, 2018 03:31

DanHeidinga approved these changes Sep 11, 2018

View reviewed changes

runtime/bcutil/cfreader.c Show resolved Hide resolved

Reject illegal entries in the bootstrap argument array

33096e2

The changes are to validate the entries stored in the bootstrap argument array to ensure they are valid constant pool index required in the JVM Spec. Signed-off-by: CHENGJin <[email protected]>

ChengJin01 force-pushed the bcv_classformat_boostrap_args branch from 7572bf4 to 33096e2 Compare September 11, 2018 18:41

DanHeidinga mentioned this pull request Sep 13, 2018

Add tests for "Reject illegal entries in the bootstrap argument array" #2856

Open

DanHeidinga merged commit ba68bbb into eclipse-openj9:master Sep 13, 2018

ChengJin01 deleted the bcv_classformat_boostrap_args branch June 23, 2020 23:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reject illegal entries in the bootstrap argument array #2700

Reject illegal entries in the bootstrap argument array #2700

ChengJin01 commented Aug 24, 2018

ChengJin01 commented Aug 24, 2018

DanHeidinga Aug 28, 2018

ChengJin01 Aug 28, 2018

DanHeidinga Aug 28, 2018

ChengJin01 Aug 28, 2018

DanHeidinga Aug 28, 2018

ChengJin01 Aug 28, 2018

ChengJin01 Aug 29, 2018

ChengJin01 Aug 30, 2018

ChengJin01 commented Aug 30, 2018 •

edited by DanHeidinga

Loading

DanHeidinga Sep 11, 2018

ChengJin01 Sep 11, 2018

DanHeidinga Sep 11, 2018

ChengJin01 Sep 11, 2018

DanHeidinga commented Sep 11, 2018

DanHeidinga commented Sep 11, 2018

ChengJin01 commented Sep 11, 2018

ChengJin01 commented Sep 11, 2018

DanHeidinga commented Sep 11, 2018

DanHeidinga commented Sep 13, 2018

Reject illegal entries in the bootstrap argument array #2700

Reject illegal entries in the bootstrap argument array #2700

Conversation

ChengJin01 commented Aug 24, 2018

ChengJin01 commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChengJin01 commented Aug 30, 2018 • edited by DanHeidinga Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanHeidinga commented Sep 11, 2018

DanHeidinga commented Sep 11, 2018

ChengJin01 commented Sep 11, 2018

ChengJin01 commented Sep 11, 2018

DanHeidinga commented Sep 11, 2018

DanHeidinga commented Sep 13, 2018

ChengJin01 commented Aug 30, 2018 •

edited by DanHeidinga

Loading