cloud-fan commented on code in PR #59927:
URL: https://github.com/apache/spark/pull/59927#discussion_r4444274770
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionSuite.scala:
##########
@@ -1468,6 +1468,101 @@ class VariantExpressionSuite extends SparkFunSuite with
ExpressionEvalHelper {
/* offset list */ 0, 2,
/* field data: INT8 with only 1 byte */ primitiveHeader(INT8), 0),
metadata)
+ // Malformed containers: the final offset is missing. A container with n
children requires
+ // n + 1 offsets, so even an empty container requires one.
+ invalid(Array(arrayHeader(false, 1),
+ /* size */ 0))
+ invalid(Array(objectHeader(false, 1, 1),
+ /* size */ 0))
+ invalid(Array(arrayHeader(false, 4),
+ /* size */ 0,
+ /* truncated offset list */ 0, 0))
+ valid(Array(arrayHeader(false, 4),
+ /* size */ 0,
+ /* offset list */ 0, 0, 0, 0))
+ invalid(Array(arrayHeader(true, 1),
+ /* size */ 0, 0, 0, 0))
+ valid(Array(arrayHeader(true, 1),
+ /* size */ 0, 0, 0, 0,
+ /* offset list */ 0))
+ invalid(Array(arrayHeader(false, 1),
+ /* size */ 1,
+ /* offset list */ 0, 2,
+ /* element data: empty array without its offset list */
arrayHeader(false, 1), 0))
+
+ // Malformed containers: the final offset points past the end of the value.
+ invalid(Array(arrayHeader(false, 1),
Review Comment:
**Nit (P3):** The `(long) dataStart + dataSize > value.length` checks added
to `validateImpl` are what stop a 4-byte final offset near `Integer.MAX_VALUE`
from wrapping and passing. No case in this test reaches them: the past-end
cases here use a 1-byte final offset of 5, and the 4-byte-offset cases above
use 0 or a truncated list. If the cast is dropped later, or the comparison is
rewritten in int, `is_valid_variant` would return true again for these
malformed values, and this test would still pass.
For example, `[arrayHeader(false, 4), 0, 0xFF, 0xFF, 0xFF, 0x7F]` has
dataStart 6. In int arithmetic, `6 + 0x7FFFFFFF` wraps to a negative number, so
the bound check passes. Nesting the same bytes as an array element does not
help: the outer per-child check computes `valueSize` in int too, so it also
wraps negative and passes, and extracting that element later fails with
MALFORMED_VARIANT. Consider adding cases like:
```scala
// Malformed containers: dataStart + final offset overflows an int.
invalid(Array(arrayHeader(false, 4),
/* size */ 0,
/* offset list */ -1, -1, -1, 0x7F))
invalid(Array(objectHeader(false, 1, 4),
/* size */ 0,
/* offset list */ -1, -1, -1, 0x7F))
invalid(Array(arrayHeader(false, 1),
/* size */ 1,
/* offset list */ 0, 6,
/* element data */ arrayHeader(false, 4), 0, -1, -1, -1, 0x7F))
```
**Verification:**
- **Regression:** A top-level array, a top-level object, and an array nested
as an array element, each with a 4-byte final offset of 0x7FFFFFFF, make
is_valid_variant return false. Computing the array data-end comparison in int
arithmetic instead of long makes both array cases fail, and doing so for
objects makes the object case fail.
- **Behavior:** Every existing is_valid_variant case keeps its current
result, including the complete empty containers with 4-byte offsets and 4-byte
sizes and the variants built by parse_json.
--
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]
