You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2020/04/28 18:47:52 UTC
[GitHub] [thrift] dugenkui03 opened a new pull request #2125: StringUtils haven't take into account
dugenkui03 opened a new pull request #2125:
URL: https://github.com/apache/thrift/pull/2125
<!-- Explain the changes in the pull request below: -->
1. StringUtils haven't take `(offset + length)` > bytes.length';
2. `if ((offset | length | (offset + length) | (size - (offset + length))) < 0)` will faster than `>`
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [x] [Apache Jira-THRIFT-5190](https://issues.apache.org/jira/browse/THRIFT-5190)
- [ ] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [x] Did you squash your changes to a single commit? (not required, but preferred)
- [ ] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [ ] If your change does not involve any code, add ` [skip ci]` at the end of your pull request to free up build resources.
<!--
The Contributing Guide at:
https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
has more details and tips for committing properly.
-->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on a change in pull request #2125: THRIFT-5190: StringUtils haven't take `(offset + length) > bytes.length` into account
Posted by GitBox <gi...@apache.org>.
Jens-G commented on a change in pull request #2125:
URL: https://github.com/apache/thrift/pull/2125#discussion_r428576854
##########
File path: lib/java/src/org/apache/thrift/utils/StringUtils.java
##########
@@ -49,12 +49,10 @@ public static String bytesToHexString(byte[] bytes) {
* @return hex string.
*/
public static String bytesToHexString(byte[] bytes, int offset, int length) {
- if (length < 0) {
- throw new IllegalArgumentException("Negative length " + length);
- }
- if (offset < 0) {
- throw new IndexOutOfBoundsException("Negative start offset " + offset);
+ if ((offset | length | (offset + length) | (bytes.length - (offset + length))) < 0) {
Review comment:
A testcase for this change would be a great addition.
##########
File path: lib/java/src/org/apache/thrift/utils/StringUtils.java
##########
@@ -49,12 +49,10 @@ public static String bytesToHexString(byte[] bytes) {
* @return hex string.
*/
public static String bytesToHexString(byte[] bytes, int offset, int length) {
- if (length < 0) {
- throw new IllegalArgumentException("Negative length " + length);
- }
- if (offset < 0) {
- throw new IndexOutOfBoundsException("Negative start offset " + offset);
+ if ((offset | length | (offset + length) | (bytes.length - (offset + length))) < 0) {
Review comment:
To be honest,I personally have a problem with code like this where you need to read it three times to get it right (and I'm still not fully sure I "groked" it). I leave it to someone else to give a final review statement. +0 from me stince I won't be standing in the way, but I'm not really in favor of it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] dugenkui03 commented on a change in pull request #2125: THRIFT-5190: StringUtils haven't take `(offset + length) > bytes.length` into account
Posted by GitBox <gi...@apache.org>.
dugenkui03 commented on a change in pull request #2125:
URL: https://github.com/apache/thrift/pull/2125#discussion_r429215993
##########
File path: lib/java/src/org/apache/thrift/utils/StringUtils.java
##########
@@ -49,12 +49,10 @@ public static String bytesToHexString(byte[] bytes) {
* @return hex string.
*/
public static String bytesToHexString(byte[] bytes, int offset, int length) {
- if (length < 0) {
- throw new IllegalArgumentException("Negative length " + length);
- }
- if (offset < 0) {
- throw new IndexOutOfBoundsException("Negative start offset " + offset);
+ if ((offset | length | (offset + length) | (bytes.length - (offset + length))) < 0) {
Review comment:
I haven't notice that I have removed IllegalArgumentException instead of IndexOutOfBoundsException, and I haven't notice that it will be a breaking change in the API.
great suggestion, thanks very much.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] dugenkui03 commented on a change in pull request #2125: THRIFT-5190: StringUtils haven't take `(offset + length) > bytes.length` into account
Posted by GitBox <gi...@apache.org>.
dugenkui03 commented on a change in pull request #2125:
URL: https://github.com/apache/thrift/pull/2125#discussion_r428821762
##########
File path: lib/java/src/org/apache/thrift/utils/StringUtils.java
##########
@@ -49,12 +49,10 @@ public static String bytesToHexString(byte[] bytes) {
* @return hex string.
*/
public static String bytesToHexString(byte[] bytes, int offset, int length) {
- if (length < 0) {
- throw new IllegalArgumentException("Negative length " + length);
- }
- if (offset < 0) {
- throw new IndexOutOfBoundsException("Negative start offset " + offset);
+ if ((offset | length | (offset + length) | (bytes.length - (offset + length))) < 0) {
Review comment:
> if ((offset | length | (offset + length) | (bytes.length - (offset + length))) < 0)
when length is less than 0, it will throw IllegalArgumentException, such as `int offset=2,length=-1,byteLength=10;`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] joaopedroantonio commented on a change in pull request #2125: THRIFT-5190: StringUtils haven't take `(offset + length) > bytes.length` into account
Posted by GitBox <gi...@apache.org>.
joaopedroantonio commented on a change in pull request #2125:
URL: https://github.com/apache/thrift/pull/2125#discussion_r428580974
##########
File path: lib/java/src/org/apache/thrift/utils/StringUtils.java
##########
@@ -49,12 +49,10 @@ public static String bytesToHexString(byte[] bytes) {
* @return hex string.
*/
public static String bytesToHexString(byte[] bytes, int offset, int length) {
- if (length < 0) {
- throw new IllegalArgumentException("Negative length " + length);
- }
- if (offset < 0) {
- throw new IndexOutOfBoundsException("Negative start offset " + offset);
+ if ((offset | length | (offset + length) | (bytes.length - (offset + length))) < 0) {
Review comment:
```suggestion
if (offset < 0 || bytes.length < (offset + length)) {
```
And maybe we should keep the IllegalArgumentException when length is less than 0?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ctubbsii commented on a change in pull request #2125: THRIFT-5190: StringUtils haven't take `(offset + length) > bytes.length` into account
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2125:
URL: https://github.com/apache/thrift/pull/2125#discussion_r428664284
##########
File path: lib/java/src/org/apache/thrift/utils/StringUtils.java
##########
@@ -49,12 +49,10 @@ public static String bytesToHexString(byte[] bytes) {
* @return hex string.
*/
public static String bytesToHexString(byte[] bytes, int offset, int length) {
- if (length < 0) {
- throw new IllegalArgumentException("Negative length " + length);
- }
- if (offset < 0) {
- throw new IndexOutOfBoundsException("Negative start offset " + offset);
+ if ((offset | length | (offset + length) | (bytes.length - (offset + length))) < 0) {
Review comment:
I'm responding to the request on the mailing list for another pair of eyes on this PR.
I agree with @Jens-G regarding the readability... readability is far more important than an every-so-slight performance benefit. Obfuscated code is very hard to maintain, even with good test cases.
@joaopedroantonio 's suggestions are a great improvement, and preserves existing behavior, while adding the bounds checks when using offset and length.
A unit test case that covers the edge cases would be a nice sanity check for this method.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G closed pull request #2125: THRIFT-5190: StringUtils haven't take `(offset + length) > bytes.length` into account
Posted by GitBox <gi...@apache.org>.
Jens-G closed pull request #2125:
URL: https://github.com/apache/thrift/pull/2125
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] dugenkui03 commented on a change in pull request #2125: THRIFT-5190: StringUtils haven't take `(offset + length) > bytes.length` into account
Posted by GitBox <gi...@apache.org>.
dugenkui03 commented on a change in pull request #2125:
URL: https://github.com/apache/thrift/pull/2125#discussion_r428821954
##########
File path: lib/java/src/org/apache/thrift/utils/StringUtils.java
##########
@@ -49,12 +49,10 @@ public static String bytesToHexString(byte[] bytes) {
* @return hex string.
*/
public static String bytesToHexString(byte[] bytes, int offset, int length) {
- if (length < 0) {
- throw new IllegalArgumentException("Negative length " + length);
- }
- if (offset < 0) {
- throw new IndexOutOfBoundsException("Negative start offset " + offset);
+ if ((offset | length | (offset + length) | (bytes.length - (offset + length))) < 0) {
Review comment:
it seem that there will always be readability problem when using bit operation.
I do agree with that the readability of code is important. The code will be changed and squashed to a single commit.
Thanks very much.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on a change in pull request #2125: THRIFT-5190: StringUtils haven't take `(offset + length) > bytes.length` into account
Posted by GitBox <gi...@apache.org>.
Jens-G commented on a change in pull request #2125:
URL: https://github.com/apache/thrift/pull/2125#discussion_r430719637
##########
File path: lib/java/test/org/apache/thrift/utils/TestStringUtils.java
##########
@@ -31,4 +31,27 @@ public void testToHexString() {
Assert.assertEquals("EFAB92", StringUtils.bytesToHexString(bytes, 2, 3));
Assert.assertNull(StringUtils.bytesToHexString(null));
}
+
+ @Test
+ public void testToHexStringIndexOutOfBoundsExceptionExamine() {
+ byte[] bytes = {1, 2, 3, 4, 5};
+ try {
+ StringUtils.bytesToHexString(bytes, 0, -1);
+ } catch (Exception e) {
Review comment:
The (hypothetical?) case when it would not throw is not covered. Same below.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] joaopedroantonio commented on a change in pull request #2125: THRIFT-5190: StringUtils haven't take `(offset + length) > bytes.length` into account
Posted by GitBox <gi...@apache.org>.
joaopedroantonio commented on a change in pull request #2125:
URL: https://github.com/apache/thrift/pull/2125#discussion_r429193227
##########
File path: lib/java/src/org/apache/thrift/utils/StringUtils.java
##########
@@ -49,12 +49,10 @@ public static String bytesToHexString(byte[] bytes) {
* @return hex string.
*/
public static String bytesToHexString(byte[] bytes, int offset, int length) {
- if (length < 0) {
- throw new IllegalArgumentException("Negative length " + length);
- }
- if (offset < 0) {
- throw new IndexOutOfBoundsException("Negative start offset " + offset);
+ if ((offset | length | (offset + length) | (bytes.length - (offset + length))) < 0) {
Review comment:
Maybe I'm being picky here but I think we should keep the IllegalArgumentException when `length < 0`, for two reasons:
1. The semantics of IndexOutOfBoundsException and the IllegalArgumentException are different and I think `length < 0` is a case of the latter.
2. Neither of those exceptions is checked and changing the type of exception when `length < 0` will not be explicit in compile time. I'd consider this a breaking change in the API.
WYT?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org