[GitHub] [hbase] saintstack commented on a change in pull request #2753: HBASE-25368 Filter out more invalid encoded name in isEncodedRegionNa…

[GitBox <gi...@apache.org>]

saintstack commented on a change in pull request #2753:
URL: https://github.com/apache/hbase/pull/2753#discussion_r538781244



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/RegionInfo.java
##########
@@ -363,7 +363,23 @@ static TableName getTable(final byte [] regionName) {
   @InterfaceAudience.Private // For use by internals only.
   public static boolean isEncodedRegionName(byte[] regionName) {
     // If not parseable as region name, presume encoded. TODO: add stringency; e.g. if hex.
-    return parseRegionNameOrReturnNull(regionName) == null && regionName.length <= MD5_HEX_LENGTH;
+    if (parseRegionNameOrReturnNull(regionName) == null) {
+      if (regionName.length > MD5_HEX_LENGTH) {
+        return false;
+      } else if (regionName.length == MD5_HEX_LENGTH) {
+        return true;
+      } else {
+        String encodedName = Bytes.toString(regionName);
+        try {
+          Integer.parseInt(encodedName);
+          // If this is a valid integer, it could be hbase:meta's encoded region name.
+          return true;

Review comment:
       We know the hbase:meta int. It does not change. Compare it? When meta splits, it will have md5 for new regions.

##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/RegionInfo.java
##########
@@ -363,7 +363,23 @@ static TableName getTable(final byte [] regionName) {
   @InterfaceAudience.Private // For use by internals only.
   public static boolean isEncodedRegionName(byte[] regionName) {
     // If not parseable as region name, presume encoded. TODO: add stringency; e.g. if hex.
-    return parseRegionNameOrReturnNull(regionName) == null && regionName.length <= MD5_HEX_LENGTH;
+    if (parseRegionNameOrReturnNull(regionName) == null) {
+      if (regionName.length > MD5_HEX_LENGTH) {
+        return false;
+      } else if (regionName.length == MD5_HEX_LENGTH) {
+        return true;

Review comment:
       What are we protecting against? Could we be passed a tablename? If so, why can't we have a tablename that is an MD5? Or 32 bytes in size?  Should we check it is all hex at least? I suppose if someone passes a tablename that is an md5, then they are asking for trouble?

##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/RegionInfo.java
##########
@@ -363,7 +363,23 @@ static TableName getTable(final byte [] regionName) {
   @InterfaceAudience.Private // For use by internals only.
   public static boolean isEncodedRegionName(byte[] regionName) {
     // If not parseable as region name, presume encoded. TODO: add stringency; e.g. if hex.
-    return parseRegionNameOrReturnNull(regionName) == null && regionName.length <= MD5_HEX_LENGTH;
+    if (parseRegionNameOrReturnNull(regionName) == null) {
+      if (regionName.length > MD5_HEX_LENGTH) {
+        return false;
+      } else if (regionName.length == MD5_HEX_LENGTH) {
+        return true;
+      } else {
+        String encodedName = Bytes.toString(regionName);
+        try {
+          Integer.parseInt(encodedName);
+          // If this is a valid integer, it could be hbase:meta's encoded region name.
+          return true;
+        } catch(NumberFormatException er) {
+          return false;
+        }
+      }
+    }
+    return false;

Review comment:
       How about some tests for this change in 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