[SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException #28750

dilipbiswal · 2020-06-08T08:20:27Z

What changes were proposed in this pull request?

A minor fix to fix the append method of StringConcat to cap the length at MAX_ROUNDED_ARRAY_LENGTH to make sure it does not overflow and cause StringIndexOutOfBoundsException

Thanks to Jeffrey Stokes for reporting the issue and explaining the underlying problem in detail in the JIRA.

Why are the changes needed?

This fixes StringIndexOutOfBoundsException on an overflow.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Added a test in StringsUtilSuite.

dilipbiswal · 2020-06-08T08:21:02Z

cc @maropu

kiszk · 2020-06-08T08:32:40Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/StringUtilsSuite.scala

    assert(checkLimit("1234567890"))
  }
+
+  test("StringConcat doesn't overflow on many inputs") {


nit: could you please add the prefix: "SPARK-31916: " ?

@kiszk Thanks.. will do.

MaxGekk

After your changes, the error message https://github.com/apache/spark/pull/28750/files#diff-46beff422d36d30d696d137ae1cfcf2eR151 will not show the actual size.

kiszk · 2020-06-08T08:34:25Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala

          val available = maxLength - length
          val stringToAppend = if (available >= sLen) s else s.substring(0, available)
          strings.append(stringToAppend)
+          length += sLen


nit: Can we move line 120 into {...} to make the scope sure?

@kiszk If i incorporate @MaxGekk's suggestion, then i will need to keep 120 where it is :-) as we need it outside the if block.

MaxGekk · 2020-06-08T08:39:24Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala

          strings.append(stringToAppend)
+          length += sLen
        }
-        length += sLen


How about:

length = Math.min(length.toLong + sLen, ByteArrayMethods.MAX_ROUNDED_ARRAY_LENGTH).toInt

@MaxGekk Yeah.. Sure. Just wondering if we absolutely need to change the length in case we are not doing an append. The error message you are referring to, is this captured in some test ? I ran catalyst and sql tests and didn't see any failure.

The error message you are referring to, is this captured in some test ?

Looks like not. Could you add a test for that?

@MaxGekk Sure.. Will add a test. Can you please point me to the error message please ? The link you have, when i click .. its not taking me to the relevant code ?

In the child class, here

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala

Line 156 in d9b3069

s"... ${length - maxLength} more characters"

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala

Line 151 in d9b3069

s"plan had length ${length} and the maximum is ${maxLength}. This behavior " +

Oh I see. If overflow is an issue, then make it a long? Capping it at this constant would then seem to potentially underreport the actual appended total.
Maybe that creates other problems, not sure.

@srowen You are right that, we may under report when the length becomes more than MAX_ROUNDED_ARRAY_LENGTH. I did consider changing the message to cover the case to say something like "greater than or equal to N". But thought against it as we are adding code to cover may be a extremely corner case. Please let me know if you have any suggestion here.

About changing to long, even if we did it, we can in theory hit the long limit, right ?

I'm not sure you can overflow a long - if 2^31-1 strings (max length of the array tracking them) are added, of length 2^31-1, you still don't overflow.
It's probably not worth debating so this is fine but I think a simpler change was also fine.

Thank you @srowen.

Ah, could you leave some comments about the intent here? I think its a bit hard to tell it at a glance.

MaxGekk

LGTM

SparkQA · 2020-06-08T12:49:22Z

Test build #123622 has finished for PR 28750 at commit 8d723d9.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-06-08T14:16:13Z

Test build #123626 has finished for PR 28750 at commit 0e71179.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-06-08T23:39:38Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/StringUtilsSuite.scala

+
+  test("SPARK-31916: StringConcat doesn't overflow on many inputs") {
+    val concat = new StringConcat(maxLength = 100)
+    0.to(Integer.MAX_VALUE).foreach { _ =>


Could you reduce # of loops in this test? I think its a bit expensive.

maropu · 2020-06-08T23:42:05Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

      Seq(Row(Short.MinValue.toLong * -1)))
  }
+
+


nit: remove the single blank.

maropu · 2020-06-08T23:42:43Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

+
+
+  test("SPARK-31916: verify that PlanStringConcat's output shows the actual length of the plan") {
+    withSQLConf("spark.sql.maxPlanStringLength" -> "0") {


plz use SQLConf.MAX_PLAN_STRING_LENGTH.key instead.

maropu · 2020-06-08T23:42:51Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

+      assert(concat.toString === "Truncated plan of 60 characters")
+    }
+
+    withSQLConf("spark.sql.maxPlanStringLength" -> "60") {


maropu · 2020-06-08T23:47:35Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

  }
+
+
+  test("SPARK-31916: verify that PlanStringConcat's output shows the actual length of the plan") {


Since PlanStringConcat's is in the catalyst package, could you move this tests there?

SparkQA · 2020-06-09T05:44:35Z

Test build #123670 has finished for PR 28750 at commit 3bedc7f.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class StringUtilsSuite extends SparkFunSuite with SQLHelper

viirya · 2020-06-09T06:07:47Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

 import org.apache.spark.sql.catalyst.optimizer.{ConvertToLocalRelation, NestedColumnAliasingSuite}
 import org.apache.spark.sql.catalyst.plans.logical.Project
 import org.apache.spark.sql.catalyst.util.StringUtils
+import org.apache.spark.sql.catalyst.util.StringUtils.PlanStringConcat


unnecessary change?

viirya

Please also update the PR description as looks like it is still for original fix.

SparkQA · 2020-06-09T07:05:01Z

Test build #123671 has finished for PR 28750 at commit df58e5c.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-06-09T07:05:02Z

Test build #123672 has finished for PR 28750 at commit 78cf8b5.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-06-09T07:06:38Z

retest this please

maropu · 2020-06-09T10:33:06Z

@dilipbiswal Have you checked my last comment? #28750 (comment) The PR itself looks okay.

SparkQA · 2020-06-09T12:27:01Z

Test build #123678 has finished for PR 28750 at commit 78cf8b5.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

viirya

Looks okay although I think making it a long might be also good and simpler.

SparkQA · 2020-06-10T06:48:12Z

Test build #123728 has finished for PR 28750 at commit 1050df3.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

dilipbiswal · 2020-06-10T06:49:17Z

@maropu

Have you checked my last comment? #28750 (comment) The PR itself looks okay.

Sorry i missed that. I have added the comment now.

@viirya

Looks okay although I think making it a long might be also good and simpler.

We could make it a long. The only thing is we may still overflow but it will take perhaps long time to hit it. I can repro the StringIndexOutOfBoundsException after changing the length to long. I made a minor tweak to change the append method to fake the input's length to be Int.MaxValue and adjust the test to increase the loop count.

maropu · 2020-06-10T09:45:34Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala

+
+        // Cap the length at  ByteArrayMethods.MAX_ROUNDED_ARRAY_LENGTH. Otherwise, we
+        // will overflow length causing StringIndexOutOfBoundsException in the substring call
+        // above.


@dilipbiswal Sorry and my comment looked ambiguous. I just wanted to leave a note about your comment above;

I think, the original intent of keeping this length outside the if block (which i missed and @MaxGekk pointed out) is to keep the true length of the input. So when we are producing a SQL plan, we want to tell users how much was that the size of the explain string and how much we truncated based on max length specification. So caller could call append method even after we have gone past the max.

I couldn't tell why the count-up is placed outside the if (!atLimit) block.

@maropu Ah .. so you wanted to explain the above comment better in the PR and not in the code ? Right ?

The comment in the code side looks okay and how about this?

// Keeps the total length of appended strings. Note that we need to cap the length at // `ByteArrayMethods.MAX_ROUNDED_ARRAY_LENGTH`; otherwise, we will overflow // length causing StringIndexOutOfBoundsException in the substring call above. length = Math.min(length.toLong + sLen, ByteArrayMethods.MAX_ROUNDED_ARRAY_LENGTH).toInt

SparkQA · 2020-06-10T11:22:15Z

Test build #123730 has finished for PR 28750 at commit c7e780b.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-06-10T23:45:18Z

retest this please

SparkQA · 2020-06-11T06:05:19Z

Test build #123797 has finished for PR 28750 at commit c7e780b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-06-11T07:05:02Z

Test build #123817 has finished for PR 28750 at commit 804edd0.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

dilipbiswal · 2020-06-11T08:33:09Z

retest this please

SparkQA · 2020-06-11T13:18:10Z

Test build #123837 has finished for PR 28750 at commit 804edd0.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

…eption ### What changes were proposed in this pull request? A minor fix to fix the append method of StringConcat to cap the length at MAX_ROUNDED_ARRAY_LENGTH to make sure it does not overflow and cause StringIndexOutOfBoundsException Thanks to **Jeffrey Stokes** for reporting the issue and explaining the underlying problem in detail in the JIRA. ### Why are the changes needed? This fixes StringIndexOutOfBoundsException on an overflow. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Added a test in StringsUtilSuite. Closes #28750 from dilipbiswal/SPARK-31916. Authored-by: Dilip Biswal <[email protected]> Signed-off-by: Takeshi Yamamuro <[email protected]> (cherry picked from commit b87a342) Signed-off-by: Takeshi Yamamuro <[email protected]>

maropu · 2020-06-12T00:21:23Z

Thanks! Merged to master/branch-3.0.
@dilipbiswal Could you make a backport PR to branch-2.4?

dilipbiswal · 2020-06-12T00:52:55Z

@maropu Sure. Thanks a lot for your comments on the pr.

maropu · 2020-06-12T00:55:58Z

Thanks!

dilipbiswal · 2020-06-12T01:49:38Z

Hi @maropu
Just checked the 2.4 codebase. We don't have this Class in 2.4. Looks like we directed appended to StringBuilder. This class was added as part of [SPARK-26504] (https://issues.apache.org/jira/browse/SPARK-26504) and fix version is 3.0.0

Please let me know.

maropu · 2020-06-12T02:36:51Z

Ah, okay. We don't need the backport. Thanks for the check, anyway!

…eption ### What changes were proposed in this pull request? A minor fix to fix the append method of StringConcat to cap the length at MAX_ROUNDED_ARRAY_LENGTH to make sure it does not overflow and cause StringIndexOutOfBoundsException Thanks to **Jeffrey Stokes** for reporting the issue and explaining the underlying problem in detail in the JIRA. ### Why are the changes needed? This fixes StringIndexOutOfBoundsException on an overflow. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Added a test in StringsUtilSuite. Closes apache#28750 from dilipbiswal/SPARK-31916. Authored-by: Dilip Biswal <[email protected]> Signed-off-by: Takeshi Yamamuro <[email protected]> (cherry picked from commit b87a342) Signed-off-by: Takeshi Yamamuro <[email protected]>

StringConcat can overflow , leads to StringIndexOutOfBoundsException

8d723d9

probot-autolabeler bot added the SQL label Jun 8, 2020

kiszk reviewed Jun 8, 2020

View reviewed changes

MaxGekk reviewed Jun 8, 2020

View reviewed changes

Review

6987a95

kiszk reviewed Jun 8, 2020

View reviewed changes

MaxGekk reviewed Jun 8, 2020

View reviewed changes

Code review

0e71179

MaxGekk approved these changes Jun 8, 2020

View reviewed changes

maropu reviewed Jun 8, 2020

View reviewed changes

maropu changed the title ~~[SPARK-31916][SQL] StringConcat can overflow , leads to StringIndexOutOfBoundsException~~ [SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException Jun 8, 2020

Code review

3bedc7f

style

df58e5c

viirya reviewed Jun 9, 2020

View reviewed changes

review

78cf8b5

viirya reviewed Jun 9, 2020

View reviewed changes

Add comment

1050df3

style

c7e780b

maropu reviewed Jun 10, 2020

View reviewed changes

Code review

804edd0

maropu approved these changes Jun 11, 2020

View reviewed changes

srowen approved these changes Jun 11, 2020

View reviewed changes

viirya approved these changes Jun 11, 2020

View reviewed changes

maropu closed this in b87a342 Jun 12, 2020



		test("SPARK-31916: verify that PlanStringConcat's output shows the actual length of the plan") {
		withSQLConf("spark.sql.maxPlanStringLength" -> "0") {

[SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException #28750

[SPARK-31916][SQL] StringConcat can lead to StringIndexOutOfBoundsException #28750

Uh oh!

Conversation

dilipbiswal commented Jun 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

dilipbiswal commented Jun 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dilipbiswal Jun 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 8, 2020

Uh oh!

SparkQA commented Jun 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 9, 2020

Uh oh!

SparkQA commented Jun 9, 2020

Uh oh!

maropu commented Jun 9, 2020

Uh oh!

maropu commented Jun 9, 2020

Uh oh!

SparkQA commented Jun 9, 2020

Uh oh!

dilipbiswal commented Jun 8, 2020 •

edited

Loading

dilipbiswal Jun 8, 2020 •

edited

Loading

dilipbiswal commented Jun 10, 2020 •

edited

Loading

dilipbiswal commented Jun 12, 2020 •

edited

Loading