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.

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

Fix erroneously large section size in ROM walk #18585

Merged
merged 1 commit into from
Jan 10, 2024

Conversation

cjjdespres
Copy link
Contributor

If a ROM class has no methods, the CP NAS section should have size zero. Before, the section size would be calculated unconditionally based on the location of the J9ROMCLASS_ROMMETHODS of the ROM class. When the romMethods field is a null SRP, that would result in a negative pointer difference that would then wrap to a very large U_32 value.

Signed-off-by: Christian Despres

If a ROM class has no methods, the CP NAS section should have size zero.
Before, the section size would be calculated unconditionally based on
the location of the J9ROMCLASS_ROMMETHODS of the ROM class. When the
romMethods field is a null SRP, that would result in a negative pointer
difference that would then wrap to a very large U_32 value.

Signed-off-by: Christian Despres
@cjjdespres
Copy link
Contributor Author

Attn @mpirvu. I'm reasonably sure, but not totally confident, that the CP NAS section should have size zero when there are no ROM methods in a ROM class.

count = (U_32)(((UDATA)firstMethod - (UDATA)srpCursor) / (sizeof(J9SRP) * 2));
} else {
count = 0;
}
rangeValid = callbacks->validateRangeCallback(romClass, srpCursor, count * sizeof(J9SRP) * 2, userData);
Copy link
Contributor Author

@cjjdespres cjjdespres Dec 8, 2023

Choose a reason for hiding this comment

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

Most callbacks in this section of the walk still visit a section even if it's empty, so I kept that behaviour here. The one exception is the walk nest members SRPs block immediately above this one, which doesn't walk the nest member SRP block if it has a zero count.

Arguably it makes more sense to skip sections that are empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For instance, this block

https://github.com/eclipse-openj9/openj9/pull/18585/files#diff-46dfbaaa744dce041c4628662712f8529314d107b5d16d2bffb540c41b7a2dc7R186-R195

visits the enclosed inner classes SRP block even if it's empty, which has the odd consequence that the srpCursor for that section will point to the enclosedInnerClasses field of the ROM class, and not past the J9ROMClass header section like you might expect. That's because J9ROMCLASS_ENCLOSEDINNERCLASSES will return a pointer to enclosedInnerClasses when that field is zero. I don't think this ever messes anything up, but its an odd behaviour to have.

@cjjdespres
Copy link
Contributor Author

Just to be explicit about the problem: when romClass->romClassMethods is zero, J9ROMCLASS_ROMMETHODS(romClass) returns a pointer to the romClassMethods field itself in the J9ROMClass header. The srpCursor, meanwhile, would always point to somewhere in the romClass past that field when count was being calculated, meaning that firstMethod - srpCursor was negative, and so count was very large.

Copy link
Contributor

@mpirvu mpirvu left a comment

Choose a reason for hiding this comment

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

LGTM, but I am not an expert in this area.
@TobiAjila Could you please review or delegate? Thanks

@mpirvu mpirvu added the comp:vm label Jan 9, 2024
@tajila tajila requested a review from babsingh January 9, 2024 23:06
@babsingh
Copy link
Contributor

@cjjdespres Which tests showed erroneously large section size in ROM walk?

@babsingh
Copy link
Contributor

jenkins test sanity.functional,sanity.openjdk plinux jdk21

@cjjdespres
Copy link
Contributor Author

@cjjdespres Which tests showed erroneously large section size in ROM walk?

I noticed the problem while I was testing the code that revealed #18568. I'm not aware of existing failing tests.

@babsingh
Copy link
Contributor

@babsingh babsingh merged commit f66dfa2 into eclipse-openj9:master Jan 10, 2024
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants