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

Add missing checks on data array for getChars #4224

Merged
merged 1 commit into from
Jan 17, 2019

Conversation

liqunl
Copy link
Contributor

@liqunl liqunl commented Jan 9, 2019

Bound check and null check on data array are missing. Fix it.

Fixes #4122

Signed-off-by: Liqun Liu liqunl@ca.ibm.com

@liqunl liqunl force-pushed the getChars branch 2 times, most recently from 9b96ebd to a37e25d Compare January 9, 2019 21:07
@liqunl
Copy link
Contributor Author

liqunl commented Jan 9, 2019

@andrewcraik Can you review?

@@ -1830,7 +1830,8 @@ void getBytes(int start, int end, char[] data, int index) {
* when buffer is null
*/
public void getChars(int start, int end, char[] data, int index) {
if (0 <= start && start <= end && end <= lengthInternal()) {
// {@code data.length} comes with a null check
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't this comment is needed

Copy link
Member

Choose a reason for hiding this comment

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

agreed

@@ -1849,7 +1850,8 @@ void getCharsNoBoundChecks(int start, int end, char[] data, int index) {
}

void getChars(int start, int end, byte[] data, int index) {
if (0 <= start && start <= end && end <= lengthInternal()) {
// {@code data.length} comes with a null check
if (0 <= start && start <= end && end <= lengthInternal() && 0 <= index && end - start <= data.length/2 - index) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this check doesn't seem right - for a decompressed string vs a compressed string you have a different min length requirement. Given this is package protected do we need this check or is a simple getClass or whatever sufficient to trigger the appropriate NPE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andrewcraik Since this is package protected, maybe we don't need any checks? The caller should guarantee that the arguments passed in are valid.

Copy link
Contributor

Choose a reason for hiding this comment

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

It can be called from the general JCL (eg other stuff in java/lang) so not sure - maybe @DanHeidinga has a better idea

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 remove this byte[] overloaded API. It was formerly used in StringBuilder and StringBuffer but we have migrated all uses to getCharsNoBoundChecks, so there are to the best of my knowledge no known uses of this overload of getChars in the JCL. I vote we deprecate it here.

@andrewcraik
Copy link
Contributor

@DanHeidinga will be the right person to review/delegate a review for JCL changes.

@liqunl
Copy link
Contributor Author

liqunl commented Jan 11, 2019

@DanHeidinga ping?

@@ -1830,7 +1830,8 @@ void getBytes(int start, int end, char[] data, int index) {
* when buffer is null
*/
public void getChars(int start, int end, char[] data, int index) {
if (0 <= start && start <= end && end <= lengthInternal()) {
// {@code data.length} comes with a null check
Copy link
Member

Choose a reason for hiding this comment

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

agreed

@@ -1830,7 +1830,8 @@ void getBytes(int start, int end, char[] data, int index) {
* when buffer is null
*/
public void getChars(int start, int end, char[] data, int index) {
if (0 <= start && start <= end && end <= lengthInternal()) {
// {@code data.length} comes with a null check
if (0 <= start && start <= end && end <= lengthInternal() && 0 <= index && end - start <= data.length - index) {
Copy link
Member

Choose a reason for hiding this comment

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

Some additional brackets would help clarify the grouping here - ((end - start) <= (data.length - index))

@@ -1849,7 +1850,8 @@ void getCharsNoBoundChecks(int start, int end, char[] data, int index) {
}

void getChars(int start, int end, byte[] data, int index) {
if (0 <= start && start <= end && end <= lengthInternal()) {
// {@code data.length} comes with a null check
Copy link
Member

Choose a reason for hiding this comment

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

Remove the comment

@liqunl liqunl force-pushed the getChars branch 2 times, most recently from 415b249 to 1543d5c Compare January 14, 2019 22:01
@fjeremic
Copy link
Contributor

Kicking off a JDK8 build to verify things are working on OpenJ9's version of StringBuilder/StringBuffer.

Jenkins test sanity xlinux JDK8

Bound check and null check on data array are missing. Fix it.

Fixes eclipse-openj9#4122

Signed-off-by: Liqun Liu <liqunl@ca.ibm.com>
@liqunl
Copy link
Contributor Author

liqunl commented Jan 15, 2019

@DanHeidinga @fjeremic @andrewcraik Have to keep void getChars(int start, int end, byte[] data, int index) for Z. Could you take a look again?

@fjeremic
Copy link
Contributor

@DanHeidinga @fjeremic @andrewcraik Have to keep void getChars(int start, int end, byte[] data, int index) for Z. Could you take a look again?

Specifically z/OS which is currently not tested at OpenJ9.

@liqunl
Copy link
Contributor Author

liqunl commented Jan 16, 2019

@andrewcraik @DanHeidinga Ping?

@andrewcraik
Copy link
Contributor

seems ok to me - review is with @DanHeidinga

@fjeremic fjeremic assigned fjeremic and DanHeidinga and unassigned fjeremic Jan 16, 2019
@DanHeidinga
Copy link
Member

Jenkins test sanity plinux jdk8

@DanHeidinga
Copy link
Member

Jenkins test extended xlinux jdk11

Copy link
Member

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

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

I'm OK with this change. I think we're also fine to delete the unused bytes[] api, though that can be done in a separate pr

@DanHeidinga DanHeidinga merged commit 6b6c2c8 into eclipse-openj9:master Jan 17, 2019
@liqunl liqunl deleted the getChars branch January 24, 2019 16:51
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.

5 participants