Skip to content

fix(reading): bound how deeply a datatype may nest - #182

Open
Blackclaws wants to merge 1 commit into
Apollo3zehn:devfrom
Blackclaws:fix/datatype-decode-recursion-depth
Open

fix(reading): bound how deeply a datatype may nest#182
Blackclaws wants to merge 1 commit into
Apollo3zehn:devfrom
Blackclaws:fix/datatype-decode-recursion-depth

Conversation

@Blackclaws

Copy link
Copy Markdown
Contributor

Decoding a datatype message is recursive: the message contains its base type inline, and DatatypeMessage.Decode descends into it from six places in DatatypePropertyDescriptions.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 wrapping H5File.OpenRead in a try/catch is 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.MaxDatatypeNestingDepth now 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 throws NotSupportedException naming 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.Decode and the four recursing property decoders, with the check in one place on the way in. The two entry points supply it from context.ReadOptions.

Worth flagging

  • H5ReadOptions gains a constructor parameter, so the record's constructor and Deconstruct signatures change. Source-compatible — the parameter is last and optional — but an assembly compiled against the previous version needs recompiling.
  • The default is a policy choice. 64 is generous for composed datatypes but it is a cap where there was none, so a file nesting deeper than 64 that previously decoded will now be refused until the option is raised. I could not construct such a file with PureHDF's own writer, and did not find one in the test corpus.
  • The limit is deliberately not derived from where the stack actually runs out, since that varies with stack size and build configuration.

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.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant