From 3fd50ee7fa3c5c4588a2dacd350c0b535623779d Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Wed, 13 Jun 2018 14:27:39 +0100 Subject: [PATCH 1/9] Add a workaround for a bug in old glib versions that are not copying the path pointer --- Modules/posixmodule.c | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 24d8be66665e82e..29867bcae27ea19 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5179,7 +5179,8 @@ enum posix_spawn_file_actions_identifier { static int parse_file_actions(PyObject *file_actions, - posix_spawn_file_actions_t *file_actionsp) + posix_spawn_file_actions_t *file_actionsp, + PyObject* temp_buffer) { PyObject *seq; PyObject *file_action = NULL; @@ -5224,9 +5225,14 @@ parse_file_actions(PyObject *file_actions, { goto fail; } + PyList_Append(temp_buffer, path); errno = posix_spawn_file_actions_addopen(file_actionsp, fd, PyBytes_AS_STRING(path), oflag, (mode_t)mode); - Py_DECREF(path); /* addopen copied it. */ + /* addopen copies the value except for some old versions of + * glibc (<2.26). The usage of temp_buffer is a workaround + * to keep this temporary objects alive until posix_spawn + * gets called.*/ + Py_DECREF(path); if (errno) { posix_error(); goto fail; @@ -5309,6 +5315,7 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, posix_spawn_file_actions_t *file_actionsp = NULL; Py_ssize_t argc, envc; PyObject *result = NULL; + PyObject *temp_buffer = NULL; pid_t pid; int err_code; @@ -5349,7 +5356,19 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, } if (file_actions != Py_None) { - if (parse_file_actions(file_actions, &file_actions_buf)) { + + /* There is a bug in old versions of glibc that makes some of the + * helper functions for manipulating file actions not copy the provided + * buffers. The use of `temp_buffer` here is a workaround that keeps the + * python objects that own the buffers alive until posix_spawn gets called. + * Check https://bugs.python.org/issue33630 for more info. */ + + temp_buffer = PyList_New(0); + + if (!temp_buffer) { + goto exit; + } + if (parse_file_actions(file_actions, &file_actions_buf, temp_buffer)) { goto exit; } file_actionsp = &file_actions_buf; @@ -5376,6 +5395,9 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, if (argvlist) { free_string_array(argvlist, argc); } + + Py_XDECREF(temp_buffer); + return result; } #endif /* HAVE_POSIX_SPAWN */ From 9a2ebfbf160e947a6435e2a57021d065f4bfcc9c Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Wed, 13 Jun 2018 15:40:54 +0100 Subject: [PATCH 2/9] Add preprocesor directives to activate the temporary buffer when needed --- Modules/posixmodule.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 29867bcae27ea19..ff2b731b610e9dd 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5177,10 +5177,16 @@ enum posix_spawn_file_actions_identifier { POSIX_SPAWN_DUP2 }; +#if defined(__GLIBC__) && ((__GLIBC__ == 2 && __GLIBC_MINOR__ < 20)) static int parse_file_actions(PyObject *file_actions, posix_spawn_file_actions_t *file_actionsp, PyObject* temp_buffer) +#else + static int + parse_file_actions(PyObject *file_actions, + posix_spawn_file_actions_t *file_actionsp) +#endif { PyObject *seq; PyObject *file_action = NULL; @@ -5225,11 +5231,13 @@ parse_file_actions(PyObject *file_actions, { goto fail; } +#if defined(__GLIBC__) && ((__GLIBC__ == 2 && __GLIBC_MINOR__ < 20)) PyList_Append(temp_buffer, path); +#endif errno = posix_spawn_file_actions_addopen(file_actionsp, fd, PyBytes_AS_STRING(path), oflag, (mode_t)mode); /* addopen copies the value except for some old versions of - * glibc (<2.26). The usage of temp_buffer is a workaround + * glibc (<2.20). The usage of temp_buffer is a workaround * to keep this temporary objects alive until posix_spawn * gets called.*/ Py_DECREF(path); @@ -5361,8 +5369,10 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, * helper functions for manipulating file actions not copy the provided * buffers. The use of `temp_buffer` here is a workaround that keeps the * python objects that own the buffers alive until posix_spawn gets called. - * Check https://bugs.python.org/issue33630 for more info. */ + * Check https://bugs.python.org/issue33630 and + * https://sourceware.org/bugzilla/show_bug.cgi?id=17048 for more info. */ +#if defined(__GLIBC__) && ((__GLIBC__ == 2 && __GLIBC_MINOR__ < 20)) temp_buffer = PyList_New(0); if (!temp_buffer) { @@ -5371,6 +5381,11 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, if (parse_file_actions(file_actions, &file_actions_buf, temp_buffer)) { goto exit; } +#else + if (parse_file_actions(file_actions, &file_actions_buf)) { + goto exit; + } +#endif file_actionsp = &file_actions_buf; } From 07e124533dead912897a094855f3e7a78dfa04b0 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Wed, 13 Jun 2018 19:22:14 +0100 Subject: [PATCH 3/9] Correct style and check error when appending to the temp list --- Modules/posixmodule.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index ff2b731b610e9dd..16c0f7b3d81d7bd 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5181,7 +5181,7 @@ enum posix_spawn_file_actions_identifier { static int parse_file_actions(PyObject *file_actions, posix_spawn_file_actions_t *file_actionsp, - PyObject* temp_buffer) + PyObject *temp_buffer) #else static int parse_file_actions(PyObject *file_actions, @@ -5232,7 +5232,10 @@ parse_file_actions(PyObject *file_actions, goto fail; } #if defined(__GLIBC__) && ((__GLIBC__ == 2 && __GLIBC_MINOR__ < 20)) - PyList_Append(temp_buffer, path); + int error = PyList_Append(temp_buffer, path); + if (error) { + goto fail; + } #endif errno = posix_spawn_file_actions_addopen(file_actionsp, fd, PyBytes_AS_STRING(path), oflag, (mode_t)mode); From 6db5f32e0236deba37d7799801dd605c40ecd025 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Fri, 15 Jun 2018 12:07:00 +0100 Subject: [PATCH 4/9] Use the temporary list always to reduce maintainance burden --- Modules/posixmodule.c | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 16c0f7b3d81d7bd..3eede70c8bb5893 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5177,16 +5177,10 @@ enum posix_spawn_file_actions_identifier { POSIX_SPAWN_DUP2 }; -#if defined(__GLIBC__) && ((__GLIBC__ == 2 && __GLIBC_MINOR__ < 20)) static int parse_file_actions(PyObject *file_actions, posix_spawn_file_actions_t *file_actionsp, PyObject *temp_buffer) -#else - static int - parse_file_actions(PyObject *file_actions, - posix_spawn_file_actions_t *file_actionsp) -#endif { PyObject *seq; PyObject *file_action = NULL; @@ -5231,12 +5225,10 @@ parse_file_actions(PyObject *file_actions, { goto fail; } -#if defined(__GLIBC__) && ((__GLIBC__ == 2 && __GLIBC_MINOR__ < 20)) int error = PyList_Append(temp_buffer, path); if (error) { goto fail; } -#endif errno = posix_spawn_file_actions_addopen(file_actionsp, fd, PyBytes_AS_STRING(path), oflag, (mode_t)mode); /* addopen copies the value except for some old versions of @@ -5375,7 +5367,6 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, * Check https://bugs.python.org/issue33630 and * https://sourceware.org/bugzilla/show_bug.cgi?id=17048 for more info. */ -#if defined(__GLIBC__) && ((__GLIBC__ == 2 && __GLIBC_MINOR__ < 20)) temp_buffer = PyList_New(0); if (!temp_buffer) { @@ -5384,11 +5375,6 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, if (parse_file_actions(file_actions, &file_actions_buf, temp_buffer)) { goto exit; } -#else - if (parse_file_actions(file_actions, &file_actions_buf)) { - goto exit; - } -#endif file_actionsp = &file_actions_buf; } From 80918dbff6bb0d3f3aac9aad1366fbe87cea7376 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Fri, 15 Jun 2018 13:06:44 +0100 Subject: [PATCH 5/9] Fix style issues and merge big comment blocks --- Modules/posixmodule.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 3eede70c8bb5893..9abd754e3257aaa 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5225,16 +5225,11 @@ parse_file_actions(PyObject *file_actions, { goto fail; } - int error = PyList_Append(temp_buffer, path); - if (error) { + if (PyList_Append(temp_buffer, path)) { goto fail; } errno = posix_spawn_file_actions_addopen(file_actionsp, fd, PyBytes_AS_STRING(path), oflag, (mode_t)mode); - /* addopen copies the value except for some old versions of - * glibc (<2.20). The usage of temp_buffer is a workaround - * to keep this temporary objects alive until posix_spawn - * gets called.*/ Py_DECREF(path); if (errno) { posix_error(); @@ -5359,16 +5354,17 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, } if (file_actions != Py_None) { - /* There is a bug in old versions of glibc that makes some of the * helper functions for manipulating file actions not copy the provided * buffers. The use of `temp_buffer` here is a workaround that keeps the * python objects that own the buffers alive until posix_spawn gets called. + * posix_spawn_file_actions_addopen copies the value of **path** except for + * some old * versions of * glibc (<2.20). The usage of temp_buffer is + * a workaround * to keep this temporary objects alive until posix_spawn + * gets called. * Check https://bugs.python.org/issue33630 and - * https://sourceware.org/bugzilla/show_bug.cgi?id=17048 for more info. */ - + * https://sourceware.org/bugzilla/show_bug.cgi?id=17048 for more info.*/ temp_buffer = PyList_New(0); - if (!temp_buffer) { goto exit; } @@ -5399,9 +5395,7 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, if (argvlist) { free_string_array(argvlist, argc); } - Py_XDECREF(temp_buffer); - return result; } #endif /* HAVE_POSIX_SPAWN */ From c832471c133934905f0090e5123245122ec093bb Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Fri, 15 Jun 2018 13:19:19 +0100 Subject: [PATCH 6/9] Remove erroneus characters in the block comment --- Modules/posixmodule.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 9abd754e3257aaa..4148fa710da66d4 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5359,9 +5359,7 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, * buffers. The use of `temp_buffer` here is a workaround that keeps the * python objects that own the buffers alive until posix_spawn gets called. * posix_spawn_file_actions_addopen copies the value of **path** except for - * some old * versions of * glibc (<2.20). The usage of temp_buffer is - * a workaround * to keep this temporary objects alive until posix_spawn - * gets called. + * some old versions of glibc (<2.20). * Check https://bugs.python.org/issue33630 and * https://sourceware.org/bugzilla/show_bug.cgi?id=17048 for more info.*/ temp_buffer = PyList_New(0); From 0e3348ea707590c25e81917470cc9b15650ed705 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Fri, 15 Jun 2018 13:24:39 +0100 Subject: [PATCH 7/9] Rewrite block comment to make it more clear an less repetitive --- Modules/posixmodule.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 4148fa710da66d4..46e8e76bb3ee50a 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5356,10 +5356,10 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, if (file_actions != Py_None) { /* There is a bug in old versions of glibc that makes some of the * helper functions for manipulating file actions not copy the provided - * buffers. The use of `temp_buffer` here is a workaround that keeps the + * buffers. The problem is that posix_spawn_file_actions_addopen copies + * the value of **path** except for some old versions of glibc (<2.20). + * The use of `temp_buffer` here is a workaround that keeps the * python objects that own the buffers alive until posix_spawn gets called. - * posix_spawn_file_actions_addopen copies the value of **path** except for - * some old versions of glibc (<2.20). * Check https://bugs.python.org/issue33630 and * https://sourceware.org/bugzilla/show_bug.cgi?id=17048 for more info.*/ temp_buffer = PyList_New(0); From c74b9884d12ca3f09a3e5b1f6d627f30fcb555b4 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Fri, 15 Jun 2018 13:52:22 +0100 Subject: [PATCH 8/9] Clarify the problem in the block comment and eliminate markdown syntax --- Modules/posixmodule.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 46e8e76bb3ee50a..25aaede13352953 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5356,9 +5356,9 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv, if (file_actions != Py_None) { /* There is a bug in old versions of glibc that makes some of the * helper functions for manipulating file actions not copy the provided - * buffers. The problem is that posix_spawn_file_actions_addopen copies - * the value of **path** except for some old versions of glibc (<2.20). - * The use of `temp_buffer` here is a workaround that keeps the + * buffers. The problem is that posix_spawn_file_actions_addopen does not + * copy the value of path for some old versions of glibc (<2.20). + * The use of temp_buffer here is a workaround that keeps the * python objects that own the buffers alive until posix_spawn gets called. * Check https://bugs.python.org/issue33630 and * https://sourceware.org/bugzilla/show_bug.cgi?id=17048 for more info.*/ From 0ee9f25433073e9513faa8980882fd65b2fb2bdc Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Tue, 19 Jun 2018 07:36:19 +0100 Subject: [PATCH 9/9] Fix reference to path when append fails --- Modules/posixmodule.c | 1 + 1 file changed, 1 insertion(+) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 25aaede13352953..51fc1c59ca84784 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5226,6 +5226,7 @@ parse_file_actions(PyObject *file_actions, goto fail; } if (PyList_Append(temp_buffer, path)) { + Py_DECREF(path); goto fail; } errno = posix_spawn_file_actions_addopen(file_actionsp,