[GitHub] [thrift] Jens-G commented on a change in pull request #2125: THRIFT-5190: StringUtils haven't take `(offset + length) > bytes.length` into account

[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