Skip to content

Error if invokeinterface receiver is reference array - #19877

Merged
tajila merged 1 commit into
eclipse-openj9:masterfrom
theresa-m:fix_19756
Jul 25, 2024
Merged

Error if invokeinterface receiver is reference array#19877
tajila merged 1 commit into
eclipse-openj9:masterfrom
theresa-m:fix_19756

Conversation

@theresa-m

@theresa-m theresa-m commented Jul 19, 2024

Copy link
Copy Markdown
Contributor

Throw an error during bytecode verification if the receiver of invokeinterface is a reference array.
Fix: #19756

@tajila

tajila commented Jul 22, 2024

Copy link
Copy Markdown
Contributor

Did you run verification tests with this change?

@tajila
tajila requested a review from ChengJin01 July 22, 2024 18:18
ChengJin01
ChengJin01 suggested changes Jul 22, 2024
View reviewed changes
Comment thread runtime/oti/bytecodewalk.h Outdated
#define BCV_TAG_BASE_ARRAY_OR_NULL 2 /* set bit means base type array */
#define BCV_SPECIAL_INIT 4 /* set bit means special init object ("this" for <init>) */
#define BCV_SPECIAL_NEW 8 /* set bit means special new object (PC offset in upper 28 bits) */
#define BCV_OBJECT_OR_ARRAY 0

@ChengJin01 ChengJin01 Jul 22, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would suggest to leave these comments out there to avoid any confusion/misconception in the future given it's clearer for developers to understand how to use these constants with the comments put aside.

@theresa-m theresa-m Jul 23, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay. I was thinking it would be good to have one set of comments, but I don't think there's a risk of this code changing significantly :) I'll add them back.

Comment thread runtime/bcverify/rtverify.c Outdated
@ChengJin01

ChengJin01 commented Jul 22, 2024 •
edited
Loading

Copy link
Copy Markdown

Did you run verification tests with this change?

@theresa-m, you will need to run internal & open builds to ensure there is no issue introduced with your changes. In addition, please share with us the output of OpenJ9 with the updated fix as compared to RI.

@theresa-m
theresa-m force-pushed the fix_19756 branch 2 times, most recently from 1c349ba to 0893ddc Compare July 23, 2024 15:47
@theresa-m

theresa-m commented Jul 23, 2024

Copy link
Copy Markdown
Contributor Author

The output of OpenJ9 for the test provided in the issue is:

Error: Unable to initialize main class TestInterface
Caused by: java.lang.VerifyError: JVMVRFY021 thrown object not throwable; class=TestInterface, method=method1()V, pc=7
Exception Details:
  Location:
    TestInterface.method1()V @7: JBinvokeinterface
  Reason:
    Type '[LTestInterface$A;' (current frame, stack[0]) is not assignable to 'java/lang/Object'
  Current Frame:
    bci: @7
    flags: { }
    locals: { '[LTestInterface$A;' }
    stack: { '[LTestInterface$A;' }

@ChengJin01

ChengJin01 commented Jul 23, 2024

Copy link
Copy Markdown

I will approve the PR once it passes all related internal & open test cases.

@theresa-m

theresa-m commented Jul 24, 2024

Copy link
Copy Markdown
Contributor Author

I updated this change after further testing to make sure the type is not BCV_BASE_TYPE_NULL since BCV_BASE_TYPE_NULL has non-zero arity bits.

@ChengJin01

ChengJin01 commented Jul 24, 2024

Copy link
Copy Markdown

I updated this change after further testing to make sure the type is not BCV_BASE_TYPE_NULL since BCV_BASE_TYPE_NULL has non-zero arity bits.

That's correct as a NULL value belongs to reference as per the VM Spec.

ChengJin01
ChengJin01 suggested changes Jul 24, 2024
View reviewed changes
Comment thread runtime/bcverify/rtverify.c Outdated
@theresa-m
Error if invokeinterface receiver is reference array
eac4b61 
Throw an error during bytecode verification if the receiver
of invokeinterface is a reference array

Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>
ChengJin01
ChengJin01 approved these changes Jul 24, 2024
View reviewed changes

@ChengJin01 ChengJin01 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM.

@tajila

tajila commented Jul 24, 2024

Copy link
Copy Markdown
Contributor

jenkins test sanity alinux64 jdk8,jdk21

@tajila
tajila merged commit 876f013 into eclipse-openj9:master Jul 25, 2024
@tajila

tajila commented Jul 25, 2024

Copy link
Copy Markdown
Contributor

@theresa-m Please make a 0.47 PR as well

@theresa-m theresa-m mentioned this pull request Jul 30, 2024
Merged
@theresa-m

theresa-m commented Jul 30, 2024

Copy link
Copy Markdown
Contributor Author

#19931 is for 0.47

@theresa-m
theresa-m deleted the fix_19756 branch October 11, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tajila tajila tajila approved these changes
+1 more reviewer
@ChengJin01 ChengJin01 ChengJin01 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

comp:vm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

J9 verification permits non-interfaces on operand stack of invokeinterface

3 participants

@theresa-m @tajila @ChengJin01