dongjoon-hyun commented on PR #58598:
URL: https://github.com/apache/spark/pull/58598#issuecomment-5581098500
Thank you for working on this. The underlying problem is real -- I
reproduced the `StackOverflowError` escaping `CatalystSqlParser.parseDataType`.
However, I believe **the depth check as implemented can be trivially
bypassed**, so the patch does not actually provide the protection it describes.
### The check is bypassable
`parseCatalystType` scans raw characters rather than tokens. But a struct
field name may be a backquoted identifier (`complexColType:
colName=errorCapturingIdentifier dataType ...`), and a backquoted identifier
can contain `>`. So inserting one fake `>` per nesting level keeps the counter
flat.
I built `sql-api` and ran the real `DataTypeParser` against a replica of
this PR's counting logic:
```
sample = struct<`>`:struct<`>`:struct<`>`:int>>>
counted max = 1 (real nesting = 3)
parses to = STRUCT<`>`: STRUCT<`>`: STRUCT<`>`: INT>>>
n=500 len=6003 counted max depth = 1 -> *** StackOverflowError
escaped parseDataType ***
n=2000 len=24003 counted max depth = 1 -> *** StackOverflowError
escaped parseDataType ***
n=10000 len=120003 counted max depth = 1 -> *** StackOverflowError
escaped parseDataType ***
```
A type nested 10,000 levels deep passes the check with a counted depth of
**1**, and blows the stack on a 1MB-stack thread. No value of
`catalystTypeParsingMaxDepth` stops it.
### The check also has false positives
The same token-vs-character mismatch rejects legitimate types:
```
struct<a:int COMMENT 'a<<<<<<<<<<b'> -> counted=11, actual nesting=1
struct<`<<<<<<<<<<`:int> -> counted=11, actual nesting=1
array<decimal(10,2)> -> counted=2, actual nesting=1
('(' counts as a level)
```
Counting `(` means `decimal(p,s)` / `varchar(n)` inflate the measured depth,
so the number the config takes does not match the "nesting depth" the doc
describes.
### Other notes
- **Default `-1` plus `internal()`**: as written this ships no protection by
default and is undocumented. Worth noting that the values Spark itself stamps
into `spark.sql.catalyst.type` are all shallow (`char(n)`, `varchar(n)`,
`time(6)`, `timestamp_ltz_ns(9)`), so a correct check could be enabled by
default at a low limit with essentially no compatibility risk.
- **No `checkValue`**: `0` and any other negative value silently mean
"disabled" as well. Something like `.checkValue(v => v > 0 || v == -1, ...)`
would make the contract explicit.
- **Doc/comment scope**: both say "during schema inference" and "driver
stack", but `parseCatalystType` also runs on executors via `AvroDeserializer`
(`SchemaConverters.toSqlType` at `AvroDeserializer.scala:145`).
- **Test coverage**: the new test only exercises a depth-8 `array<...>`. It
does not assert that an input which actually overflows the stack is rejected,
does not cover the backquote bypass, and does not cover the
`parseStampedStringType` / map-key-type paths. Adding the `struct<`>`:...>`
string above to the test makes the current implementation fail immediately.
### Suggestion
Character counting effectively re-implements the grammar, and this class of
bypass tends to keep reappearing. Simpler and more robust options:
1. **Catch `StackOverflowError` and convert it.** Three lines, no config, no
heuristic, no bypass, no false positives. There is precedent in the codebase
already -- `AbstractSqlParser.withErrorHandling` and `SqlStatementSplitter`
both do this. Note `AbstractParser.parse` only catches `ParseException` and
friends, and the SOE-catching `withErrorHandling` is not on the `parseDataType`
path, which is why the error escapes today.
2. **Put the depth guard in the parser itself.** The same exposure exists
wherever untrusted text reaches `parseDataType` -- `from_json` DDL schema
strings, JDBC/metastore schema strings, and so on. Fixing it only for Avro is
quite narrow.
3. If a pre-check is still preferred, **bound the string length** rather
than the depth. Length is a hard upper bound on nesting and cannot be gamed by
the content.
--
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]
