fix(reading): bound how deeply a datatype may nest - #182
Open
Blackclaws wants to merge 1 commit into
Open
Conversation
A datatype message contains its base type inline, so decoding one recurses - through an array's base type, a compound's members, an enumeration's base type or a variable-length sequence's. The depth comes from the file, and nothing bounded it, so a file that nests deeply enough exhausts the stack. That is the one decoding failure a caller cannot handle: the runtime terminates the process on StackOverflowException, where every other malformed input raises something catchable. Reachable by any truncated, corrupted or hostile file, and around 1500 levels sufficed here - though the exact depth depends on the stack the reader happens to run on. H5ReadOptions.MaxDatatypeNestingDepth now bounds it, defaulting to 64: far above the handful of levels a real datatype uses, far below where the stack runs out, and raisable for a file that genuinely nests further. Exceeding it reports the limit and names the option. BREAKING CHANGE: H5ReadOptions gains a constructor parameter, so its constructor and Deconstruct signatures change. Source-compatible, but assemblies compiled against the previous version need recompiling. Co-Authored-By: Claude <noreply@anthropic.com>
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.
Decoding a datatype message is recursive: the message contains its base type inline, and
DatatypeMessage.Decodedescends into it from six places inDatatypePropertyDescriptions.cs— an array's base type, a compound's members, an enumeration's base type and a variable-length sequence's. Nothing bounded that recursion, and the depth is read from the file.A file that nests deeply enough therefore exhausts the stack. That is the one decoding failure a caller cannot handle: the runtime terminates the process on
StackOverflowException, where every other malformed input raises something catchable, so wrappingH5File.OpenReadin atry/catchis no defence. Around 1500 levels sufficed on my machine, though the exact depth depends on the stack the reader happens to run on. The nesting need not even be invalid — a well-formed chain of variable-length sequences costs 8 bytes per level, so under 16 kB of file is enough.H5ReadOptions.MaxDatatypeNestingDepthnow bounds it, defaulting to 64. That is far above the handful of levels a real datatype composes — a compound of arrays of compounds — and far below where the stack runs out. Exceeding it throwsNotSupportedExceptionnaming the option, so a file that genuinely nests further can still be read by raising the bound, and a caller reading untrusted files can lower it.The bound and the current depth are threaded through
DatatypeMessage.Decodeand the four recursing property decoders, with the check in one place on the way in. The two entry points supply it fromcontext.ReadOptions.Worth flagging
H5ReadOptionsgains a constructor parameter, so the record's constructor andDeconstructsignatures change. Source-compatible — the parameter is last and optional — but an assembly compiled against the previous version needs recompiling.Found while investigating the object header message size limit in #179 — one of the corrupt files that bug produced happened to decode into deep nesting — but the two are independent, and this reaches any malformed file rather than anything PureHDF writes.