SEPURI-SAI-KRISHNA opened a new pull request, #57922:
URL: https://github.com/apache/spark/pull/57922
### What changes were proposed in this pull request?
This PR fixes three integer-overflow / missing-guard bugs in the argument
handling of
`substring()`, `lpad()` and `rpad()`, all of which let an out-of-range `len`
argument reach an
allocation it should never have reached.
`ByteArray.subStringSQL` (the BINARY implementation of `substring()`)
computed its end offset in
`int` arithmetic:
```java
if ((bytes.length - start) < len) { // overflows when len is large negative
end = bytes.length;
} else {
end = start + len; // overflows when start + len >
Integer.MAX_VALUE
}
start = Math.max(start, 0); // start clamped only AFTER it was
used above
```
When `start` is still negative, `bytes.length - start` overflows, the `end =
bytes.length` clamp is
skipped, and `end` keeps an out-of-range value. `Arrays.copyOfRange` accepts
an end past the array
length and zero-pads the remainder, so the result is silently a large array
of zero bytes. This is
now computed in `long` and clamped to `bytes.length`, with `start` clamped
only afterwards.
`ByteArray.lpad` / `ByteArray.rpad` guarded only the exact value `0` before
`new byte[len]`, so a
negative `len` threw `NegativeArraySizeException`. The guard is now `len <=
0`, which also makes
`padWithEmptyPattern` unreachable with a non-positive length.
`UTF8String.lpad` / `UTF8String.rpad` start with `int spaces = len -
numChars()`, which wraps to a
large positive value when `len == Integer.MIN_VALUE`. That skips the `spaces
<= 0` branch and takes
the padding branch, failing in `Math.toIntExact`. Both now return the empty
string up front for
`len <= 0`, before the wrapping subtraction is evaluated.
With all three in place, a non-positive length yields the empty value for
STRING and BINARY alike
across the whole `int` range, and no argument combination can produce a
result longer than the
input.
### Why are the changes needed?
All three are reachable from plain SQL with default configuration, on both
the interpreted and
codegen paths.
```sql
-- 1. wrong results, and an allocation unrelated to the size of the input
SELECT length(substring(CAST('Spark SQL' AS BINARY), -1207959552,
-1207959552));
-- before: 1879048201 (a 1.75 GiB array of mostly zero bytes)
-- after: 0
SELECT substring(CAST('Spark SQL' AS BINARY), -2147483648, 5);
-- before: the whole 9-byte input
-- after: empty
-- 2. raw JVM exception rather than a SparkThrowable
SELECT lpad(CAST('hi' AS BINARY), -1, CAST('??' AS BINARY));
-- before: java.lang.NegativeArraySizeException: -1
-- after: empty binary
-- 3. same, on the STRING side, at one specific length
SELECT lpad('hello', -2147483648, '??');
-- before: java.lang.ArithmeticException: integer overflow
-- after: empty string
```
The equivalent STRING expressions already returned `''` for the first two
cases, so this is also a
STRING/BINARY consistency fix rather than new behaviour being invented. The
STRING path was fixed
for this class of overflow in SPARK-32115, but `ByteArray.subStringSQL`
(added by SPARK-28412) was
never brought in line, even though its comment says the offsets are computed
"according to
UTF8String#subStringSQL". The pad overloads were added by SPARK-37047.
Beyond returning wrong results, the first case is a robustness concern: the
allocation size is
driven by user-supplied arguments and is unrelated to the size of the input,
so a modestly sized
query can allocate gigabytes and OOM the executor.
### Does this PR introduce _any_ user-facing change?
Yes, for the argument combinations described above, all of which are
currently broken.
| Expression | Before | After |
|---|---|---|
| `substring(<binary>, -1207959552, -1207959552)` | 1879048201-byte array of
zeros | empty binary |
| `substring(<binary>, -2147483648, 5)` | the whole input | empty binary |
| `lpad(<binary>, -1, <pad>)` | `NegativeArraySizeException` | empty binary |
| `rpad(<binary>, -1, <pad>)` | `NegativeArraySizeException` | empty binary |
| `lpad('hello', -2147483648, '??')` | `ArithmeticException` | empty string |
In every case the new result matches what the corresponding STRING
expression already returned for
non-positive lengths. No behaviour changes for any in-range argument.
### How was this patch tested?
New unit tests:
- `ByteArraySuite.testSubStringSQL` and `ByteArraySuite.testPad` cover the
positive, zero and
negative positions, the overflowing combinations above, and the null-input
cases.
- `UTF8StringSuite.pad` gains the non-positive lengths including
`Integer.MIN_VALUE`, for both an
empty and a non-empty padding pattern.
- `StringExpressionsSuite` covers the same combinations at the expression
level for `Substring`,
`StringLPad` / `StringRPad` and `BinaryPad`. These go through
`checkEvaluation`, so both the
interpreted and the codegen paths are exercised.
In addition, `ByteArray.subStringSQL` was checked against
`UTF8String.substringSQL` as an oracle
over 3125 `(input, pos, len)` combinations built from boundary values
(`Integer.MIN_VALUE`,
`Integer.MAX_VALUE`, `+/-1207959552`, and small offsets around the input
length). For pure-ASCII
inputs character indexing and byte indexing coincide, so the two must agree
exactly. Current master
reports 239 mismatches; with this patch, 0.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5)
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
