iemejia commented on code in PR #55924:
URL: https://github.com/apache/spark/pull/55924#discussion_r3925086995
##########
sql/core/src/main/java/org/apache/spark/sql/execution/datasources/parquet/VectorizedDeltaByteArrayReader.java:
##########
@@ -79,20 +84,31 @@ private void readValues(int total, WritableColumnVector c,
int rowId) {
// value of the page should have an empty prefix, it may not
// because of PARQUET-246.
int prefixLength = prefixLengthVector.getInt(currentRow);
- ByteBuffer suffix = suffixReader.getBytes(currentRow);
- byte[] suffixArray = suffix.array();
- int suffixLength = suffix.limit() - suffix.position();
+ int suffixLength = suffixReader.getSuffixLength(currentRow);
int length = prefixLength + suffixLength;
- // We have to do this to materialize the output
+ // The prefix is shared with the previously decoded value, so it can be
at most as long
+ // as that value. A larger prefixLength means the file is corrupt (e.g.
PARQUET-246 on the
+ // first value of the first page). prevBuf is pre-zeroed and reused, so
without this guard
+ // such input would silently assemble stale/zero prefix bytes instead of
failing.
+ checkPrefixLength(prefixLength);
+
+ // Grow prevBuf if needed, preserving the prefix bytes already in place.
+ if (length > prevBuf.length) {
+ byte[] newBuf = new byte[Math.max(length, prevBuf.length * 2)];
Review Comment:
Good catch -- addressed in cee762f73f2. `getSuffixLength` now validates the
length before any caller sizes `prevBuf` from it:
```java
public int getSuffixLength(int rowId) {
int length = lengthsVector.getInt(rowId);
if (length < 0) {
throw new ParquetDecodingException(
"Negative suffix length " + length + "; the DELTA_BYTE_ARRAY page is
corrupt");
}
int available = in.available();
if (length > available) {
throw new ParquetDecodingException(
"Suffix length " + length + " exceeds the " + available
+ " bytes remaining in the page; the DELTA_BYTE_ARRAY page is
corrupt");
}
return length;
}
```
As you noted, `in` is private to `VectorizedDeltaLengthByteArrayReader` and
sits exactly at this row's suffix when this is called (all three loops call
`getSuffixLength` before growing, and earlier suffixes are already consumed),
so `available()` is exact and anything larger is corrupt. This restores
master's fail-fast-before-allocating behavior (previously implicit in
`in.slice(length)`'s `EOFException`), so a corrupt page yields a plain
`ParquetDecodingException` instead of an up-to-2GB allocation /
`OutOfMemoryError`. The negative `suffixLength` lives here too; the negative
`prefixLength` now lives in `checkPrefixLength` alongside the existing upper
bound:
```java
private void checkPrefixLength(int prefixLength) {
if (prefixLength < 0 || prefixLength > prevLen) {
throw new ParquetDecodingException(
"Prefix length " + prefixLength + " is out of range [0, " + prevLen
+ "]; the DELTA_BYTE_ARRAY page is corrupt");
}
}
```
Covered by `corrupt suffix/prefix lengths fail fast without over-allocating`
(too-large suffix, negative suffix, negative prefix).
--
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]
