Possible segfault if globals dict is NULL in run_eval_code_obj

[ngr-ilmarh]

Bug report

Bug description:

Possible interpreter crash condition was found by Linux Verification Center (portal.linuxtesting.ru) with SVACE.

run_eval_code_obj is called with globals dict passed as pointer without NULL check and that may cause segmentation fault in

PyEval_EvalCode->_PyEval_BuiltinsFromGlobals->PyDict_GetItemWithError line 2272 in PyDict_Check(op).

Some functions, for example, _run_script or builtin_eval_impl have globals NULL check.

Other, like PyRun_SimpleStringFlags,
PyRun_InteractiveOneObjectEx or PyRun_FileExFlags just use the pointer from the PyObject struct.

For my understanding the globals dict should always be present and it is an error condition, when it is NULL.
And no one encounted with such faults, so the patch will have cosmetic effect (is on the way).

CPython versions tested on:

3.10

Operating systems tested on:

Linux

Linked PRs

• gh-116180: check if globals is NULL and set error in run_eval_code_obj() #116182
• gh-116180: check if globals is NULL and set error in run_eval_code_obj() #116637

[NGRsoftlab]

@gpshead i've made pr against main branch, as you pointed in #116182

[erlend-aasland]

And no one encounted with such faults, so the patch will have cosmetic effect (is on the way).

It would be nice if you could come up with a reproducer, either in Python, the C API, or both.

[NGRsoftlab]

Yes, this patch has a cosmetic effect, should i mention it somehow in other issues, that will follow?

Testcase is difficult for me to offer right now, i'll try to figure it out, but with no guarantees that i succeed.

[zooba]

This looks pretty trivially reproducible - just pass globals=NULL to PyRun_FileExFlags or any of the functions that wrap it up. They all just pass through without checking.

Prior to d6c33fb there was a NULL check deeper in that raised an error. The place where it was no longer exists, but we should probably add the check closer to where it's being used.

[NGRsoftlab]

Yes, finally i've figured it out and left comment in PR thread with example and outcome.
Tried assert(), but when you pass NULL everything just segfaults with no visible notification, same behaviour without assert().

[serhiy-storchaka]

There are two ways to fix this issue:

• Make the code working with globals = NULL. The part of the code already supports this case, you should add workarounds for other parts.
• If this case should not be supported, make the code raising an exception with clear and precise error message. Why globals is NULL? If it is passed by the user of the C API, the arguments should be validated and SystemError should be passed. If it is the result of other operations, it should be checked after performing these operations, and the error message should be more specific.

You perhaps need to try both ways to determine what way is more correct.

[NGRsoftlab]

I thought that it's better to check NULL in one place, and there it's hard to figure out where NULL came from.
Ok, i'll try to make more research on the topic. Maybe you are right and it's better check NULL in C API functions directly.

[zooba]

The fix is to bring back the null check that Mark deleted in his refactoring. It used to be in _PyEval_EvalCode:

    if (globals == NULL) {
        _PyErr_SetString(tstate, PyExc_SystemError,
                         "PyEval_EvalCodeEx: NULL globals");
        return NULL;
    }

(See lines 4386-4390 of ceval.c in d6c33fb - sorry Github won't let me link directly for some reason)

[NGRsoftlab]

It seems you should check discussion in PR. Unfortunately it forked

[serhiy-storchaka]

After working on the proposed PR and tests, I think that it is not a bug. The lack of the validation in the C API is not a bug. You are supposed to pass valid arguments, and if it fails for invalid arguments, it is your fault.

Of course, it would be nice to handle the user error and raise a SystemError, but this is a new feature, not a bug fix. This API is high-level, so adding few new checks will not add much overhead.

[NGRsoftlab]

I've logged-in to check PR status and sync sources. And found out, that our changes were merged through #117968. It ruined somehow my branch with changes to run_eval_code_obj(), but finally I managed to restore changes.
Should i continue maintain fix branch for PR?