Detect self-referencing values instead of crashing with a stack overflow - #1007
Open
pujitha24 wants to merge 1 commit into
Open
Detect self-referencing values instead of crashing with a stack overflow#1007pujitha24 wants to merge 1 commit into
pujitha24 wants to merge 1 commit into
Conversation
Motivation: A Starlark list or dict that (directly or indirectly) contains itself made ytt's Starlark-to-Go value conversion recurse forever, aborting the process with an unrecoverable Go runtime "fatal error: stack overflow" after allocating up to 1GB of stack. This has no ytt-level error message and exit code 2, and because it is a Go runtime fatal error (not a panic), it cannot be recovered by an embedding program using ytt as a library. This affects YAML/text template rendering as well as @ytt:json's json.encode() and @ytt:yaml's yaml.encode(), all of which route through the same conversion path. Approach: pkg/template/core/starlark_value.go's asInterface/dictAsInterface/ itearableAsInterface now thread a path argument through the recursive conversion, recording the *starlark.Dict and *starlark.List values currently being converted (i.e. the ancestors in the current recursion). Encountering a dict or list already present in path now returns a normal ytt error instead of recursing further. Only Dict and List are checked: starlark.Tuple is immutable after construction, *starlark.Set can only hold hashable elements so it can never hold a Dict/List/itself, and ytt's own *StarlarkStruct is built once from a fixed map with no field-mutation exposed to Starlark code, so none of those three can participate in a cycle. This mirrors the cycle detection already used by the vendored starlark-go library itself in starlark/value.go's writeValue/pathContains (used by str() on Starlark values), which uses the same append(path, x) idiom and also only checks List and Dict. The remainder of the diff (extracting scalarAsInterface, dictItemAsInterface, and extendPathWithList, wrapping long function signatures, and a dictItemKeyValueLen constant) is a mechanical, behavior-preserving split of the existing type switch, needed to stay under this repo's golangci-lint (revive, enable-all-rules) complexity and line-length limits once the path parameter touched those lines. Validation: - go build ./... succeeds. - go test ./... passes for the whole repo, including a new test, TestSelfReferencingValueReturnsError in pkg/cmd/template/cmd_test.go, covering a direct self-referencing list, a direct self-referencing dict, and an indirect list->dict-> list cycle, using the exact repro from the issue. Confirmed this test fails before the fix: with only the fix reverted (via git stash on starlark_value.go), the list subtest reproduces the actual "fatal error: stack overflow" crash from the issue; restoring the fix makes all subtests pass. - golangci-lint run ./... (v2.12.2, matching the version pinned in .github/workflows/golangci-lint.yml) reports 0 issues. - Manually ran the built ytt binary against the issue's exact repro (self-referencing list and dict), against @ytt:json's json.encode() on a self-referencing list, and against a non-cyclic case where the same dict value is legitimately shared by reference at multiple points in the output, to confirm sharing (as opposed to a cycle) is still converted correctly and not falsely flagged. Report: carvel-dev#1006 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: claude-sonnet-5 (via Claude Code)
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.
Motivation:
A Starlark list or dict that (directly or indirectly) contains itself
made ytt's Starlark-to-Go value conversion recurse forever, aborting
the process with an unrecoverable Go runtime "fatal error: stack
overflow" after allocating up to 1GB of stack. This has no ytt-level
error message and exit code 2, and because it is a Go runtime fatal
error (not a panic), it cannot be recovered by an embedding program
using ytt as a library. This affects YAML/text template rendering as
well as @ytt:json's json.encode() and @ytt:yaml's yaml.encode(), all
of which route through the same conversion path.
Approach:
pkg/template/core/starlark_value.go's asInterface/dictAsInterface/
itearableAsInterface now thread a path argument through the recursive
conversion, recording the *starlark.Dict and *starlark.List values
currently being converted (i.e. the ancestors in the current
recursion). Encountering a dict or list already present in path now
returns a normal ytt error instead of recursing further. Only Dict
and List are checked: starlark.Tuple is immutable after construction,
*starlark.Set can only hold hashable elements so it can never hold a
Dict/List/itself, and ytt's own *StarlarkStruct is built once from a
fixed map with no field-mutation exposed to Starlark code, so none of
those three can participate in a cycle. This mirrors the cycle
detection already used by the vendored starlark-go library itself in
starlark/value.go's writeValue/pathContains (used by str() on
Starlark values), which uses the same append(path, x) idiom and also
only checks List and Dict.
The remainder of the diff (extracting scalarAsInterface,
dictItemAsInterface, and extendPathWithList, wrapping long function
signatures, and a dictItemKeyValueLen constant) is a mechanical,
behavior-preserving split of the existing type switch, needed to stay
under this repo's golangci-lint (revive, enable-all-rules) complexity
and line-length limits once the path parameter touched those lines.
Validation:
TestSelfReferencingValueReturnsError in
pkg/cmd/template/cmd_test.go, covering a direct self-referencing
list, a direct self-referencing dict, and an indirect list->dict->
list cycle, using the exact repro from the issue. Confirmed this
test fails before the fix: with only the fix reverted (via git
stash on starlark_value.go), the list subtest reproduces the actual
"fatal error: stack overflow" crash from the issue; restoring the
fix makes all subtests pass.
.github/workflows/golangci-lint.yml) reports 0 issues.
(self-referencing list and dict), against @ytt:json's json.encode()
on a self-referencing list, and against a non-cyclic case where the
same dict value is legitimately shared by reference at multiple
points in the output, to confirm sharing (as opposed to a cycle) is
still converted correctly and not falsely flagged.
Report: #1006
Signed-off-by: Pujitha Paladugu 10557236+pujitha24@users.noreply.github.com
Assisted-by: claude-sonnet-5 (via Claude Code)
Fixes #1006