From 98ca32d9c5e573c433f180a57c4939a78468518e Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sun, 1 Apr 2018 14:19:03 +0300 Subject: [PATCH 1/9] bpo-20104: Fix numerous issues with os.posix_spawn(). --- .../2018-04-01-19-21-04.bpo-20104.-AKcGa.rst | 1 + Modules/posixmodule.c | 237 +++++++++--------- 2 files changed, 118 insertions(+), 120 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2018-04-01-19-21-04.bpo-20104.-AKcGa.rst 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 00000000000000..85afd76c27db3e --- /dev/null +++ b/Misc/NEWS.d/next/Library/2018-04-01-19-21-04.bpo-20104.-AKcGa.rst @@ -0,0 +1 @@ +Fixed numerous issues with :func:`os.posix_spawn()`. diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index e6721032f211b7..f9693b5978b363 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5114,6 +5114,107 @@ 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; + } + int overflow; + long tag = PyLong_AsLongAndOverflow(PyTuple_GET_ITEM(file_action, 0), + &overflow); + 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", + &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); + if (errno) { + Py_DECREF(path); + posix_error(); + goto fail; + } + break; + } + case POSIX_SPAWN_CLOSE: { + int fd; + if (!PyArg_ParseTuple(file_action, "Oi", &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", + &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_ValueError, + "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 @@ -5137,18 +5238,18 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, { 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 +5281,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; + posix_error(); 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 */ From 7437d255698c50569b315c94a79244c3bb06e641 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 1 Apr 2018 10:58:46 -0700 Subject: [PATCH 2/9] just use PyLong_AsLong, overflow wasn't used. --- Modules/posixmodule.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index f9693b5978b363..bbf62dfeba85db 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5143,9 +5143,7 @@ parse_file_actions(PyObject *file_actions, "Each file_actions element must be a non-empty tuple"); goto fail; } - int overflow; - long tag = PyLong_AsLongAndOverflow(PyTuple_GET_ITEM(file_action, 0), - &overflow); + long tag = PyLong_AsLong(PyTuple_GET_ITEM(file_action, 0)); if (tag == -1 && PyErr_Occurred()) { goto fail; } From a9d86bf0a7adfcd51e76de39be8a1fedc83fdcf7 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 1 Apr 2018 11:11:03 -0700 Subject: [PATCH 3/9] Always Py_DECREF(path), not just on error. --- Modules/posixmodule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index bbf62dfeba85db..0e0ed0bccf7149 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5162,8 +5162,8 @@ parse_file_actions(PyObject *file_actions, } 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) { - Py_DECREF(path); posix_error(); goto fail; } From 6839f0bd622c4554680a24623c8d8f822de6e0a0 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sun, 1 Apr 2018 21:59:23 +0300 Subject: [PATCH 4/9] Fix the style of tests. --- Lib/test/test_posix.py | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/Lib/test/test_posix.py b/Lib/test/test_posix.py index 8ada0e3ab3c7d6..284419b1b20697 100644 --- a/Lib/test/test_posix.py +++ b/Lib/test/test_posix.py @@ -179,18 +179,23 @@ def test_fexecve(self): @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)) + 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)) + file_actions = [ + (0, 3, os.path.realpath(__file__), 0, 0), + (os.POSIX_SPAWN_CLOSE, 2), + (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()") From 6b46b4883852fd9835708df34257a6af1d5857d1 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Mon, 2 Apr 2018 09:52:44 +0300 Subject: [PATCH 5/9] Add more tests. --- Lib/test/test_posix.py | 176 ++++++++++++++++++++++++++++----- Modules/clinic/posixmodule.c.h | 4 +- Modules/posixmodule.c | 22 +++-- 3 files changed, 170 insertions(+), 32 deletions(-) diff --git a/Lib/test/test_posix.py b/Lib/test/test_posix.py index 284419b1b20697..6dc65743a0372e 100644 --- a/Lib/test/test_posix.py +++ b/Lib/test/test_posix.py @@ -177,27 +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 = [ - (0, 3, os.path.realpath(__file__), 0, 0), - (os.POSIX_SPAWN_CLOSE, 2), - (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): @@ -1442,9 +1421,162 @@ def test_setgroups(self): posix.setgroups(groups) self.assertListEqual(groups, posix.getgroups()) + +@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): + with self.assertRaises(FileNotFoundError) as cm: + posix.posix_spawn('no_such_executable', + ['no_such_executable'], + os.environ) + self.assertEqual(cm.exception.filename, 'no_such_executable') + + 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/Modules/clinic/posixmodule.c.h b/Modules/clinic/posixmodule.c.h index 4054389d15f33e..e4bbd082450baa 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 0e0ed0bccf7149..4ac6e7658999d2 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5154,7 +5154,8 @@ parse_file_actions(PyObject *file_actions, int fd, oflag; PyObject *path; unsigned long mode; - if (!PyArg_ParseTuple(file_action, "OiO&ik", + if (!PyArg_ParseTuple(file_action, "OiO&ik" + ";A open file_action tuple must have 5 elements", &tag_obj, &fd, PyUnicode_FSConverter, &path, &oflag, &mode)) { @@ -5171,7 +5172,10 @@ parse_file_actions(PyObject *file_actions, } case POSIX_SPAWN_CLOSE: { int fd; - if (!PyArg_ParseTuple(file_action, "Oi", &tag_obj, &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); @@ -5183,12 +5187,14 @@ parse_file_actions(PyObject *file_actions, } case POSIX_SPAWN_DUP2: { int fd1, fd2; - if (!PyArg_ParseTuple(file_action, "Oii", + 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); + errno = posix_spawn_file_actions_adddup2(file_actionsp, + fd1, fd2); if (errno) { posix_error(); goto fail; @@ -5196,7 +5202,7 @@ parse_file_actions(PyObject *file_actions, break; } default: { - PyErr_SetString(PyExc_ValueError, + PyErr_SetString(PyExc_TypeError, "Unknown file_actions identifier"); goto fail; } @@ -5223,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. @@ -5232,7 +5238,7 @@ 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; @@ -5292,7 +5298,7 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, _Py_END_SUPPRESS_IPH if (err_code) { errno = err_code; - posix_error(); + PyErr_SetFromErrnoWithFilenameObject(PyExc_OSError, path->object); goto exit; } result = PyLong_FromPid(pid); From 7474141abada3d691bf26a9c03d54ec369634606 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Tue, 10 Apr 2018 15:16:19 +0300 Subject: [PATCH 6/9] Update a news entry. --- .../next/Library/2018-04-01-19-21-04.bpo-20104.-AKcGa.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 85afd76c27db3e..150401dc741203 100644 --- 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 @@ -1 +1 @@ -Fixed numerous issues with :func:`os.posix_spawn()`. +Improved error handling and fixed a reference leak in :func:`os.posix_spawn()`. From b5a8c7b899cfc47b7cda7b8e6411dfb42348f55b Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Tue, 10 Apr 2018 16:00:14 +0300 Subject: [PATCH 7/9] Improve docs formatting and describe file actions. --- Doc/library/os.rst | 38 +++++++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/Doc/library/os.rst b/Doc/library/os.rst index d269d0b0eaa6ce..079911a034fcfc 100644 --- a/Doc/library/os.rst +++ b/Doc/library/os.rst @@ -3355,25 +3355,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 From 5da8953b9ca067dab8e034f573bda79e603a0cfe Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Thu, 26 Apr 2018 15:11:59 +0300 Subject: [PATCH 8/9] Try to fix tests for no_such_executable. --- Lib/test/test_posix.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_posix.py b/Lib/test/test_posix.py index 6dc65743a0372e..a581cd8aa029d9 100644 --- a/Lib/test/test_posix.py +++ b/Lib/test/test_posix.py @@ -12,6 +12,7 @@ import os import platform import pwd +import random import stat import tempfile import unittest @@ -1440,11 +1441,12 @@ def test_returns_pid(self): self.assertEqual(f.read(), str(pid)) def test_no_such_executable(self): + no_such_executable = 'no_such_executable%s' % random.getrandbits(32) with self.assertRaises(FileNotFoundError) as cm: - posix.posix_spawn('no_such_executable', - ['no_such_executable'], + posix.posix_spawn(no_such_executable, + [no_such_executable], os.environ) - self.assertEqual(cm.exception.filename, 'no_such_executable') + self.assertEqual(cm.exception.filename, no_such_executable) def test_specify_environment(self): envfile = support.TESTFN From a3e1fae30515de01ac5882b3fccc5c1f9dc3d955 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Thu, 26 Apr 2018 16:03:43 +0300 Subject: [PATCH 9/9] Yet one attempt to fix a test. --- Lib/test/test_posix.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/Lib/test/test_posix.py b/Lib/test/test_posix.py index a581cd8aa029d9..b94da3f45a2ced 100644 --- a/Lib/test/test_posix.py +++ b/Lib/test/test_posix.py @@ -12,7 +12,6 @@ import os import platform import pwd -import random import stat import tempfile import unittest @@ -1441,12 +1440,17 @@ def test_returns_pid(self): self.assertEqual(f.read(), str(pid)) def test_no_such_executable(self): - no_such_executable = 'no_such_executable%s' % random.getrandbits(32) - with self.assertRaises(FileNotFoundError) as cm: - posix.posix_spawn(no_such_executable, - [no_such_executable], - os.environ) - self.assertEqual(cm.exception.filename, no_such_executable) + 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