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.

Sign up for GitHub

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

Jump to bottom

bpo-28254: _posixsubprocess uses PyGC_Enable/PyGC_Disable #25693

Merged
merged 2 commits into from
Apr 28, 2021
Merged

bpo-28254: _posixsubprocess uses PyGC_Enable/PyGC_Disable #25693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
498f09f
bpo-28254: _posixsubprocess uses PyGC_Enable/PyGC_Disable
vstinner Apr 28, 2021
db214fe
Update test_subprocess
vstinner Apr 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions Lib/test/test_subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2147,8 +2147,6 @@ def raise_it():
def test_preexec_gc_module_failure(self):
# This tests the code that disables garbage collection if the child
# process will execute any Python.
def raise_runtime_error():
raise RuntimeError("this shouldn't escape")
enabled = gc.isenabled()
orig_gc_disable = gc.disable
orig_gc_isenabled = gc.isenabled
Expand All @@ -2165,16 +2163,6 @@ def raise_runtime_error():
subprocess.call([sys.executable, '-c', ''],
preexec_fn=lambda: None)
self.assertTrue(gc.isenabled(), "Popen left gc disabled.")

gc.disable = raise_runtime_error
self.assertRaises(RuntimeError, subprocess.Popen,
[sys.executable, '-c', ''],
preexec_fn=lambda: None)

del gc.isenabled # force an AttributeError
self.assertRaises(AttributeError, subprocess.Popen,
[sys.executable, '-c', ''],
preexec_fn=lambda: None)
finally:
gc.disable = orig_gc_disable
gc.isenabled = orig_gc_isenabled
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

saving and restoring these two with the try: finally: seems no longer necessary now that they're not being monkeypatched out to cause errors. (harmless, but anyone reading this will wonder why...)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you're right: I created #25709

Expand Down
116 changes: 5 additions & 111 deletions Modules/_posixsubprocess.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,47 +69,8 @@

#define POSIX_CALL(call) do { if ((call) == -1) goto error; } while (0)

typedef struct {
PyObject* disable;
PyObject* enable;
PyObject* isenabled;
} _posixsubprocessstate;

static struct PyModuleDef _posixsubprocessmodule;

static inline _posixsubprocessstate*
get_posixsubprocess_state(PyObject *module)
{
void *state = PyModule_GetState(module);
assert(state != NULL);
return (_posixsubprocessstate *)state;
}

/* If gc was disabled, call gc.enable(). Ignore errors. */
static void
_enable_gc(int need_to_reenable_gc, PyObject *gc_module, _posixsubprocessstate *state)
{
PyObject *result;
PyObject *exctype, *val, *tb;

if (need_to_reenable_gc) {
PyErr_Fetch(&exctype, &val, &tb);
result = PyObject_CallMethodNoArgs(
gc_module, state->enable);
if (result == NULL) {
/* We might have created a child process at this point, we
* we have no good way to handle a failure to reenable GC
* and return information about the child process. */
PyErr_Print();
}
Py_XDECREF(result);
if (exctype != NULL) {
PyErr_Restore(exctype, val, tb);
}
}
}


/* Convert ASCII to a positive int, no libc call. no overflow. -1 on error. */
static int
_pos_int_from_ascii(const char *name)
Expand Down Expand Up @@ -780,7 +741,6 @@ subprocess_fork_exec(PyObject *module, PyObject *args)
Py_ssize_t arg_num, num_groups = 0;
int need_after_fork = 0;
int saved_errno = 0;
_posixsubprocessstate *state = get_posixsubprocess_state(module);

if (!PyArg_ParseTuple(
args, "OOpO!OOiiiiiiiiiiOOOiO:fork_exec",
Expand Down Expand Up @@ -820,30 +780,7 @@ subprocess_fork_exec(PyObject *module, PyObject *args)

/* We need to call gc.disable() when we'll be calling preexec_fn */
if (preexec_fn != Py_None) {
PyObject *result;

gc_module = PyImport_ImportModule("gc");
if (gc_module == NULL)
return NULL;
result = PyObject_CallMethodNoArgs(
gc_module, state->isenabled);
if (result == NULL) {
Py_DECREF(gc_module);
return NULL;
}
need_to_reenable_gc = PyObject_IsTrue(result);
Py_DECREF(result);
if (need_to_reenable_gc == -1) {
Py_DECREF(gc_module);
return NULL;
}
result = PyObject_CallMethodNoArgs(
gc_module, state->disable);
if (result == NULL) {
Py_DECREF(gc_module);
return NULL;
}
Py_DECREF(result);
need_to_reenable_gc = PyGC_Disable();
}

exec_array = _PySequence_BytesToCharpArray(executable_list);
Expand Down Expand Up @@ -1068,7 +1005,9 @@ subprocess_fork_exec(PyObject *module, PyObject *args)
if (exec_array)
_Py_FreeCharPArray(exec_array);

_enable_gc(need_to_reenable_gc, gc_module, state);
if (need_to_reenable_gc) {
PyGC_Enable();
}
Py_XDECREF(gc_module);

return pid == -1 ? NULL : PyLong_FromPid(pid);
Expand Down Expand Up @@ -1108,67 +1047,22 @@ Raises: Only on an error in the parent process.\n\
PyDoc_STRVAR(module_doc,
"A POSIX helper for the subprocess module.");

static int
_posixsubprocess_exec(PyObject *module)
{
_posixsubprocessstate *state = get_posixsubprocess_state(module);

state->disable = PyUnicode_InternFromString("disable");
if (state->disable == NULL) {
return -1;
}

state->enable = PyUnicode_InternFromString("enable");
if (state->enable == NULL) {
return -1;
}

state->isenabled = PyUnicode_InternFromString("isenabled");
if (state->isenabled == NULL) {
return -1;
}

return 0;
}

static PyMethodDef module_methods[] = {
{"fork_exec", subprocess_fork_exec, METH_VARARGS, subprocess_fork_exec_doc},
{NULL, NULL} /* sentinel */
};

static PyModuleDef_Slot _posixsubprocess_slots[] = {
{Py_mod_exec, _posixsubprocess_exec},
{0, NULL}
};

static int _posixsubprocess_traverse(PyObject *m, visitproc visit, void *arg) {
Py_VISIT(get_posixsubprocess_state(m)->disable);
Py_VISIT(get_posixsubprocess_state(m)->enable);
Py_VISIT(get_posixsubprocess_state(m)->isenabled);
return 0;
}

static int _posixsubprocess_clear(PyObject *m) {
Py_CLEAR(get_posixsubprocess_state(m)->disable);
Py_CLEAR(get_posixsubprocess_state(m)->enable);
Py_CLEAR(get_posixsubprocess_state(m)->isenabled);
return 0;
}

static void _posixsubprocess_free(void *m) {
_posixsubprocess_clear((PyObject *)m);
}

static struct PyModuleDef _posixsubprocessmodule = {
PyModuleDef_HEAD_INIT,
.m_name = "_posixsubprocess",
.m_doc = module_doc,
.m_size = sizeof(_posixsubprocessstate),
.m_size = 0,
.m_methods = module_methods,
.m_slots = _posixsubprocess_slots,
.m_traverse = _posixsubprocess_traverse,
.m_clear = _posixsubprocess_clear,
.m_free = _posixsubprocess_free,
};

PyMODINIT_FUNC
Expand Down