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 String.indexOf corner case where start == Integer.MAX_VALUE - 1 #6403

Merged
merged 2 commits into from
Jul 11, 2019

Conversation

fjeremic
Copy link
Contributor

@fjeremic fjeremic commented Jul 9, 2019

When calling the java.lang.String.indexOf API with a String
argument and passing the starting offset to be Integer.MAX_VALUE - 1
we encounter an overflow when we test for the following condition:

if (start + s2len > s1len) {
    return -1;
}

This if statement checks that the there are enough characters from the
starting position the user specified for the substring to occur. i.e.
if the source string is "abcdefg" and the substring is "fghijk" and
start offset is 2 then because the length of the source string is 7 and
the length of the substring is 6, 2 + 6 > 7 so it is impossible for the
substring to be contained within the source string. We can return -1
immediately.

The problem with the if statement is if we pass a starting offset to be
Integer.MAX_VALUE - 1 which is a valid input the addition in the if
statement overflows and we never enter the if block, even though we
should have had there been no overflow. This causes us to call the
JITHelpers intrinsic which has implicit assumptions about the arguments
and we end up dereferencing memory outside of the heap.

This commit fixes the overflow by using subtraction. We also take this
opportunity to document the implicit assumptions on the JIT intrinsics
that we make use of.

Signed-off-by: Filip Jeremic fjeremic@ca.ibm.com

When calling the `java.lang.String.indexOf` API with a `String`
argument and passing the starting offset to be `Integer.MAX_VALUE - 1`
we encounter an overflow when we test for the following condition:

```
if (start + s2len > s1len) {
    return -1;
}
```

This if statement checks that the there are enough characters from the
starting position the user specified for the substring to occur. i.e.
if the source string is "abcdefg" and the substring is "fghijk" and
start offset is 2 then because the length of the source string is 7 and
the length of the substring is 6, 2 + 6 > 7 so it is impossible for the
substring to be contained within the source string. We can return -1
immediately.

The problem with the if statement is if we pass a starting offset to be
`Integer.MAX_VALUE - 1` which is a valid input the addition in the if
statement overflows and we never enter the if block, even though we
should have had there been no overflow. This causes us to call the
JITHelpers intrinsic which has implicit assumptions about the arguments
and we end up dereferencing memory outside of the heap.

This commit fixes the overflow by using subtraction. We also take this
opportunity to document the implicit assumptions on the JIT intrinsics
that we make use of.

Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
Use the same function name across all codegens currently inlining
`IntrinsicIndexOf` and standardize the call argument names to be
identical to that of the Java code.

Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
@fjeremic
Copy link
Contributor Author

fjeremic commented Jul 9, 2019

Jenkins test sanity all jdk8

@andrewcraik
Copy link
Contributor

x changes and common code changes look good - I'll leave it to someone else to verify the POWER and z changes since I'm not an expert on those areas.

@fjeremic
Copy link
Contributor Author

@gita-omr @r30shah could we get a review for Power and Z please?

Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

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

Refactoring done in the Z codegen evaluator looks good to me.

@gita-omr
Copy link
Contributor

@aviansie-ben could you please review Power changes?

@gita-omr
Copy link
Contributor

Looks fine to me. Just wanted @aviansie-ben to take a look at the Power changes.

Copy link
Contributor

@aviansie-ben aviansie-ben left a comment

Choose a reason for hiding this comment

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

Power codegen changes look good to me

@gita-omr gita-omr self-requested a review July 11, 2019 16:28
@pshipton
Copy link
Member

I'll go ahead and merge since it seems everyone has approved now.

@pshipton pshipton merged commit 14f290f into eclipse-openj9:master Jul 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants