-
Notifications
You must be signed in to change notification settings - Fork 738
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
Conversation
9b96ebd
to
a37e25d
Compare
@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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
@DanHeidinga will be the right person to review/delegate a review for JCL changes. |
@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 |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the comment
415b249
to
1543d5c
Compare
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>
@DanHeidinga @fjeremic @andrewcraik Have to keep |
Specifically z/OS which is currently not tested at OpenJ9. |
@andrewcraik @DanHeidinga Ping? |
seems ok to me - review is with @DanHeidinga |
Jenkins test sanity plinux jdk8 |
Jenkins test extended xlinux jdk11 |
There was a problem hiding this 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
Bound check and null check on data array are missing. Fix it.
Fixes #4122
Signed-off-by: Liqun Liu liqunl@ca.ibm.com