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.

Sign up for GitHub

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

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

Closed
wants to merge 3 commits into from
+127 −29
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
77ef700
[WIP] Adds static and dynamic verification for ConDy
Apr 2, 2018
8bc6a12
[WIP] Adds initial changes to verification WIP
Apr 9, 2018
225a3a7
[WIP] Adds secondary changes to verification WIP
Apr 9, 2018
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
2 changes: 1 addition & 1 deletion runtime/bcverify/bcverify.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1510,7 +1510,7 @@ simulateStack (J9BytecodeVerificationData * verifyData)
} else {
index = PARAM_16(bcIndex, 1);
}
stackTop = pushLdcType(romClass, index, stackTop);
stackTop = pushLdcType(verifyData, romClass, index, stackTop);
break;

/* Change lookup table to generate constant of correct type */
Expand Down
2 changes: 1 addition & 1 deletion runtime/bcverify/rtverify.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -705,7 +705,7 @@ verifyBytecodes (J9BytecodeVerificationData * verifyData)
} else {
index = PARAM_16(bcIndex, 1);
}
stackTop = pushLdcType(romClass, index, stackTop);
stackTop = pushLdcType(verifyData, romClass, index, stackTop);
break;

/* Change lookup table to generate constant of correct type */
Expand Down
28 changes: 26 additions & 2 deletions runtime/bcverify/staticverify.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,8 @@ checkBytecodeStructure (J9CfrClassFile * classfile, UDATA methodIndex, UDATA len

case CFR_BC_ldc:
case CFR_BC_ldc_w:
J9CfrConstantPoolInfo *constantDynamicNameAndSignature = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit pick, maybe constantDynamicSignature would be more reflective?


if (bc == CFR_BC_ldc) {
NEXT_U8(index, bcIndex);
} else {
Expand All @@ -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];
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

if (!((tag == CFR_CONSTANT_Integer)
|| (tag == CFR_CONSTANT_Float)
|| (tag == CFR_CONSTANT_String)
Expand All @@ -239,7 +245,13 @@ checkBytecodeStructure (J9CfrClassFile * classfile, UDATA methodIndex, UDATA len
|| ((flags & BCT_MajorClassFileVersionMask) == 0)))
|| (((tag == CFR_CONSTANT_MethodType) || (tag == CFR_CONSTANT_MethodHandle))
&& (((flags & BCT_MajorClassFileVersionMask) >= BCT_Java7MajorVersionShifted)
|| ((flags & BCT_MajorClassFileVersionMask) == 0))) )) {
|| ((flags & BCT_MajorClassFileVersionMask) == 0)))
|| ((tag == CFR_CONSTANT_Dynamic)
&& (((flags & BCT_MajorClassFileVersionMask) >= BCT_Java11MajorVersionShifted)
|| ((flags & BCT_MajorClassFileVersionMask) == 0))
&& ('D' != constantDynamicNameAndSignature->bytes[constantDynamicNameAndSignature->slot1 - 1])
&& ('J' != constantDynamicNameAndSignature->bytes[constantDynamicNameAndSignature->slot1 - 1]))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, updating

)) {
errorType = J9NLS_CFR_ERR_BC_LDC_NOT_CONSTANT__ID;
/* Jazz 82615: Set the constant pool index to show up in the error message framework */
errorDataIndex = index;
Expand All @@ -248,6 +260,8 @@ checkBytecodeStructure (J9CfrClassFile * classfile, UDATA methodIndex, UDATA len
break;

case CFR_BC_ldc2_w:
J9CfrConstantPoolInfo *constantDynamicNameAndSignature = NULL;

NEXT_U16(index, bcIndex);
if ((!index) || (index >= cpCount - 1)) {
errorType = J9NLS_CFR_ERR_BAD_INDEX__ID;
Expand All @@ -257,8 +271,18 @@ 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];
}
if (!((tag == CFR_CONSTANT_Double)
|| (tag == CFR_CONSTANT_Long))) {
|| (tag == CFR_CONSTANT_Long)
|| ((tag == CFR_CONSTANT_Dynamic)
&& (((flags & BCT_MajorClassFileVersionMask) >= BCT_Java11MajorVersionShifted)
|| ((flags & BCT_MajorClassFileVersionMask) == 0))
&& ('D' != constantDynamicNameAndSignature->bytes[constantDynamicNameAndSignature->slot1 - 1])
&& ('J' != constantDynamicNameAndSignature->bytes[constantDynamicNameAndSignature->slot1 - 1]))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

)) {
errorType = J9NLS_CFR_ERR_BC_LDC_NOT_CONSTANT__ID;
Copy link
Contributor

Choose a reason for hiding this comment

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

same with error message

/* Jazz 82615: Set the constant pool index to show up in the error message framework */
errorDataIndex = index;
Expand Down
11 changes: 10 additions & 1 deletion runtime/bcverify/vrfyhelp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,7 @@ initializeClassNameList(J9BytecodeVerificationData *verifyData)


UDATA *
pushLdcType(J9ROMClass * romClass, UDATA index, UDATA * stackTop)
pushLdcType(J9BytecodeVerificationData *verifyData, J9ROMClass * romClass, UDATA index, UDATA * stackTop)
{
switch(J9_CP_TYPE(J9ROMCLASS_CPSHAPEDESCRIPTION(romClass), index)) {
case J9CPTYPE_CLASS:
Expand All @@ -417,6 +417,15 @@ pushLdcType(J9ROMClass * romClass, UDATA index, UDATA * stackTop)
case J9CPTYPE_METHODHANDLE:
PUSH(BCV_JAVA_LANG_INVOKE_METHODHANDLE_INDEX << BCV_CLASS_INDEX_SHIFT);
break;
case J9CPTYPE_CONSTANT_DYNAMIC:
J9ROMConstantDynamicRef* romConstantDynamicRef = (J9ROMConstantDynamicRef *)(J9_ROM_CP_FROM_ROM_CLASS(romClass) + index);
J9UTF8 *nameAndSignature = J9ROMCONSTANTDYNAMICREF_NAMEANDSIGNATURE(romConstantDynamicRef);
U_8* signature = J9UTF8_DATA(nameAndSignature);

/* The return value of a method follows the first ')' listed in its signature */
while (*signature++ != ')');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@fengxue-IS fengxue-IS Apr 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

pushType(verifyData, signature, stackTop);
break;
}

return stackTop;
Expand Down
2 changes: 1 addition & 1 deletion runtime/oti/bcverify_api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

can you update the description comments?



/**
Expand Down
4 changes: 4 additions & 0 deletions runtime/oti/j9nonbuilder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2133,6 +2133,8 @@ typedef struct J9ConstantPool {
#define J9CPTYPE_INTERFACE_STATIC_METHOD 16
#define J9CPTYPE_INTERFACE_INSTANCE_METHOD 17

#define J9CPTYPE_CONSTANT_DYNAMIC 18
Copy link
Contributor

Choose a reason for hiding this comment

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

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


#define J9_CP_BITS_PER_DESCRIPTION 8
#define J9_CP_DESCRIPTIONS_PER_U32 4
#define J9_CP_DESCRIPTION_MASK 255
Expand Down Expand Up @@ -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*)
Copy link
Contributor

@fengxue-IS fengxue-IS Apr 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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) )


typedef struct J9ROMFieldRef {
U_32 classRefCPIndex;
J9SRP nameAndSignature;
Expand Down