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

Some builtin single phase modules do get loaded into subinterpreters which do not support them #119450

Closed
sebsura opened this issue May 23, 2024 · 3 comments
Labels
topic-subinterpreters type-bug An unexpected behavior, bug, or error

Comments

@sebsura
Copy link

sebsura commented May 23, 2024

Bug description:

If a subinterpreter is started with

PyInterpreterConfig cfg = {
...
    .check_multi_interp_extensions = 1,
};
Py_NewInterpreterFromConfig(&ts, &cfg);

I expect that no single phase module is allowed to be loaded. This works for all self build modules and with most builtin ones, but not all. Some modules i found to be misbehaving are e.g. datetime, tracemalloc.

This can easily cause a crash:

#include <Python.h>
#include <assert.h>

// make subinterpreter from config
PyThreadState* make_sub(PyInterpreterConfig* config)
{
  PyThreadState* sub = NULL;
  PyStatus status = Py_NewInterpreterFromConfig(&sub, config);

  if (PyStatus_Exception(status)) {
    return NULL;
  }

  return sub;
}

// create an isolated subinterpreter, then load a module with name 'name'
void sub_import(PyThreadState* main, const char* name)
{
  PyEval_AcquireThread(main);

  PyInterpreterConfig config = {
      .use_main_obmalloc = 0,
      .allow_fork = 0,
      .allow_exec = 0,
      .allow_threads = 1,
      .allow_daemon_threads = 0,
      .check_multi_interp_extensions = 1,
      .gil = PyInterpreterConfig_OWN_GIL,
  };

  PyThreadState* sub = make_sub(&config);
  assert(sub);

  PyObject* module = PyImport_ImportModule(name);
  
  printf("Loaded: %s\n", module ? "yes" : "no");

  Py_XDECREF(module);

  Py_EndInterpreter(sub);
}

int main()
{
  Py_InitializeEx(0);

  PyThreadState* interp = PyEval_SaveThread();

  const char* module_name = "datetime";

  sub_import(interp, module_name);
  sub_import(interp, module_name); // <- crash

  PyEval_AcquireThread(interp);
  Py_FinalizeEx();
}

This is because the module will get loaded a second time, which will cause python to try and cleanup the old state from the old interpreter inside the new interpreter.

// obmalloc.c
void
_PyObject_Free(void *ctx, void *p) // p will point to an object allocated by pymalloc from the old interpreter here
{
    /* PyObject_Free(NULL) has no effect */
    if (p == NULL) {
        return;
    }

    OMState *state = get_state();
    if (UNLIKELY(!pymalloc_free(state, ctx, p))) { // <- this returns false, since the object does not belong to the new interpreter
        /* pymalloc didn't allocate this address */
        PyMem_RawFree(p); // <- this will crash, since the pointer was not allocated directly through malloc
        raw_allocated_blocks--;
    }
}

This may be related to #117953 and #104621 but its important to note that this does not happen with the readline module as it correctly refuses to load.

CPython versions tested on:

3.12

Operating systems tested on:

Linux

@sebsura sebsura added the type-bug An unexpected behavior, bug, or error label May 23, 2024
@sebsura
Copy link
Author

sebsura commented May 23, 2024

The deallocation is caused by this:

// _tetimemodule.c:6755
static int
_datetime_exec(PyObject* module)
{
    ...
    PyObject *d = PyDateTime_DeltaType.tp_dict; // <- this is a global variable
    DATETIME_ADD_MACRO(d, "resolution", new_delta(0, 0, 1, 0));

@ericsnowcurrently
Copy link
Member

This should be resolved now for 3.13+, as of gh-121060. Sadly, the fix is a bit too complex to backport to 3.12.

@ericsnowcurrently ericsnowcurrently added the pending The issue will be closed if no feedback is provided label Jul 8, 2024
@sebsura
Copy link
Author

sebsura commented Jul 9, 2024

Thanks for the heads up. I can now run the sample code i gave above without crashing!

@sebsura sebsura closed this as completed Jul 9, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in Subinterpreters Jul 9, 2024
@erlend-aasland erlend-aasland removed the pending The issue will be closed if no feedback is provided label Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-subinterpreters type-bug An unexpected behavior, bug, or error
Projects
Status: Done
Development

No branches or pull requests

4 participants