Repository: pinpoint-apm/pinpoint
Issue: PreparedStatementExecuteQueryInterceptor.toBindVariable() array index bug fix. 
Labels: bug

incorrect array index handling of SqlBindValue 
#736


Issue comments:

Pull request review comments:

Patch that resolved the issue:
diff --git a/plugins/jdbc-driver/src/main/java/com/navercorp/pinpoint/plugin/jdbc/common/bindvalue/BindValueUtils.java b/plugins/jdbc-driver/src/main/java/com/navercorp/pinpoint/plugin/jdbc/common/bindvalue/BindValueUtils.java
index 5cc7ed292c49..72ccd7954431 100644
--- a/plugins/jdbc-driver/src/main/java/com/navercorp/pinpoint/plugin/jdbc/common/bindvalue/BindValueUtils.java
+++ b/plugins/jdbc-driver/src/main/java/com/navercorp/pinpoint/plugin/jdbc/common/bindvalue/BindValueUtils.java
@@ -18,14 +18,51 @@
 
 import com.navercorp.pinpoint.bootstrap.util.StringUtils;
 
+import java.util.Map;
+
 /**
+ * duplicate : com.navercorp.pinpoint.profiler.modifier.db.interceptor.BindValueUtils
  * @author emeroad
  */
-public class BindValueUtils {
+public final class BindValueUtils {
 
     private BindValueUtils() {
     }
 
+    public static String bindValueToString(final Map<Integer, String> bindValueMap, int limit) {
+        if (bindValueMap == null) {
+            return "";
+        }
+        if (bindValueMap.isEmpty()) {
+            return "";
+        }
+        final int maxParameterIndex = getMaxParameterIndex(bindValueMap);
+        if (maxParameterIndex <= 0) {
+            return "";
+        }
+        final String[] temp = new String[maxParameterIndex];
+        for (Map.Entry<Integer, String> entry : bindValueMap.entrySet()) {
+            final int parameterIndex = entry.getKey() - 1;
+            if (parameterIndex < 0) {
+                // invalid index. PreparedStatement first parameterIndex is 1
+                continue;
+            }
+            if (temp.length <= parameterIndex) {
+                continue;
+            }
+            temp[parameterIndex] = entry.getValue();
+        }
+        return bindValueToString(temp, limit);
+    }
+
+    private static int getMaxParameterIndex(Map<Integer, String> bindValueMap) {
+        int maxIndex = 0;
+        for (Integer idx : bindValueMap.keySet()) {
+            maxIndex = Math.max(maxIndex, idx);
+        }
+        return maxIndex;
+    }
+
     public static String bindValueToString(String[] bindValueArray, int limit) {
         if (bindValueArray == null) {
             return "";
@@ -39,7 +76,8 @@ public static String bindValueToString(String[] bindValueArray, int limit) {
                 appendLength(sb, length);
                 break;
             }
-            StringUtils.appendDrop(sb, bindValueArray[i], limit);
+            final String bindValue = StringUtils.defaultString(bindValueArray[i], "");
+            StringUtils.appendDrop(sb, bindValue, limit);
             if (i < end) {
                 sb.append(", ");
             }
diff --git a/plugins/jdbc-driver/src/main/java/com/navercorp/pinpoint/plugin/jdbc/common/interceptor/PreparedStatementExecuteQueryInterceptor.java b/plugins/jdbc-driver/src/main/java/com/navercorp/pinpoint/plugin/jdbc/common/interceptor/PreparedStatementExecuteQueryInterceptor.java
index 815734ba5931..ee7c6bcb7f1e 100644
--- a/plugins/jdbc-driver/src/main/java/com/navercorp/pinpoint/plugin/jdbc/common/interceptor/PreparedStatementExecuteQueryInterceptor.java
+++ b/plugins/jdbc-driver/src/main/java/com/navercorp/pinpoint/plugin/jdbc/common/interceptor/PreparedStatementExecuteQueryInterceptor.java
@@ -130,17 +130,7 @@ private void clean(Object target) {
     }
 
     private String toBindVariable(Map<Integer, String> bindValue) {
-        final String[] temp = new String[bindValue.size()];
-        for (Map.Entry<Integer, String> entry : bindValue.entrySet()) {
-            Integer key = entry.getKey() - 1;
-            if (temp.length <= key) {
-                continue;
-            }
-            temp[key] = entry.getValue();
-        }
-
-        return BindValueUtils.bindValueToString(temp, maxSqlBindValueLength);
-
+        return BindValueUtils.bindValueToString(bindValue, maxSqlBindValueLength);
     }
 
     @Override
diff --git a/profiler/src/main/java/com/navercorp/pinpoint/profiler/modifier/db/interceptor/BindValueUtils.java b/profiler/src/main/java/com/navercorp/pinpoint/profiler/modifier/db/interceptor/BindValueUtils.java
index 815f613b2bb8..50c78810eef4 100644
--- a/profiler/src/main/java/com/navercorp/pinpoint/profiler/modifier/db/interceptor/BindValueUtils.java
+++ b/profiler/src/main/java/com/navercorp/pinpoint/profiler/modifier/db/interceptor/BindValueUtils.java
@@ -18,14 +18,50 @@
 
 import com.navercorp.pinpoint.bootstrap.util.StringUtils;
 
+import java.util.Map;
+
 /**
  * @author emeroad
  */
-public class BindValueUtils {
+public final class BindValueUtils {
 
     private BindValueUtils() {
     }
 
+    public static String bindValueToString(final Map<Integer, String> bindValueMap, int limit) {
+        if (bindValueMap == null) {
+            return "";
+        }
+        if (bindValueMap.isEmpty()) {
+            return "";
+        }
+        final int maxParameterIndex = getMaxParameterIndex(bindValueMap);
+        if (maxParameterIndex <= 0) {
+            return "";
+        }
+        final String[] temp = new String[maxParameterIndex];
+        for (Map.Entry<Integer, String> entry : bindValueMap.entrySet()) {
+            final int parameterIndex = entry.getKey() - 1;
+            if (parameterIndex < 0) {
+                // invalid index. PreparedStatement first parameterIndex is 1
+                continue;
+            }
+            if (temp.length <= parameterIndex) {
+                continue;
+            }
+            temp[parameterIndex] = entry.getValue();
+        }
+        return bindValueToString(temp, limit);
+    }
+
+    private static int getMaxParameterIndex(Map<Integer, String> bindValueMap) {
+        int maxIndex = 0;
+        for (Integer idx : bindValueMap.keySet()) {
+            maxIndex = Math.max(maxIndex, idx);
+        }
+        return maxIndex;
+    }
+
     public static String bindValueToString(String[] bindValueArray, int limit) {
         if (bindValueArray == null) {
             return "";
@@ -39,7 +75,8 @@ public static String bindValueToString(String[] bindValueArray, int limit) {
                 appendLength(sb, length);
                 break;
             }
-            StringUtils.appendDrop(sb, bindValueArray[i], limit);
+            final String bindValue = StringUtils.defaultString(bindValueArray[i], "");
+            StringUtils.appendDrop(sb, bindValue, limit);
             if (i < end) {
                 sb.append(", ");
             }
diff --git a/profiler/src/main/java/com/navercorp/pinpoint/profiler/modifier/db/interceptor/PreparedStatementExecuteQueryInterceptor.java b/profiler/src/main/java/com/navercorp/pinpoint/profiler/modifier/db/interceptor/PreparedStatementExecuteQueryInterceptor.java
index 0b1f9da3d938..4ea56ffcf2db 100644
--- a/profiler/src/main/java/com/navercorp/pinpoint/profiler/modifier/db/interceptor/PreparedStatementExecuteQueryInterceptor.java
+++ b/profiler/src/main/java/com/navercorp/pinpoint/profiler/modifier/db/interceptor/PreparedStatementExecuteQueryInterceptor.java
@@ -106,17 +106,7 @@ private void clean(Object target) {
     }
 
     private String toBindVariable(Map<Integer, String> bindValue) {
-        final String[] temp = new String[bindValue.size()];
-        for (Map.Entry<Integer, String> entry : bindValue.entrySet()) {
-            Integer key = entry.getKey() - 1;
-            if (temp.length < key) {
-                continue;
-            }
-            temp[key] = entry.getValue();
-        }
-
-        return BindValueUtils.bindValueToString(temp, maxSqlBindValueLength);
-
+        return BindValueUtils.bindValueToString(bindValue, maxSqlBindValueLength);
     }
 
     @Override


Test patch:
diff --git a/profiler/src/test/java/com/navercorp/pinpoint/profiler/modifier/db/interceptor/BindValueUtilsTest.java b/profiler/src/test/java/com/navercorp/pinpoint/profiler/modifier/db/interceptor/BindValueUtilsTest.java
index a903ed82965a..765e628c2b82 100644
--- a/profiler/src/test/java/com/navercorp/pinpoint/profiler/modifier/db/interceptor/BindValueUtilsTest.java
+++ b/profiler/src/test/java/com/navercorp/pinpoint/profiler/modifier/db/interceptor/BindValueUtilsTest.java
@@ -17,10 +17,10 @@
 package com.navercorp.pinpoint.profiler.modifier.db.interceptor;
 
 import org.junit.Assert;
-
 import org.junit.Test;
 
-import com.navercorp.pinpoint.profiler.modifier.db.interceptor.BindValueUtils;
+import java.util.HashMap;
+import java.util.Map;
 
 public class BindValueUtilsTest {
 
@@ -85,7 +85,7 @@ public void testBindValueToString_limit7() throws Exception {
 
     @Test
     public void testBindValueToString_null() throws Exception {
-        String result = BindValueUtils.bindValueToString(null, 10);
+        String result = BindValueUtils.bindValueToString((String[])null, 10);
         Assert.assertEquals("", result);
     }
 
@@ -109,4 +109,56 @@ public void testBindValueToString_twoLargeString() throws Exception {
         String result = BindValueUtils.bindValueToString(bindValue, 5);
         Assert.assertEquals("12345...(6), ...(2)", result);
     }
+
+    // #737 https://github.com/naver/pinpoint/issues/737
+    @Test
+    public void test_734_bug_regression() throws Exception {
+        Map<Integer, String> bindValue = new HashMap<Integer, String>();
+        bindValue.put(1, "1");
+        bindValue.put(2, "2");
+        // skip 3
+        bindValue.put(4, "4");
+
+        String bindValueToString = BindValueUtils.bindValueToString(bindValue, 100);
+        Assert.assertEquals("1, 2, , 4", bindValueToString);
+    }
+
+    @Test
+    public void test_index_error_zero() throws Exception {
+        Map<Integer, String> bindValue = new HashMap<Integer, String>();
+        bindValue.put(0, "0");
+
+        String bindValueToString = BindValueUtils.bindValueToString(bindValue, 100);
+        Assert.assertEquals("", bindValueToString);
+    }
+
+    @Test
+    public void test_index_error_negative() throws Exception {
+        Map<Integer, String> bindValue = new HashMap<Integer, String>();
+        bindValue.put(-2, "-2");
+
+        String bindValueToString = BindValueUtils.bindValueToString(bindValue, 100);
+        Assert.assertEquals("", bindValueToString);
+    }
+
+    @Test
+    public void test_index_error_complex() throws Exception {
+        Map<Integer, String> bindValue = new HashMap<Integer, String>();
+        bindValue.put(-2, "-2");
+        bindValue.put(0, "0");
+        bindValue.put(1, "1");
+        bindValue.put(3, "3");
+
+        String bindValueToString = BindValueUtils.bindValueToString(bindValue, 100);
+        Assert.assertEquals("1, , 3", bindValueToString);
+    }
+
+    @Test
+    public void test_NullElement() throws Exception {
+        String[] temp = {"1", null, "3"};
+        String bindValueToString = BindValueUtils.bindValueToString(temp, 100);
+        Assert.assertEquals("1, , 3", bindValueToString);
+    }
+
+
 }
\ No newline at end of file