Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

starlarkstruct Freeze() can cause a stack overflow if circular references are involved #566

Closed
oprypin opened this issue Nov 25, 2024 · 2 comments · Fixed by #567
Closed

Comments

@oprypin
Copy link
Member

oprypin commented Nov 25, 2024

If starlarkstruct.Struct is exposed to the starlark environment, and one creates a struct with a circular reference, and causes it to be frozen, then there will be a stack overflow.

A module with this Starlark content should reproduce the issue, but I haven't double-checked that it actually does:

def make_struct():
    items = []
    foo = struct(items=items)
    foo.items.append(foo)
    return foo

toplevel = make_struct()

A fix for this issue could be done as follows:

--- a/starlarkstruct/struct.go
+++ b/starlarkstruct/struct.go
@@ -99,6 +99,7 @@ func FromStringDict(constructor starlark
 type Struct struct {
        constructor starlark.Value
        entries     entries // sorted by name
+       frozen      bool
 }
 
 // Default is the default constructor for structs.
@@ -172,8 +173,11 @@ func (s *Struct) Hash() (uint32, error) 
        return x, nil
 }
 func (s *Struct) Freeze() {
-       for _, e := range s.entries {
-               e.value.Freeze()
+       if !s.frozen {
+               s.frozen = true
+               for _, e := range s.entries {
+                       e.value.Freeze()
+               }
        }
 }
@oprypin oprypin changed the title starlarkstruct Freeze() can cause a stack overflow if ciircular references are involved starlarkstruct Freeze() can cause a stack overflow if circular references are involved Nov 25, 2024
@adonovan
Copy link
Collaborator

Thanks, that fix is correct. The Freeze function should give better guidance about setting the flag before descending for precisely this reason. Do you want to send a PR?

@oprypin
Copy link
Member Author

oprypin commented Nov 25, 2024

I'll try to make a PR, hopefully creating a test will not be too hard :)

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 a pull request may close this issue.

2 participants