[Python-checkins] bpo-20104: Improve error handling and fix a reference leak in os.posix_spawn(). (GH-6332)
Miss Islington (bot)
webhook-mailer at python.org
Tue May 1 10:18:48 EDT 2018
https://github.com/python/cpython/commit/77fa7835da0cb49d30ac5d4c32bf6eb71eae0742
commit: 77fa7835da0cb49d30ac5d4c32bf6eb71eae0742
branch: 3.7
author: Miss Islington (bot) <31488909+miss-islington at users.noreply.github.com>
committer: GitHub <noreply at github.com>
date: 2018-05-01T07:18:44-07:00
summary:
bpo-20104: Improve error handling and fix a reference leak in os.posix_spawn(). (GH-6332)
(cherry picked from commit ef347535f289baad22c0601e12a36b2dcd155c3a)
Co-authored-by: Serhiy Storchaka <storchaka at gmail.com>
files:
A Misc/NEWS.d/next/Library/2018-04-01-19-21-04.bpo-20104.-AKcGa.rst
M Doc/library/os.rst
M Lib/test/test_posix.py
M Modules/clinic/posixmodule.c.h
M Modules/posixmodule.c
diff --git a/Doc/library/os.rst b/Doc/library/os.rst
index 2f3b31edd5d7..5699d50a2034 100644
--- a/Doc/library/os.rst
+++ b/Doc/library/os.rst
@@ -3367,25 +3367,41 @@ written in Python, such as a mail server's external command delivery program.
.. function:: posix_spawn(path, argv, env, file_actions=None)
- Wraps the posix_spawn() C library API for use from Python.
+ Wraps the :c:func:`posix_spawn` C library API for use from Python.
- Most users should use :class:`subprocess.run` instead of posix_spawn.
+ Most users should use :func:`subprocess.run` instead of :func:`posix_spawn`.
The *path*, *args*, and *env* arguments are similar to :func:`execve`.
The *file_actions* argument may be a sequence of tuples describing actions
to take on specific file descriptors in the child process between the C
- library implementation's fork and exec steps. The first item in each tuple
- must be one of the three type indicator listed below describing the
- remaining tuple elements:
+ library implementation's :c:func:`fork` and :c:func:`exec` steps.
+ The first item in each tuple must be one of the three type indicator
+ listed below describing the remaining tuple elements:
- (os.POSIX_SPAWN_OPEN, fd, path, open flags, mode)
- (os.POSIX_SPAWN_CLOSE, fd)
- (os.POSIX_SPAWN_DUP2, fd, new_fd)
+ .. data:: POSIX_SPAWN_OPEN
- These tuples correspond to the C library posix_spawn_file_actions_addopen,
- posix_spawn_file_actions_addclose, and posix_spawn_file_actions_adddup2 API
- calls used to prepare for the posix_spawn call itself.
+ (``os.POSIX_SPAWN_OPEN``, *fd*, *path*, *flags*, *mode*)
+
+ Performs ``os.dup2(os.open(path, flags, mode), fd)``.
+
+ .. data:: POSIX_SPAWN_CLOSE
+
+ (``os.POSIX_SPAWN_CLOSE``, *fd*)
+
+ Performs ``os.close(fd)``.
+
+ .. data:: POSIX_SPAWN_DUP2
+
+ (``os.POSIX_SPAWN_DUP2``, *fd*, *new_fd*)
+
+ Performs ``os.dup2(fd, new_fd)``.
+
+ These tuples correspond to the C library
+ :c:func:`posix_spawn_file_actions_addopen`,
+ :c:func:`posix_spawn_file_actions_addclose`, and
+ :c:func:`posix_spawn_file_actions_adddup2` API calls used to prepare
+ for the :c:func:`posix_spawn` call itself.
.. versionadded:: 3.7
diff --git a/Lib/test/test_posix.py b/Lib/test/test_posix.py
index 8ada0e3ab3c7..b94da3f45a2c 100644
--- a/Lib/test/test_posix.py
+++ b/Lib/test/test_posix.py
@@ -177,22 +177,6 @@ def test_fexecve(self):
os.close(fp)
- @unittest.skipUnless(hasattr(os, 'posix_spawn'), "test needs os.posix_spawn")
- def test_posix_spawn(self):
- pid = posix.posix_spawn(sys.executable, [sys.executable, "-c", "pass"], os.environ,[])
- self.assertEqual(os.waitpid(pid,0),(pid,0))
-
-
- @unittest.skipUnless(hasattr(os, 'posix_spawn'), "test needs os.posix_spawn")
- def test_posix_spawn_file_actions(self):
- file_actions = []
- file_actions.append((0,3,os.path.realpath(__file__),0,0))
- file_actions.append((os.POSIX_SPAWN_CLOSE,2))
- file_actions.append((os.POSIX_SPAWN_DUP2,1,4))
- pid = posix.posix_spawn(sys.executable, [sys.executable, "-c", "pass"], os.environ, file_actions)
- self.assertEqual(os.waitpid(pid,0),(pid,0))
-
-
@unittest.skipUnless(hasattr(posix, 'waitid'), "test needs posix.waitid()")
@unittest.skipUnless(hasattr(os, 'fork'), "test needs os.fork()")
def test_waitid(self):
@@ -1437,9 +1421,168 @@ def test_setgroups(self):
posix.setgroups(groups)
self.assertListEqual(groups, posix.getgroups())
+
+ at unittest.skipUnless(hasattr(os, 'posix_spawn'), "test needs os.posix_spawn")
+class TestPosixSpawn(unittest.TestCase):
+ def test_returns_pid(self):
+ pidfile = support.TESTFN
+ self.addCleanup(support.unlink, pidfile)
+ script = f"""if 1:
+ import os
+ with open({pidfile!r}, "w") as pidfile:
+ pidfile.write(str(os.getpid()))
+ """
+ pid = posix.posix_spawn(sys.executable,
+ [sys.executable, '-c', script],
+ os.environ)
+ self.assertEqual(os.waitpid(pid, 0), (pid, 0))
+ with open(pidfile) as f:
+ self.assertEqual(f.read(), str(pid))
+
+ def test_no_such_executable(self):
+ no_such_executable = 'no_such_executable'
+ try:
+ pid = posix.posix_spawn(no_such_executable,
+ [no_such_executable],
+ os.environ)
+ except FileNotFoundError as exc:
+ self.assertEqual(exc.filename, no_such_executable)
+ else:
+ pid2, status = os.waitpid(pid, 0)
+ self.assertEqual(pid2, pid)
+ self.assertNotEqual(status, 0)
+
+ def test_specify_environment(self):
+ envfile = support.TESTFN
+ self.addCleanup(support.unlink, envfile)
+ script = f"""if 1:
+ import os
+ with open({envfile!r}, "w") as envfile:
+ envfile.write(os.environ['foo'])
+ """
+ pid = posix.posix_spawn(sys.executable,
+ [sys.executable, '-c', script],
+ {'foo': 'bar'})
+ self.assertEqual(os.waitpid(pid, 0), (pid, 0))
+ with open(envfile) as f:
+ self.assertEqual(f.read(), 'bar')
+
+ def test_empty_file_actions(self):
+ pid = posix.posix_spawn(
+ sys.executable,
+ [sys.executable, '-c', 'pass'],
+ os.environ,
+ []
+ )
+ self.assertEqual(os.waitpid(pid, 0), (pid, 0))
+
+ def test_multiple_file_actions(self):
+ file_actions = [
+ (os.POSIX_SPAWN_OPEN, 3, os.path.realpath(__file__), os.O_RDONLY, 0),
+ (os.POSIX_SPAWN_CLOSE, 0),
+ (os.POSIX_SPAWN_DUP2, 1, 4),
+ ]
+ pid = posix.posix_spawn(sys.executable,
+ [sys.executable, "-c", "pass"],
+ os.environ, file_actions)
+ self.assertEqual(os.waitpid(pid, 0), (pid, 0))
+
+ def test_bad_file_actions(self):
+ with self.assertRaises(TypeError):
+ posix.posix_spawn(sys.executable,
+ [sys.executable, "-c", "pass"],
+ os.environ, [None])
+ with self.assertRaises(TypeError):
+ posix.posix_spawn(sys.executable,
+ [sys.executable, "-c", "pass"],
+ os.environ, [()])
+ with self.assertRaises(TypeError):
+ posix.posix_spawn(sys.executable,
+ [sys.executable, "-c", "pass"],
+ os.environ, [(None,)])
+ with self.assertRaises(TypeError):
+ posix.posix_spawn(sys.executable,
+ [sys.executable, "-c", "pass"],
+ os.environ, [(12345,)])
+ with self.assertRaises(TypeError):
+ posix.posix_spawn(sys.executable,
+ [sys.executable, "-c", "pass"],
+ os.environ, [(os.POSIX_SPAWN_CLOSE,)])
+ with self.assertRaises(TypeError):
+ posix.posix_spawn(sys.executable,
+ [sys.executable, "-c", "pass"],
+ os.environ, [(os.POSIX_SPAWN_CLOSE, 1, 2)])
+ with self.assertRaises(TypeError):
+ posix.posix_spawn(sys.executable,
+ [sys.executable, "-c", "pass"],
+ os.environ, [(os.POSIX_SPAWN_CLOSE, None)])
+ with self.assertRaises(ValueError):
+ posix.posix_spawn(sys.executable,
+ [sys.executable, "-c", "pass"],
+ os.environ,
+ [(os.POSIX_SPAWN_OPEN, 3, __file__ + '\0',
+ os.O_RDONLY, 0)])
+
+ def test_open_file(self):
+ outfile = support.TESTFN
+ self.addCleanup(support.unlink, outfile)
+ script = """if 1:
+ import sys
+ sys.stdout.write("hello")
+ """
+ file_actions = [
+ (os.POSIX_SPAWN_OPEN, 1, outfile,
+ os.O_WRONLY | os.O_CREAT | os.O_TRUNC,
+ stat.S_IRUSR | stat.S_IWUSR),
+ ]
+ pid = posix.posix_spawn(sys.executable,
+ [sys.executable, '-c', script],
+ os.environ, file_actions)
+ self.assertEqual(os.waitpid(pid, 0), (pid, 0))
+ with open(outfile) as f:
+ self.assertEqual(f.read(), 'hello')
+
+ def test_close_file(self):
+ closefile = support.TESTFN
+ self.addCleanup(support.unlink, closefile)
+ script = f"""if 1:
+ import os
+ try:
+ os.fstat(0)
+ except OSError as e:
+ with open({closefile!r}, 'w') as closefile:
+ closefile.write('is closed %d' % e.errno)
+ """
+ pid = posix.posix_spawn(sys.executable,
+ [sys.executable, '-c', script],
+ os.environ,
+ [(os.POSIX_SPAWN_CLOSE, 0),])
+ self.assertEqual(os.waitpid(pid, 0), (pid, 0))
+ with open(closefile) as f:
+ self.assertEqual(f.read(), 'is closed %d' % errno.EBADF)
+
+ def test_dup2(self):
+ dupfile = support.TESTFN
+ self.addCleanup(support.unlink, dupfile)
+ script = """if 1:
+ import sys
+ sys.stdout.write("hello")
+ """
+ with open(dupfile, "wb") as childfile:
+ file_actions = [
+ (os.POSIX_SPAWN_DUP2, childfile.fileno(), 1),
+ ]
+ pid = posix.posix_spawn(sys.executable,
+ [sys.executable, '-c', script],
+ os.environ, file_actions)
+ self.assertEqual(os.waitpid(pid, 0), (pid, 0))
+ with open(dupfile) as f:
+ self.assertEqual(f.read(), 'hello')
+
+
def test_main():
try:
- support.run_unittest(PosixTester, PosixGroupsTester)
+ support.run_unittest(PosixTester, PosixGroupsTester, TestPosixSpawn)
finally:
support.reap_children()
diff --git a/Misc/NEWS.d/next/Library/2018-04-01-19-21-04.bpo-20104.-AKcGa.rst b/Misc/NEWS.d/next/Library/2018-04-01-19-21-04.bpo-20104.-AKcGa.rst
new file mode 100644
index 000000000000..150401dc7412
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2018-04-01-19-21-04.bpo-20104.-AKcGa.rst
@@ -0,0 +1 @@
+Improved error handling and fixed a reference leak in :func:`os.posix_spawn()`.
diff --git a/Modules/clinic/posixmodule.c.h b/Modules/clinic/posixmodule.c.h
index 4054389d15f3..e4bbd082450b 100644
--- a/Modules/clinic/posixmodule.c.h
+++ b/Modules/clinic/posixmodule.c.h
@@ -1742,7 +1742,7 @@ PyDoc_STRVAR(os_posix_spawn__doc__,
" env\n"
" Dictionary of strings mapping to strings.\n"
" file_actions\n"
-" FileActions object.");
+" A sequence of file action tuples.");
#define OS_POSIX_SPAWN_METHODDEF \
{"posix_spawn", (PyCFunction)os_posix_spawn, METH_FASTCALL, os_posix_spawn__doc__},
@@ -6589,4 +6589,4 @@ os_getrandom(PyObject *module, PyObject *const *args, Py_ssize_t nargs, PyObject
#ifndef OS_GETRANDOM_METHODDEF
#define OS_GETRANDOM_METHODDEF
#endif /* !defined(OS_GETRANDOM_METHODDEF) */
-/*[clinic end generated code: output=fc603214822bdda6 input=a9049054013a1b77]*/
+/*[clinic end generated code: output=8d3d9dddf254c3c2 input=a9049054013a1b77]*/
diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c
index e8964ce33901..a81580db0a3b 100644
--- a/Modules/posixmodule.c
+++ b/Modules/posixmodule.c
@@ -5114,6 +5114,111 @@ enum posix_spawn_file_actions_identifier {
POSIX_SPAWN_DUP2
};
+static int
+parse_file_actions(PyObject *file_actions,
+ posix_spawn_file_actions_t *file_actionsp)
+{
+ PyObject *seq;
+ PyObject *file_action = NULL;
+ PyObject *tag_obj;
+
+ seq = PySequence_Fast(file_actions,
+ "file_actions must be a sequence or None");
+ if (seq == NULL) {
+ return -1;
+ }
+
+ errno = posix_spawn_file_actions_init(file_actionsp);
+ if (errno) {
+ posix_error();
+ Py_DECREF(seq);
+ return -1;
+ }
+
+ for (int i = 0; i < PySequence_Fast_GET_SIZE(seq); ++i) {
+ file_action = PySequence_Fast_GET_ITEM(seq, i);
+ Py_INCREF(file_action);
+ if (!PyTuple_Check(file_action) || !PyTuple_GET_SIZE(file_action)) {
+ PyErr_SetString(PyExc_TypeError,
+ "Each file_actions element must be a non-empty tuple");
+ goto fail;
+ }
+ long tag = PyLong_AsLong(PyTuple_GET_ITEM(file_action, 0));
+ if (tag == -1 && PyErr_Occurred()) {
+ goto fail;
+ }
+
+ /* Populate the file_actions object */
+ switch (tag) {
+ case POSIX_SPAWN_OPEN: {
+ int fd, oflag;
+ PyObject *path;
+ unsigned long mode;
+ if (!PyArg_ParseTuple(file_action, "OiO&ik"
+ ";A open file_action tuple must have 5 elements",
+ &tag_obj, &fd, PyUnicode_FSConverter, &path,
+ &oflag, &mode))
+ {
+ goto fail;
+ }
+ errno = posix_spawn_file_actions_addopen(file_actionsp,
+ fd, PyBytes_AS_STRING(path), oflag, (mode_t)mode);
+ Py_DECREF(path); /* addopen copied it. */
+ if (errno) {
+ posix_error();
+ goto fail;
+ }
+ break;
+ }
+ case POSIX_SPAWN_CLOSE: {
+ int fd;
+ if (!PyArg_ParseTuple(file_action, "Oi"
+ ";A close file_action tuple must have 2 elements",
+ &tag_obj, &fd))
+ {
+ goto fail;
+ }
+ errno = posix_spawn_file_actions_addclose(file_actionsp, fd);
+ if (errno) {
+ posix_error();
+ goto fail;
+ }
+ break;
+ }
+ case POSIX_SPAWN_DUP2: {
+ int fd1, fd2;
+ if (!PyArg_ParseTuple(file_action, "Oii"
+ ";A dup2 file_action tuple must have 3 elements",
+ &tag_obj, &fd1, &fd2))
+ {
+ goto fail;
+ }
+ errno = posix_spawn_file_actions_adddup2(file_actionsp,
+ fd1, fd2);
+ if (errno) {
+ posix_error();
+ goto fail;
+ }
+ break;
+ }
+ default: {
+ PyErr_SetString(PyExc_TypeError,
+ "Unknown file_actions identifier");
+ goto fail;
+ }
+ }
+ Py_DECREF(file_action);
+ }
+ Py_DECREF(seq);
+ return 0;
+
+fail:
+ Py_DECREF(seq);
+ Py_DECREF(file_action);
+ (void)posix_spawn_file_actions_destroy(file_actionsp);
+ return -1;
+}
+
/*[clinic input]
os.posix_spawn
@@ -5124,7 +5229,7 @@ os.posix_spawn
env: object
Dictionary of strings mapping to strings.
file_actions: object = None
- FileActions object.
+ A sequence of file action tuples.
/
Execute the program specified by path in a new process.
@@ -5133,22 +5238,22 @@ Execute the program specified by path in a new process.
static PyObject *
os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv,
PyObject *env, PyObject *file_actions)
-/*[clinic end generated code: output=d023521f541c709c input=0ec9f1cfdc890be5]*/
+/*[clinic end generated code: output=d023521f541c709c input=a3db1021d33230dc]*/
{
EXECV_CHAR **argvlist = NULL;
EXECV_CHAR **envlist = NULL;
- posix_spawn_file_actions_t _file_actions;
+ posix_spawn_file_actions_t file_actions_buf;
posix_spawn_file_actions_t *file_actionsp = NULL;
Py_ssize_t argc, envc;
- PyObject* result = NULL;
- PyObject* seq = NULL;
-
+ PyObject *result = NULL;
+ pid_t pid;
+ int err_code;
/* posix_spawn has three arguments: (path, argv, env), where
- argv is a list or tuple of strings and env is a dictionary
+ argv is a list or tuple of strings and env is a dictionary
like posix.environ. */
- if (!PySequence_Check(argv)) {
+ if (!PyList_Check(argv) && !PyTuple_Check(argv)) {
PyErr_SetString(PyExc_TypeError,
"posix_spawn: argv must be a tuple or list");
goto exit;
@@ -5180,139 +5285,35 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv,
goto exit;
}
- pid_t pid;
- if (file_actions != NULL && file_actions != Py_None) {
- if(posix_spawn_file_actions_init(&_file_actions) != 0){
- PyErr_SetString(PyExc_OSError,
- "Error initializing file actions");
- goto exit;
- }
-
- file_actionsp = &_file_actions;
-
- seq = PySequence_Fast(file_actions, "file_actions must be a sequence");
- if(seq == NULL) {
+ if (file_actions != Py_None) {
+ if (parse_file_actions(file_actions, &file_actions_buf)) {
goto exit;
}
- PyObject* file_actions_obj;
- PyObject* mode_obj;
-
- for (int i = 0; i < PySequence_Fast_GET_SIZE(seq); ++i) {
- file_actions_obj = PySequence_Fast_GET_ITEM(seq, i);
-
- if(!PySequence_Check(file_actions_obj) | !PySequence_Size(file_actions_obj)) {
- PyErr_SetString(PyExc_TypeError,"Each file_action element must be a non empty sequence");
- goto exit;
- }
-
-
- mode_obj = PySequence_Fast_GET_ITEM(file_actions_obj, 0);
- int mode = PyLong_AsLong(mode_obj);
-
- /* Populate the file_actions object */
-
- switch(mode) {
-
- case POSIX_SPAWN_OPEN:
- if(PySequence_Size(file_actions_obj) != 5) {
- PyErr_SetString(PyExc_TypeError,"A open file_action object must have 5 elements");
- goto exit;
- }
-
- long open_fd = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 1));
- if(PyErr_Occurred()) {
- goto exit;
- }
- const char* open_path = PyUnicode_AsUTF8(PySequence_GetItem(file_actions_obj, 2));
- if(open_path == NULL) {
- goto exit;
- }
- long open_oflag = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 3));
- if(PyErr_Occurred()) {
- goto exit;
- }
- long open_mode = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 4));
- if(PyErr_Occurred()) {
- goto exit;
- }
- if(posix_spawn_file_actions_addopen(file_actionsp, open_fd, open_path, open_oflag, open_mode)) {
- PyErr_SetString(PyExc_OSError,"Failed to add open file action");
- goto exit;
- }
-
- break;
-
- case POSIX_SPAWN_CLOSE:
- if(PySequence_Size(file_actions_obj) != 2){
- PyErr_SetString(PyExc_TypeError,"A close file_action object must have 2 elements");
- goto exit;
- }
-
- long close_fd = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 1));
- if(PyErr_Occurred()) {
- goto exit;
- }
- if(posix_spawn_file_actions_addclose(file_actionsp, close_fd)) {
- PyErr_SetString(PyExc_OSError,"Failed to add close file action");
- goto exit;
- }
- break;
-
- case POSIX_SPAWN_DUP2:
- if(PySequence_Size(file_actions_obj) != 3){
- PyErr_SetString(PyExc_TypeError,"A dup2 file_action object must have 3 elements");
- goto exit;
- }
-
- long fd1 = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 1));
- if(PyErr_Occurred()) {
- goto exit;
- }
- long fd2 = PyLong_AsLong(PySequence_GetItem(file_actions_obj, 2));
- if(PyErr_Occurred()) {
- goto exit;
- }
- if(posix_spawn_file_actions_adddup2(file_actionsp, fd1, fd2)) {
- PyErr_SetString(PyExc_OSError,"Failed to add dup2 file action");
- goto exit;
- }
- break;
-
- default:
- PyErr_SetString(PyExc_TypeError,"Unknown file_actions identifier");
- goto exit;
- }
- }
+ file_actionsp = &file_actions_buf;
}
_Py_BEGIN_SUPPRESS_IPH
- int err_code = posix_spawn(&pid, path->narrow, file_actionsp, NULL, argvlist, envlist);
+ err_code = posix_spawn(&pid, path->narrow,
+ file_actionsp, NULL, argvlist, envlist);
_Py_END_SUPPRESS_IPH
- if(err_code) {
- PyErr_SetString(PyExc_OSError,"posix_spawn call failed");
+ if (err_code) {
+ errno = err_code;
+ PyErr_SetFromErrnoWithFilenameObject(PyExc_OSError, path->object);
goto exit;
}
result = PyLong_FromPid(pid);
exit:
-
- Py_XDECREF(seq);
-
- if(file_actionsp) {
- posix_spawn_file_actions_destroy(file_actionsp);
+ if (file_actionsp) {
+ (void)posix_spawn_file_actions_destroy(file_actionsp);
}
-
if (envlist) {
free_string_array(envlist, envc);
}
-
if (argvlist) {
free_string_array(argvlist, argc);
}
-
return result;
-
-
}
#endif /* HAVE_POSIX_SPAWN */
More information about the Python-checkins
mailing list