Repository navigation
test_posix: TestPosixSpawn fails on PPC64 Fedora 3.x #77811
Description
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2018-06-19.09:10:16.986>
created_at = <Date 2018-05-24.02:18:53.880>
labels = ['3.8', 'type-bug', 'tests']
title = 'test_posix: TestPosixSpawn fails on PPC64 Fedora 3.x'
updated_at = <Date 2018-06-19.09:10:16.985>
user = 'https://github.com/vstinner'bugs.python.org fields:
activity = <Date 2018-06-19.09:10:16.985>
actor = 'vstinner'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2018-06-19.09:10:16.986>
closer = 'vstinner'
components = ['Tests']
creation = <Date 2018-05-24.02:18:53.880>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 33630
keywords = ['patch']
message_count = 31.0
messages = ['317509', '317524', '317527', '317797', '317827', '319385', '319386', '319387', '319388', '319389', '319390', '319394', '319427', '319429', '319431', '319432', '319434', '319454', '319457', '319458', '319463', '319481', '319485', '319594', '319598', '319605', '319606', '319736', '319936', '319937', '319946']
nosy_count = 7.0
nosy_names = ['vstinner', 'alex', 'martin.panter', 'serhiy.storchaka', 'xiang.zhang', 'cstratak', 'pablogsal']
pr_nums = ['7663', '7685']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue33630'
versions = ['Python 3.8']
Activity
vstinner commentedon May 24, 2018
http://buildbot.python.org/all/#/builders/53/builds/1062
(...)
test_close_file (test.test_posix.TestPosixSpawn) ... ok
test_dup2 (test.test_posix.TestPosixSpawn) ... ok
test_empty_file_actions (test.test_posix.TestPosixSpawn) ... ok
test_multiple_file_actions (test.test_posix.TestPosixSpawn) ... FAIL
test_no_such_executable (test.test_posix.TestPosixSpawn) ... ok
test_open_file (test.test_posix.TestPosixSpawn) ... ERROR
test_returns_pid (test.test_posix.TestPosixSpawn) ... ok
Warning -- files was modified by test_posix
Before: []
After: ['\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb\udcdb']
test test_posix failed
test_specify_environment (test.test_posix.TestPosixSpawn) ... ok
======================================================================
ERROR: test_open_file (test.test_posix.TestPosixSpawn)
----------------------------------------------------------------------
======================================================================
FAIL: test_multiple_file_actions (test.test_posix.TestPosixSpawn)
----------------------------------------------------------------------
First differing element 1:
32512
0
? ^^^^^
+ (31319, 0)
? ^
----------------------------------------------------------------------
Ran 101 tests in 2.175s
FAILED (failures=1, errors=1, skipped=9)
1 test failed again:
test_posix
serhiy-storchaka commentedon May 24, 2018
What is \udcdb\udcdb\udcdb...? Looks like a corrupted memory.
serhiy-storchaka commentedon May 24, 2018
Seems there are 2 or 3 different bugs here. Race condition, using freed memory, and maybe something other. I take this.
pablogsal commentedon May 27, 2018
@serhiy.storchaka Is the race condition/freed memory reusage in the test suite or in the posix module?
serhiy-storchaka commentedon May 28, 2018
The race condition is in tests (we can open a file before it be written in a child). The reed memory reusage is in the posix module. Didn't look at it close, but seems the content of a temporary Python object (the result of encoding/decoding) is passed to C library function, and this object is freed before the content be used.
vstinner commentedon Jun 12, 2018
I'm not sure that the attached PR fixes all race conditions.
serhiy-storchaka commentedon Jun 12, 2018
The part that is related to \udcdb\udcdb\udcdb... is a problem in posix_spawn() implementation, not in tests. It can't be fixed by changing tests.
pablogsal commentedon Jun 12, 2018
In PR 7663 I am only fixing the race condition.
pablogsal commentedon Jun 12, 2018
A separate PR is needed for the apparent reusing of freed memory in the implementation of posix_spawn. I am currently looking into it.
serhiy-storchaka commentedon Jun 12, 2018
For reference: posix_spawn() was implemented in bpo-20104.
pablogsal commentedon Jun 12, 2018
Should we merge PR6693 and PR6725 before fixing the memory problem?
7 remaining items
pablogsal commentedon Jun 13, 2018
If we want to support older versions of glibc a temporary (and somewhat inelegant) workaround is storing the temporaries in a list that the caller passes and destroy the list after calling
posix_spawn.vstinner commentedon Jun 13, 2018
Creating a copy seems to be a reasonable tradeoff.
Another option is to detect the bug at runtime and raise an exception if the bug is detected. But I dislike this option: I hate when a function is available in the os module but calling it raises a NotImplementedError :-( Moreover, posix_spawn() seems to be a lot of benefit, so I would like to be able to use it on "old" glibc versions.
pablogsal commentedon Jun 13, 2018
I have updated the PR with a workaround.
pablogsal commentedon Jun 13, 2018
Serhiy commented in the PR:
Should we use the list approach always to avoid problems with this?
serhiy-storchaka commentedon Jun 13, 2018
Yes, as I wrote on GitHub.
serhiy-storchaka commentedon Jun 15, 2018
As I wrote on GitHub, I think that it is better to use the list approach always for two reasons:
Additional #if's and duplications clutter the code.
It is hard to test the alternate branch if the other branch is used on most developer computers.
It is better to have a single branch and add a comment that explains a workaround. It can be removed after platforms with buggy glibc versions will grow out of use.
vstinner commentedon Jun 15, 2018
"It is better to have a single branch and add a comment that explains
a workaround. It can be removed after platforms with buggy glibc
versions will grow out of use."
Recently, I proposed to drop support for old Linux kernels, but some
people still use them and the code to support these kernels don't need
a lot of maintenance. So we keep the code :-)
https://mail.python.org/pipermail/python-dev/2018-January/151821.html
pablogsal commentedon Jun 15, 2018
I have updated PR7685 to use the temporary list as per Serhiy comments.
pablogsal commentedon Jun 15, 2018
As I commented on GitHub, it seems that the race in the test (problem covered in PR7663) disappears once the memory problem is fixed (PR33630). As is a race condition I am not sure that this affirmation is true, but running 3 test suites in parallel that executes test_posix 200 times does not raise any problem on the buildbot after applying the patch in PR33630
vadmium commentedon Jun 16, 2018
I doubt it is a race condition. “Waitpid” only returns after the child has terminated, and the file should be created as the child starts, before it terminates. Surely it is just the buggy glibc opening the DBDBDB file, when the test case expects it to create outfile.
serhiy-storchaka commentedon Jun 19, 2018
New changeset cb97073 by Serhiy Storchaka (Pablo Galindo) in branch 'master':
bpo-33630: Fix using of freed memory in old versions of glicb for posix_spawn(). (GH-7685)
cb97073
serhiy-storchaka commentedon Jun 19, 2018
Thank you Pablo! Seems failures in other tests that look like a race condition actually are caused by the glibc bug. I think that after passing tests on builtbots this issue can be closed.
vstinner commentedon Jun 19, 2018
PPC64 Fedora 3.x, PPC64LE Fedora 3.x, s390x Debian 3.x and s390x RHEL 3.x buildbots are back to green thanks to this fix.
Oh wow, so it was a glibc bug: that was unexpected :-( Hopefully, the workaround is tiny and cheap!
Right. I closed the PR 7663 and I close this issue.
Thank you all for investigating this issue, thanks Serhiy and Xiang for the review, and thanks Pablo for the fix!