fix: bound untrusted GeoJSON input to prevent parser resource exhaustion#1733
Open
agilira wants to merge 1 commit into
Open
fix: bound untrusted GeoJSON input to prevent parser resource exhaustion#1733agilira wants to merge 1 commit into
agilira wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
googlemaps#1699 and googlemaps#1710 added nesting-depth guards, but they run on the tree that Json.parseToJsonElement() has already fully materialised, so they act too late: GeoJsonParser.parse() reads and materialises the whole untrusted document before any check. A hostile layer can therefore still - overflow the parser stack with deep nesting (StackOverflowError) before the MAX_GEOMETRY_DEPTH=20 guard runs (contrast KmlParser, which bounds depth *during* streaming via DepthLimitingReader); - exhaust the heap with a wide, shallow document (OutOfMemoryError), which no depth guard addresses; - forward non-finite coordinates ("Infinity"/"NaN", accepted by String.toDouble()) straight into LatLng/LatLngBounds. The first two throw java.lang.Error subclasses, so the catch (Exception) in DataLayerLoader does not contain them and the host app crashes on load. Bound the input up front, before parseToJsonElement: - readTextBounded() caps the characters read (configurable, default 10 MiB); - checkStructuralDepth() rejects raw '{'/'[' nesting beyond a limit in a single string-aware pass (configurable, default 512 - safe on small Android stacks, far above any legitimate GeoJSON); - parseCoordinates() rejects non-finite values. Limits are constructor parameters with safe defaults, mirroring KmzParser. Existing behaviour is preserved (incl. the googlemaps#1699 depth-200 GeometryCollection test); adds tests for each case.
agilira
force-pushed
the
fix/geojson-parser-resource-exhaustion
branch
from
July 25, 2026 15:36
0e1e861 to
7a73969
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Harden
GeoJsonParseragainst resource exhaustion when parsing untrusted GeoJSON, by bounding the input before it is materialised. This closes the remaining gap left by the nesting-depth guards added in #1699 and #1710.Problem
#1699 (
MAX_GEOMETRY_DEPTH) and #1710 both guard against deeply nested input, but the GeoJSON guard runs during a second pass over the tree thatJson.parseToJsonElement()has already fully materialised:So for GeoJSON the guard acts too late. With an untrusted document a caller can still hit:
StackOverflowError.kotlinx-serialization's JSON reader recurses on nested arrays, so a few thousand nested arrays overflow the stack insideparseToJsonElement, beforeMAX_GEOMETRY_DEPTHis ever reached. (KmlParseravoids this by bounding depth during streaming withDepthLimitingReader; GeoJSON has no equivalent.)OutOfMemoryError. A large flat array (e.g. aMultiPointwith millions of positions) materialises to far more heap than the source text, with no nesting for a depth guard to catch."Infinity"/"NaN"are accepted byString.toDouble()and currently flow straight intoLatLng/LatLngBounds.The first two throw
java.lang.Errorsubclasses, so thecatch (e: Exception)inDataLayerLoaderdoes not contain them and the failure propagates to the host app.Fix
Bound the input up front, before
parseToJsonElement():readTextBounded()— caps the number of characters read (configurable, default 10 MiB), so an oversized document is never fully materialised.checkStructuralDepth()— rejects raw{/[nesting beyond a limit, in a single string-aware pass (configurable, default512— safe on small Android thread stacks, and far above any legitimate GeoJSON, whose structural depth stays well under 100 even with maximally nested geometries).parseCoordinates()— rejects non-finite values.Both limits are constructor parameters with safe defaults, mirroring
KmzParser's configurable zip-bomb limits (#1677). Rejected input throwsIllegalArgumentException(anException), soDataLayerLoaderdegrades to anulllayer instead of crashing.Testing
Adds unit tests for each case (deep-nested arrays, excessively nested
GeometryCollection, oversized input, non-finite coordinate). Existing behaviour is preserved, including the #1699testDeeplyNestedGeometryCollectionDoesNotThrowStackOverflowcase (depth 200 stays well within the structural limit and still parses).Compatibility
GeoJsonParser's new parameters have defaults, soGeoJsonParser()continues to work unchanged for all existing (Kotlin) call sites (DataLayerLoader,GeoJsonLayer). No@JvmOverloadsis added, matching the siblingKmzParser.