bpo-33630: Fix memory leak in old versions of glicb for `posix_spawn`. #7685

pablogsal · 2018-06-13T18:18:34Z

https://bugs.python.org/issue33630

…the path pointer

pablogsal · 2018-06-13T18:22:58Z

This is complementing #7663. @serhiy-storchaka I have addressed your feedback in 07e1245

pablogsal · 2018-06-15T11:13:09Z

I can confirm again that this fixes the buildbot problems. Tested on the buildbot itself:

[pablogsal@gcc1-power7 cpython]$ git rev-parse HEAD
6db5f32e0236deba37d7799801dd605c40ecd025


[pablogsal@gcc1-power7 cpython]$ uname -a
Linux gcc1-power7.osuosl.org 3.10.0-514.26.2.el7.ppc64 #1 SMP Mon Jul 10 02:26:53 GMT 2017 ppc64 ppc64 ppc64 GNU/Linux

[pablogsal@gcc1-power7 cpython]$ ./configure --with-pydebug && make -j
....

[pablogsal@gcc1-power7 cpython]$ ./python -m unittest test.test_posix
ss....s....................ss................ss............s...s.....................................
----------------------------------------------------------------------
Ran 101 tests in 1.916s

OK (skipped=9)
[pablogsal@gcc1-power7 cpython]$ ./python -m test test_posix -R 3:3
Run tests sequentially
0:00:00 load avg: 7.20 [1/1] test_posix
beginning 6 repetitions
123456
......

== Tests result: SUCCESS ==

1 test OK.

Total duration: 6 sec
Tests result: SUCCESS

pablogsal · 2018-06-15T11:15:12Z

It seems that the problem covered by #7663 (the race in the test) disappears once this is fixed. As is a race condition I am not sure, but running 3 test suites in parallel that executes test_posix 200 times does not raise any problem on the buildbot.

serhiy-storchaka

Thank you @pablogsal for your responsibility!

LGTM in general, added just few stylistic comments.

serhiy-storchaka · 2018-06-15T11:26:11Z

Modules/posixmodule.c

@@ -5224,9 +5225,17 @@ parse_file_actions(PyObject *file_actions,
                {
                    goto fail;
                }
+                int error = PyList_Append(temp_buffer, path);
+                if (error) {


Seems the error variable is used only here. In this case it would be better to inline it.

Done in commit 80918db

serhiy-storchaka · 2018-06-15T11:29:25Z

Modules/posixmodule.c

@@ -5349,7 +5359,20 @@ 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


There are two large comments about this issue which are not duplicates of one other. It would be better to merge them.

Done in commit 80918db

serhiy-storchaka · 2018-06-15T11:30:41Z

Modules/posixmodule.c

+         * https://sourceware.org/bugzilla/show_bug.cgi?id=17048 for more info. */
+
+        temp_buffer = PyList_New(0);
+


IMHO it would look clearer if remove some of empty lines (or even all new empty lines).

Done in commit 80918db.

serhiy-storchaka · 2018-06-15T12:10:05Z

Modules/posixmodule.c

        /* 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


What all these stars mean?

Sorry, that was an error after wrapping the lines. Fixed in 54de1e2

I am rewriting a bit this paragraph.

pablogsal · 2018-06-15T12:25:18Z

@serhiy-storchaka I have rewritten a bit the comment in 0e3348e to make it more clear.

serhiy-storchaka · 2018-06-15T12:30:55Z

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


Actually the problem is not that posix_spawn_file_actions_addopen copies the value of path, but that it isn't copies in on glibc <2.20.

Comments are plain text, no need to use the Markdown mark up (**path**, `temp_buffer`).

Actually the problem is not that posix_spawn_file_actions_addopen copies the value >of path, but that it isn't copies in on glibc <2.20.

Sorry, bad redaction on my part. I was trying to express that addopen copies it except for older versions of glibc and the fact that these old versions don't copy it is the problem.

Fixed in c74b988

zhangyangyu · 2018-06-19T04:00:24Z

Modules/posixmodule.c

@@ -5224,9 +5225,12 @@ parse_file_actions(PyObject *file_actions,
                {
                    goto fail;
                }
+                if (PyList_Append(temp_buffer, path)) {
+                    goto fail;


Hmm, we keep a reference to path and need to decrease here?

Fixed in 0ee9f25

pablogsal added 2 commits June 13, 2018 19:17

Add a workaround for a bug in old glib versions that are not copying …

3fd50ee

…the path pointer

Add preprocesor directives to activate the temporary buffer when needed

9a2ebfb

the-knights-who-say-ni added the CLA signed label Jun 13, 2018

bedevere-bot added the awaiting review label Jun 13, 2018

pablogsal changed the title ~~bpo-33630: Fix memory leak in older versions of glicb for posix_spawn.~~ bpo-33630: Fix memory leak in old versions of glicb for posix_spawn. Jun 13, 2018

pablogsal mentioned this pull request Jun 13, 2018

bpo-33630: Fix race condition in TestPosixSpawn.test_open #7663

Closed

Correct style and check error when appending to the temp list

07e1245

Use the temporary list always to reduce maintainance burden

6db5f32

serhiy-storchaka reviewed Jun 15, 2018

View reviewed changes

serhiy-storchaka added type-bug An unexpected behavior, bug, or error skip news labels Jun 15, 2018

Fix style issues and merge big comment blocks

80918db

serhiy-storchaka reviewed Jun 15, 2018

View reviewed changes

Remove erroneus characters in the block comment

c832471

pablogsal force-pushed the bpo33630-2 branch from 54de1e2 to c832471 Compare June 15, 2018 12:21

serhiy-storchaka approved these changes Jun 15, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Jun 15, 2018

Rewrite block comment to make it more clear an less repetitive

0e3348e

serhiy-storchaka reviewed Jun 15, 2018

View reviewed changes

Clarify the problem in the block comment and eliminate markdown syntax

c74b988

serhiy-storchaka approved these changes Jun 15, 2018

View reviewed changes

zhangyangyu reviewed Jun 19, 2018

View reviewed changes

Fix reference to path when append fails

0ee9f25

zhangyangyu approved these changes Jun 19, 2018

View reviewed changes

serhiy-storchaka approved these changes Jun 19, 2018

View reviewed changes

serhiy-storchaka merged commit cb97073 into python:master Jun 19, 2018

bedevere-bot removed the awaiting merge label Jun 19, 2018

pablogsal deleted the bpo33630-2 branch May 19, 2021 18:57

		* https://sourceware.org/bugzilla/show_bug.cgi?id=17048 for more info. */

		temp_buffer = PyList_New(0);

Uh oh!

bpo-33630: Fix memory leak in old versions of glicb for posix_spawn. #7685

bpo-33630: Fix memory leak in old versions of glicb for posix_spawn. #7685

Uh oh!

Conversation

pablogsal commented Jun 13, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pablogsal commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pablogsal commented Jun 15, 2018

Uh oh!

pablogsal commented Jun 15, 2018

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal commented Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bpo-33630: Fix memory leak in old versions of glicb for `posix_spawn`. #7685

bpo-33630: Fix memory leak in old versions of glicb for `posix_spawn`. #7685

pablogsal commented Jun 13, 2018 •

edited by bedevere-bot

Loading

pablogsal commented Jun 13, 2018 •

edited

Loading

pablogsal commented Jun 15, 2018 •

edited

Loading

pablogsal Jun 15, 2018 •

edited

Loading