Repository navigation
fix(data): reject impossible FlatGeobuf index sizes - #965
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Validate computed index size crates/data/src/source/fgb.rs |
A shared reader opener checks whether the advertised spatial index size can be computed and converted to usize. It returns a format error when either check fails. |
Apply validation to reader entry points crates/data/src/source/fgb.rs, crates/data/src/source/tests.rs |
Source creation, local reads, geometry probing, and remote reads use the shared opener. Tests verify format errors for oversized feature counts across byte, layer-list, path, reopened-path, and remote entry points. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Possibly related issues
opengeos/RustGISissue 779: The change rejects one malformed FlatGeobuf input before index operations. It does not add the issue’s proposed fuzzing infrastructure.
Suggested reviewers: giswqs
Merge Risk: 
3223c
-
The described reader paths reject impossible index sizes with a format error. No additional issue is established that needs resolution before merging.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 45.45% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title "fix(data): reject impossible FlatGeobuf index sizes" matches the main change. The diff adds an open_readervalidation in fgb.rs that rejects FlatGeobuf files whose advertised spatial inde…Description check The description follows the template's three sections. Summary explains the problem (an indexed header with an overflowing feature count panics in select_all), the validation added, and the scope (not… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title "fix(data): reject impossible FlatGeobuf index sizes" matches the main change. The diff adds an
open_readervalidation in fgb.rs that rejects FlatGeobuf files whose advertised spatial index size cannot be represented, and adds a regression test for impossible feature counts. The title is specific, concise, and follows a conventional-commit style.
Full details: Description check
-
Explanation
The description follows the template's three sections. Summary explains the problem (an indexed header with an overflowing feature count panics in select_all), the validation added, and the scope (not closing the broader fuzzing proposal under
#779). Testing lists the regression test, cargo test for the flatgeobuf module, cargo xtask ci, pre-commit, and a wasm32 build. Related issue states "Part of#779." The description is thorough and specific. Minor note: the Testing section lists commands and results but does not include a screenshot or manual steps, which the template does not require for this change.
-
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks each index size,
And spots the counts that grow too wide.
The reader stops with errors clear,
While tests check paths both far and near.
Then hops away with files in gear.
Comment @coderabbitai help to get the list of available commands.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| let bytes = level_bounds(h.features_count(), u64::from(h.index_node_size())) | ||
| // The dependency includes a root even when there is just one leaf. | ||
| .and_then(|levels| levels.first().and_then(|(_, end)| (*end).max(2).checked_mul(NODE))); | ||
| if bytes.and_then(|n| usize::try_from(n).ok()).is_none() { |
There was a problem hiding this comment.
Medium confidence. On wasm32 this branch also rejects valid but very large indexes (roughly 100M+ features, where the index passes 4 GiB), and reports them as "an impossible spatial index size". Those files aren't impossible; they just don't fit a 32-bit address space. The message should say that, e.g. "FlatGeobuf: its spatial index is too large for this platform", with the impossible-count wording reserved for the level_bounds None case. Also, bytes.and_then(|n| usize::try_from(n).ok()).is_none() reads awkwardly. This is clearer, and is_none_or is already used in this file:
| if bytes.and_then(|n| usize::try_from(n).ok()).is_none() { | |
| if bytes.is_none_or(|n| usize::try_from(n).is_err()) { |
| let slot = header._tab.vtable().get(flatgeobuf::Header::VT_FEATURES_COUNT) as usize; | ||
| assert_ne!(slot, 0); | ||
| let offset = 8 + header._tab.loc() + slot; | ||
| for count in [1u64 << 63, u64::MAX, (1u64 << 40) + 1] { |
There was a problem hiding this comment.
High confidence on the gap. All three counts (2^63, u64::MAX, 2^40 + 1) are rejected by the items > 1 << 40 cap in level_bounds. That means the new usize::try_from check, which the PR description says covers wasm32, and the single-leaf max(2) root path are never exercised by this regression test. Suggest pulling the size check into a small helper that takes the byte limit, e.g. fn index_fits(bytes: u64, max: u64) -> bool, and unit-testing it with max = u32::MAX as u64 so the 32-bit branch is covered on 64-bit CI.
Code reviewBugs
Security
Performance
Quality
AGENTS.md
Change sizeCompared with
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
📖 Docs preview
GitHub Pages takes a minute or two to publish a new commit. |
Summary
An indexed FlatGeobuf header can advertise a feature count that overflows
flatgeobuf's packed R-tree size calculation. A generated file with its count changed to2^63panics duringselect_allinstead of returning a read error.Validate each opened reader before index operations, using the existing checked level calculation and its feature-count bound, then checking that the index byte size fits
usize(including wasm32). Impossible indexes now return a format error. Validation also runs when a local path is reopened, during geometry-type detection, and when the remote reader is opened over fetched ranges.This is a focused reader fix under #779; it does not implement or close the broader fuzzing proposal. No new dependency or format change.
Testing
cargo test -p rustgis-data flatgeobuf: 5 passed, including the new malformed-count regression and existing local/range-backed round trips.cargo xtask ci: 2,406 passed, 0 failed, 16 ignored; layers passed.uvx pre-commit run --all-filescargo build --locked --target wasm32-unknown-unknown -p rustgis-webRelated issue
Part of #779.
Summary by CodeRabbit