[WIP] ConstantDynamic: Adds static and dynamic verification for ConDy #1631

taliamccormick · 2018-04-06T19:54:21Z

WIP PR for ConDy Verification

This will add static/runtime verification for ConstantDynamic. It checks that:

ConDy uses occur in classfiles with version Java11 and up
Validation that ldc and ldc_w bytecodes receive ConstantDynamic entries which do not return a long (J) or double (D)
Validation that ldc2_w bytecodes receive ConstantDynamic entries which return a long or double

Resolves #1278
Depends on issue #1271 with corresponding PRs #1426 and https://github.com/eclipse/openj9/pull/1533/files

@fengxue-IS

fengxue-IS

great start, btw copyright dates

fengxue-IS · 2018-04-06T20:29:31Z

runtime/bcverify/staticverify.c

+						  && (((flags & BCT_MajorClassFileVersionMask) >= BCT_Java11MajorVersionShifted)
+									|| ((flags & BCT_MajorClassFileVersionMask) == 0))
+						  && ('D' != constantDynamicNameAndSignature->bytes[constantDynamicNameAndSignature->slot1 - 1])
+						  && ('J' != constantDynamicNameAndSignature->bytes[constantDynamicNameAndSignature->slot1 - 1]))


I think we should use a more informative error message, not just ldc not constant if the return type is wrong

Agreed, updating

fengxue-IS · 2018-04-06T20:30:41Z

runtime/bcverify/staticverify.c

+									|| ((flags & BCT_MajorClassFileVersionMask) == 0))
+						  && ('D' != constantDynamicNameAndSignature->bytes[constantDynamicNameAndSignature->slot1 - 1])
+						  && ('J' != constantDynamicNameAndSignature->bytes[constantDynamicNameAndSignature->slot1 - 1]))
+			)) {
 				errorType = J9NLS_CFR_ERR_BC_LDC_NOT_CONSTANT__ID;


same with error message

fengxue-IS · 2018-04-06T20:33:25Z

runtime/oti/j9nonbuilder.h

@@ -2133,6 +2133,8 @@ typedef struct J9ConstantPool {
 #define J9CPTYPE_INTERFACE_STATIC_METHOD 16
 #define J9CPTYPE_INTERFACE_INSTANCE_METHOD 17 

+#define J9CPTYPE_CONSTANT_DYNAMIC 18


this is already defined in #1426, forgot to tell you, sry

fengxue-IS · 2018-04-06T20:35:20Z

runtime/oti/bcverify_api.h

@@ -452,7 +452,7 @@ pushFieldType(J9BytecodeVerificationData *verifyData, J9UTF8 * utf8string, UDATA
 * @return UDATA *
 */
 UDATA * 
-pushLdcType(J9ROMClass * romClass, UDATA index, UDATA * stackTop);
+pushLdcType(J9BytecodeVerificationData *verifyData, J9ROMClass * romClass, UDATA index, UDATA * stackTop);


can you update the description comments?

fengxue-IS · 2018-04-06T21:04:38Z

runtime/bcverify/vrfyhelp.c

+			U_8* signature = J9UTF8_DATA(nameAndSignature);
+
+			/* The return value of a method follows the first ')' listed in its signature */
+			while (*signature++ != ')');


read the condy spec 4.4.13, it seem to suggest that the signature referenced in a ConstantDynamic entry is a fieldDescriptor, so there will be no () in the string, it should only contain the return type.

I'll read the spec details to confirm this, may need to add more checks in static verify to consider cases such as double array signature [D

Right, thanks. Looks like I could directly give the value of J9UTF8_DATA(nameAndSignature) then, and pushType will still handle the rest.

What situations istatic verify would/would not accept cases such as the double array signature [[D? From what I understand:

Reference types are any non-primitive (eg. objects, arrays)

Constant dynamic does not place any restrictions on its return value (4.10.1.3)

dynamic(ConstantName, FieldDescriptor) for a CONSTANT_Dynamic_info, where ConstantName and FieldDescriptor correspond to the name and field descriptor referenced by the name_and_type_index item of the CONSTANT_Dynamic_info structure (the bootstrap_method_attr_index item is irrelevant to verification);

&

loadableConstantType(CP, Type) :-
CP = dynamic(ConstantName, FieldDescriptor),
parseFieldDescriptor(FieldDescriptor, Type).

ldc & ldc_w bytecodes would still accept the reference types (including double array) and primitives which are not long/double

ldc2_w bytecode would still only accept long/double - but not rerfences, such as [[D

Quick reread of the section you pointed out gives:

4.4.13 gives:

4.4.13 The CONSTANT_Dynamic_info Structure
CONSTANT_Dynamic_info {
    u1 tag;
   u2 bootstrap_method_attr_index;
   u2 name_and_type_index;
}
...
The value of the name_and_type_index item must be a valid index into the constant_pool table. The constant_pool entry at that index must be a CONSTANT_NameAndType_info structure (4.4.6) representing a name and field descriptor (4.3.2).

4.4.6 gives:

CONSTANT_NameAndType_info {
    u1 tag;
    u2 name_index;
    u2 descriptor_index;
}
...
The value of the descriptor_index item must be a valid index into the constant_pool table. The constant_pool entry at that index must be a CONSTANT_Utf8_info structure (§4.4.7) representing a valid field descriptor or method descriptor (§4.3.2, §4.3.3).

4.3.2 gives the grammar for a field descriptor as:

FieldDescriptor:
   FieldType
FieldType:
   BaseType | ObjectType | ArrayType
BaseType:
    (one of) B C D F I J S Z
ObjectType:
   L ClassName ;
ArrayType:
   [ ComponentType
ComponentType:
   FieldType
(http://cr.openjdk.java.net/~dlsmith/constant-dynamic.html)

Woops, you're right - sounds like I can put directly into pushType

I'm not seeing any restrictions on the type of the signature - references (including cases like [[D) are valid to be passed to ldc or ldc_w

your current check is checking at the last char of the fieldDescriptor, which would mix up [[D and D we would have to either check the first char to be D or J or add check for size.
the reverse would apply to ldc and ldc_w

Yes, but if it's a a fieldDescriptor, then can't we directly check the first index instead?
ie. it's either a BaseType, ObjectType, or ArrayType
If a BaseType, then one of B C D F I J S Z will be the first char; if an ObjectType, then L will be the first char; and if ArrayType, then [ will be the first char

fengxue-IS · 2018-04-07T03:54:44Z

runtime/bcverify/staticverify.c

@@ -217,6 +217,8 @@ checkBytecodeStructure (J9CfrClassFile * classfile, UDATA methodIndex, UDATA len

 		case CFR_BC_ldc:
 		case CFR_BC_ldc_w:
+			J9CfrConstantPoolInfo *constantDynamicNameAndSignature = NULL;


nit pick, maybe constantDynamicSignature would be more reflective?

fengxue-IS · 2018-04-07T04:03:18Z

runtime/bcverify/staticverify.c

@@ -231,6 +233,10 @@ checkBytecodeStructure (J9CfrClassFile * classfile, UDATA methodIndex, UDATA len
 			info = &(classfile->constantPool[index]);
 			tag = (UDATA) info->tag;

+			if (CFR_CONSTANT_Dynamic == tag) {
+				constantDynamicNameAndSignature = &classFile->constantPool[classFile->constantPool[info].slot2];


&classFile->constantPool[classFile->constantPool[info->slot2].slot2] missing ->slot2

fengxue-IS · 2018-04-07T04:07:49Z

runtime/bcverify/staticverify.c

+						  && (((flags & BCT_MajorClassFileVersionMask) >= BCT_Java11MajorVersionShifted)
+									|| ((flags & BCT_MajorClassFileVersionMask) == 0))
+						  && ('D' != constantDynamicNameAndSignature->bytes[constantDynamicNameAndSignature->slot1 - 1])
+						  && ('J' != constantDynamicNameAndSignature->bytes[constantDynamicNameAndSignature->slot1 - 1]))


should be 'D' == & 'J' ==

- Copyright dates - Refactor ldc for more informative error messages - Remove definition of 'J9CPTYPE_CONSTANT_DYNAMIC ' value (defined in other PR) - Update description comments on pushLdcType(...) - Update pushLdcType to expect a constant dynamic to provide a field descriptor instead of a method signature - Rename variable constantDynamicNameAndSignature to constantDynamicSignature Signed-off-by: Talia McCormick <[email protected]>

fengxue-IS · 2018-04-09T16:21:25Z

runtime/oti/j9nonbuilder.h

@@ -2239,6 +2241,8 @@ typedef struct J9ROMStringRef {

 #define J9ROMSTRINGREF_UTF8DATA(base) NNSRP_GET((base)->utf8Data, struct J9UTF8*)

+#define J9ROMCONSTANTDYNAMICREF_NAMEANDSIGNATURE(base) NNSRP_GET((base)->nameAndSignature, struct J9UTF8*)


apology for the error in my PR, this should be
#define J9ROMCONSTANTDYNAMICREF_NAMEANDSIGNATURE(base) NNSRP_GET((base)->nameAndSignature, struct J9ROMNameAndSignature*)

and you would access the field descriptor with
J9UTF8 * signature = J9ROMNAMEANDSIGNATURE_SIGNATURE( J9ROMCONSTANTDYNAMICREF_NAMEANDSIGNATURE(J9ROMConstantDynamicRef) )

- Refactor ldc error codes - Correct signature-check - Update J9ROMCONSTANTDYNAMICREF_NAMEANDSIGNATURE macro Signed-off-by: Talia McCormick <[email protected]>

taliamccormick · 2018-04-09T19:56:15Z

Updated

    - Copyright dates
    - Refactor ldc for more informative error messages
    - Remove definition of 'J9CPTYPE_CONSTANT_DYNAMIC ' value (defined in
    other PR)
    - Update description comments on pushLdcType(...)
    - Update pushLdcType to expect a constant dynamic to provide a field
    descriptor instead of a method signature
    - Rename variable constantDynamicNameAndSignature to
    constantDynamicSignature

and

    - Refactor ldc error codes
    - Correct signature-check
    - Update J9ROMCONSTANTDYNAMICREF_NAMEANDSIGNATURE macro

Edit: will handle copyrights when squashing commits.

fengxue-IS · 2018-04-10T16:20:08Z

runtime/oti/j9nonbuilder.h

@@ -2244,8 +2244,7 @@ typedef struct J9ROMFieldRef {
 	J9SRP nameAndSignature;
 } J9ROMFieldRef;

-#define J9ROMFIELDREF_NAMEANDSIGNATURE(base) NNSRP_GET((base)->nameAndSignature, struct J9ROMNameAndSignature*)


removed by accident?

Woops, yes - thanks

taliamccormick · 2018-05-12T00:06:26Z

Closing since it is/will be replaced by #1858

- Add validation to ldc ldc_w and ldc2_w for ConstantDynamic entry - Check that the Condy return type matches the bytecode - Refactor ldc with more informative error messages - Update description comments on pushLdcType(...) - Update pushLdcType to expect a constant dynamic to provide a field - Refactor ldc error codes Code by Talia McCormick from eclipse-openj9#1631 Signed-off-by: Jack Lu <[email protected]>

[WIP] Adds static and dynamic verification for ConDy

77ef700

taliamccormick mentioned this pull request Apr 6, 2018

ConstantDynamic: Verifier checks for Constant dynamic #1278

Closed

fengxue-IS suggested changes Apr 6, 2018

View reviewed changes

fengxue-IS suggested changes Apr 7, 2018

View reviewed changes

fengxue-IS reviewed Apr 9, 2018

View reviewed changes

[WIP] Adds secondary changes to verification WIP

225a3a7

- Refactor ldc error codes - Correct signature-check - Update J9ROMCONSTANTDYNAMICREF_NAMEANDSIGNATURE macro Signed-off-by: Talia McCormick <[email protected]>

fengxue-IS reviewed Apr 10, 2018

View reviewed changes

DanHeidinga added comp:vm jdk11 labels Apr 12, 2018

fengxue-IS mentioned this pull request May 4, 2018

Adds static and dynamic verification for ConstantDynamic Entry #1858

Merged

taliamccormick closed this May 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] ConstantDynamic: Adds static and dynamic verification for ConDy #1631

[WIP] ConstantDynamic: Adds static and dynamic verification for ConDy #1631

taliamccormick commented Apr 6, 2018

fengxue-IS left a comment •

edited

Loading

fengxue-IS Apr 6, 2018

taliamccormick Apr 9, 2018

fengxue-IS Apr 6, 2018

fengxue-IS Apr 6, 2018

fengxue-IS Apr 6, 2018

fengxue-IS Apr 6, 2018

taliamccormick Apr 7, 2018

taliamccormick Apr 7, 2018

fengxue-IS Apr 7, 2018 •

edited

Loading

taliamccormick Apr 9, 2018

fengxue-IS Apr 7, 2018

fengxue-IS Apr 7, 2018

fengxue-IS Apr 7, 2018

fengxue-IS Apr 9, 2018 •

edited

Loading

taliamccormick commented Apr 9, 2018 •

edited

Loading

fengxue-IS Apr 10, 2018

taliamccormick Apr 10, 2018

taliamccormick commented May 12, 2018

		@@ -2239,6 +2241,8 @@ typedef struct J9ROMStringRef {

		#define J9ROMSTRINGREF_UTF8DATA(base) NNSRP_GET((base)->utf8Data, struct J9UTF8*)

		#define J9ROMCONSTANTDYNAMICREF_NAMEANDSIGNATURE(base) NNSRP_GET((base)->nameAndSignature, struct J9UTF8*)

[WIP] ConstantDynamic: Adds static and dynamic verification for ConDy #1631

[WIP] ConstantDynamic: Adds static and dynamic verification for ConDy #1631

Conversation

taliamccormick commented Apr 6, 2018

fengxue-IS left a comment • edited 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

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

fengxue-IS Apr 7, 2018 • edited 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

Choose a reason for hiding this comment

fengxue-IS Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

taliamccormick commented Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taliamccormick commented May 12, 2018

fengxue-IS left a comment •

edited

Loading

fengxue-IS Apr 7, 2018 •

edited

Loading

fengxue-IS Apr 9, 2018 •

edited

Loading

taliamccormick commented Apr 9, 2018 •

edited

Loading