bpo-33630: Fix memory leak in old versions of glicb for posix_spawn. by pablogsal · Pull Request #7685 · python/cpython (original) (raw)
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.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation18 Commits9 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
bpo-33630: Fix memory leak in older versions of glicb for bpo-33630: Fix memory leak in old versions of glicb for posix_spawn.posix_spawn.
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @pablogsal for your responsibility!
LGTM in general, added just few stylistic comments.
| @@ -5224,9 +5225,17 @@ parse_file_actions(PyObject *file_actions, |
|---|
| { |
| goto fail; |
| } |
| int error = PyList_Append(temp_buffer, path); |
| if (error) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems the error variable is used only here. In this case it would be better to inline it.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit 80918db
| @@ -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 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two large comments about this issue which are not duplicates of one other. It would be better to merge them.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit 80918db
| * https://sourceware.org/bugzilla/show\_bug.cgi?id=17048 for more info. */ |
|---|
| temp_buffer = PyList_New(0); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO it would look clearer if remove some of empty lines (or even all new empty lines).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit 80918db.
| /* 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 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What all these stars mean?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, that was an error after wrapping the lines. Fixed in 54de1e2
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am rewriting a bit this paragraph.
| @@ -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 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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`).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
| @@ -5224,9 +5225,12 @@ parse_file_actions(PyObject *file_actions, |
|---|
| { |
| goto fail; |
| } |
| if (PyList_Append(temp_buffer, path)) { |
| goto fail; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we keep a reference to path and need to decrease here?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 0ee9f25