nest-server: BadRequest: 400 Bad Request: name '<function name>' is not defined

[med-ayssar]

Fixes #2666

exec(script: string|bytes, globals: dict, locals: dict):

# script.py
def foo():  # stored in locals
    pass

def bar(): # stored in locals
    foo()  # is it in `bar.__locals__`?, if yes then ok, found it, 
           # if not, seach in bar._globals__ (same as the provided global), if not found, then search in __builtin__  
           # --> result `foo` not found

here, we must set locals to globals, to be able to call the defined functions within other defined functions.

[heplesser]

@med-ayssar Thanks for the PR! Have you confirmed that the problem still exists with NEST 3.9? It was originally reported against 3.4 and a number of things have happened in NEST Server since then, I believe.

Adding @Helveg as a reviewer who has discovered security issues in the server earlier, as I am concerned about potential risks in making globals to locals.

@babsey I started NEST server on my computer (Auth disabled) and confirmed that I can connect to it. But when I then try to run a script from file, I just get the content of the file displayed and response is None.

from nest_client import NESTClient
In [12]: response = nc.from_file('/Users/plesser/NEST/code/src/main/pynest/examples/brunel_alpha_nest.py', return_vars='num_synapses')
Execute script code of /Users/plesser/NEST/code/src/main/pynest/examples/brunel_alpha_nest.py
Return variables: num_synapses
--------------------
...

What am I doing wrong?

[med-ayssar]

Yeah, I tested it yesterday with the current master, and it did not work.

The globals are anyway provided to the function.

To make it work, you need to set ( Export ) one environment variable, check the do_exec function in nest-server, otherwise, I will edit my comment once I start my laptop.

[Helveg]

globals/locals's namespace interactions with exec are a known pain-point with undefined behavior, rectified only in PEP667 since Python 3.13.

Example:

>>> source = """
... def y():
...     return 5
... 
... def z():
...     return y()
... 
... print(z())
... """
>>> exec(source)
5
>>> exec(source, {})
5
>>> exec(source, {}, {})
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<string>", line 8, in <module>
  File "<string>", line 6, in z
NameError: name 'y' is not defined

Which is explained by the note in the docs on exec here:

Note: When exec gets two separate objects as globals and locals, the code will be executed as if it were embedded in a class definition. This means functions and classes defined in the executed code will not be able to access variables assigned at the top level (as the “top level” variables are treated as class variables in a class definition).

The solution proposed by @med-ayssar is essentially summarized as follows:

globals_ = get_restricted_globals()  # Create a safe dictionary
locals_ = globals_
exec(source, globals_, locals_)

which is actually equivalent to exec(source, globals_, globals_) which avoids the noted "2 different object" condition, but which is actually also equivalent to simply exec(source, globals_).

So why are we even passing in locals_ at all? This is a historically relatively widespread trick to "fish out" variables from an exec statement without polluting the current namespace, and in our source code looks like this:

globals_ = ...
locals_ = {}
exec(source, globals_, locals_)
response["stdout"] = "".join(locals_["_print"].txt)

The purpose of this seems to be to capture stdout while inside of the RestrictedPython environment. The pattern if I had to guess would be phylogenetically related to the also relatively widespread (ab)use of:

exec("x = 5") # which is equivalent to exec(code, globals(), locals())
print(locals()["x"]) # prints 5: `exec(code)` occurs within current namespace, and would mutate/pollute it.

this breaks in Python 3.13 after the change of the semantics (see python/cpython#118888).

Given all this information, I think the safest and clearest way to achieve the desired behavior here is to simply not create any locals_ dict, and fish it out from the globals_ we pass in, which is both semantically correct (since _print gets defined globally in the exec namespace), avoids redundancy, and works both before and after Python 3.13:

def do_exec(args, kwargs):
    source_code = kwargs.get("source", "")
    source_cleaned = clean_code(source_code)

    response = dict()
    if RESTRICTION_DISABLED:
        with Capturing() as stdout:
            globals_ = globals().copy()
            globals_.update(get_modules_from_env())
            get_or_error(exec)(source_cleaned, globals_)
        if len(stdout) > 0:
            response["stdout"] = "\n".join(stdout)
    else:
        code = RestrictedPython.compile_restricted(source_cleaned, "<inline>", "exec")  # noqa
        globals_ = get_restricted_globals()
        globals_.update(get_modules_from_env())
        get_or_error(exec)(code, globals_)
        if "_print" in globals_:
            response["stdout"] = "".join(globals_["_print"].txt)

The only possible change that might occur compared to the current source code (which is also a caveat of @med-ayssar's proposal) is if _print were already defined in the globals_ we pass in for some reason? Most likely not the case though.

[Helveg]

See my discourse in the conversation. The last code block is my suggested change, but I can't propose it here because it involves changing things outside of the code range I can propose suggestions to in the Review tab.

[babsey]

I agree with @Helveg. As he suggested that you just remove locals_ from exec and take variables from globals_ for the response.

Let review simple samples:

exec("def foo(): print('hello')\n\ndef bar(): foo()\n\nbar()", {})

and with locals:

exec("def foo(): print('hello')\n\ndef bar(): foo()\n\nbar()", {}, {})

To me, it looks that locals arg in exec function causes this issue #2666.

[github-actions]

Pull request automatically marked stale!

[babsey]

@ayssar100 Friendly ping!

[babsey]

Remove locals_ variable at line 195 and rename locals_ to globals_ at lines 215 - 218

[med-ayssar]

Sorry, here you go!

[med-ayssar]

@ayssar100 Friendly ping!

I wonder how did you get that account ID there.

[babsey]

@ayssar100 Friendly ping!

I wonder how did you get that account ID there.

I apologize, I wrote your last name and took the first suggestion without checking it.

[babsey]

@med-ayssar I tried to create a PR to your branch issue-2666.
But I am not able to find your base repository on PR create.

So, I write here what you need to change these lines before I give a LGTM! sign.

1. Remove locals_ = dict() at line 195.
2. Rename all locals_ to globals_ in lines 208 - 215.

[med-ayssar]

It should be correct now.

[Helveg]

I think this still does not do what reviewers agreed on:

• locals_ on line 195 should be removed
• The get_or_error(exec)(code, globals_, locals_) calls should be changed to get_or_error(exec)(code, globals_)

[babsey]

LGTM!

Thank you for the fix.
It works with function in nest-server exec call.

[babsey]

@heplesser @Helveg Friendly ping!

[Helveg]

main needs to be merged for the macos build to succeed. (i think, @heplesser sent an email on this)

[med-ayssar]

main needs to be merged for the macos build to succeed. (i think, @heplesser sent an email on this)

Done

[babsey]

Friendly ping!

[babsey]

Friendly ping!

Why is it still open? 2 reviewers approved the changes.