Segfault when functions access objects outside closure

[brandonchinn178]

#232 added support for Python functions, but it doesn't properly capture objects within the closure.

from starlark_go import *

def global_foo(*args):
    print(args)

# works
Starlark(globals={"foo": global_foo}).exec("foo()")
Starlark(globals={"foo": global_foo}).exec("foo(1)")
Starlark(globals={"foo": global_foo}).exec("foo(1,2)")

def mk_foo():
    def foo(*args):
        print(args)
    return foo

# segfaults
Starlark(globals={"foo": mk_foo()}).exec("foo()")
Starlark(globals={"foo": mk_foo()}).exec("foo(1)")
Starlark(globals={"foo": mk_foo()}).exec("foo(1,2)")

Probably not easy to fix, but at least we can track the issue here.

Workaround is to add the objects to some global state, e.g.

_FOOS=[]
def mk_foo():
    def foo(*args):
        print(args)
    _FOOS.append(foo)
    return foo

[colindean]

Thanks for reporting this!

To clarify my understanding of your post, does your example work if the mk_foo function is replaced with the function and global declaration in the workaround?

[jordemort]

Haven't looked at the code but my guess is we want to Py_INCREF everything in globals at the top of an exec and Py_DECREF everything on the way out

[jordemort]

After looking at it, it looks like everything but functions is converted all the way through, so holding a reference shouldn't be necessary for those things. I think the conversions of functions and methods need to increment the refcount of obj/f respectively and then somehow clean that up when they are no longer reachable via globals.

[brandonchinn178]

To clarify my understanding of your post, does your example work if the mk_foo function is replaced with the function and global declaration in the workaround?

@colindean Yes, this works:

_FOOS=[]
def mk_foo():
    def foo(*args):
        print(args)
    _FOOS.append(foo)
    return foo

# works
Starlark(globals={"foo": mk_foo()}).exec("foo()")
Starlark(globals={"foo": mk_foo()}).exec("foo(1)")
Starlark(globals={"foo": mk_foo()}).exec("foo(1,2)")

After looking at it, it looks like everything but functions is converted all the way through, so holding a reference shouldn't be necessary for those things. I think the conversions of functions and methods need to increment the refcount of obj/f respectively and then somehow clean that up when they are no longer reachable via globals.

@jordemort Makes sense! I'll try to get around to putting up a PR