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

pablogsal · 2018-06-12T12:44:45Z

https://bugs.python.org/issue33630

This PR is triying to fix this error (more info in the bpo):

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)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/test/test_posix.py", line 1542, in test_open_file
    with open(outfile) as f:
FileNotFoundError: [Errno 2] No such file or directory: '@test_42482_tmp'

…n_file`

pablogsal · 2018-06-12T12:46:28Z

@serhiy-storchaka Do you mind reviewing this? I am not completely happy about the code to wait for the inode to be created but I don't know if I am missing something obviously better.

I am referring to this code:

        max_tries = 3
        while not os.path.exists(outfile) and max_tries:
            max_tries -= 1
            time.sleep(0.1)

Note: This code has changed in 28a8ced

vstinner · 2018-06-12T14:00:51Z

Lib/test/test_posix.py

                                os.environ, file_actions)
        self.assertEqual(os.waitpid(pid, 0), (pid, 0))
+
+        max_tries = 3


For tests, I prefer to use a timeout rather than a number of tries. Like:

deadline = time.monotonic()
while 1:
if time.monotonic() > deadline: ... timeout ...
... the code ...

Changed in 28a8ced

…est_open_file`

serhiy-storchaka · 2018-06-12T14:32:11Z

There is still a race condition. It is possible that the file is created, but is still empty.

Relying on time is not reliable. The child can be paused at any moment.

pablogsal · 2018-06-12T14:44:27Z

If the child is paused the self.assertEqual(os.waitpid(pid, 0), (pid, 0)) will still block until the child actually finishes, right? Notice that we are waiting until the inode is created after the child has finished. Also, we are flushing now stdout before the child finishes.

pablogsal · 2018-06-12T14:56:11Z

Commit 1ad0a08 handles the situation in which the file is created but nothing is still written into it.

…to it

vstinner

I would prefer to first see the posix_spawn() memory issue fixed, before fixing the Python test.

bedevere-bot · 2018-06-13T09:14:14Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

pablogsal · 2018-06-13T12:53:25Z

@vstinner It seems that the memory corruption is due to a bug in glibc<2.20. There is a discussion on the issue to skip the test for older versions of glibc or add a workaround.

pablogsal · 2018-06-13T13:28:29Z

I have made the requested changes; please review again

bedevere-bot · 2018-06-13T13:28:32Z

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

zhangyangyu · 2018-06-13T14:10:27Z

Modules/posixmodule.c

I think the version is glibc < 2.20. It would also be nice to mention CVE-2014-4043 so later code readers could easily find out details about the glibc bug if they are interested.

zhangyangyu

My question is we should use the temp list always or just use it for glibc < 2.20? We could use macros __GLIBC__ and __GLIBC_MINOR__.

pablogsal · 2018-06-13T14:41:29Z

@zhangyangyu Done in 869dd1c

pablogsal · 2018-06-13T15:58:25Z

I can confirm that this PR fixes both bugs. Tested on gcc110.fsffrance.org:

[pablogsal@gcc1-power7 cpython]$ git rev-parse HEAD
2c7f50d712bb9f64c53404c2629ce1998e6e2b0f

[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]$ ldd --version
ldd (GNU libc) 2.17

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

OK (skipped=9)

[pablogsal@gcc1-power7 cpython]$ ./python -m test test_posix -R 3:3

Run tests sequentially
0:00:00 load avg: 3.94 [1/1] test_posix
beginning 6 repetitions
123456
......

== Tests result: SUCCESS ==

1 test OK.

Total duration: 9 sec
Tests result: SUCCESS

serhiy-storchaka

I think the temp list should always be used. This would make code clearer and make testing easier.
Make two different PRs for these changes. Changes to the test still don't LGTM.

serhiy-storchaka · 2018-06-13T17:58:53Z

Modules/posixmodule.c

__GLIBC__ and __GLIBC_MINOR__ give you the version of glibc used at compile time. But can not the different version be dynamically linked at run time?

Yup. I updated the issue to check what people think.

@serhiy-storchaka Isn't this the case for all other condition macros? For example HAVE_POSIX_SPAWN could be defined at compile time but at run time you could link to a version doesn't have posix_spawn.

Perhaps you can link only to the lib with equal or larger version.

serhiy-storchaka · 2018-06-13T17:59:27Z

Modules/posixmodule.c

PEP 7: Add space before *, not after it.

Fixed in the new PR (see comment below).

serhiy-storchaka · 2018-06-13T18:00:07Z

Modules/posixmodule.c

PyList_Append() can fail.

Fixed in the new PR (see comment below).

bedevere-bot · 2018-06-13T18:06:18Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

pablogsal · 2018-06-13T18:17:57Z

@serhiy-storchaka I am going to open another PR for the changes in posixmodule.c leaving this one for the tests only.

pablogsal · 2018-06-13T18:19:28Z

Created #7685 for the memory problems. This PR is only for the test race conditions.

pablogsal · 2018-06-15T12:15:46Z

I have simplified this bugfix:

As far as I understand this won't finish until the inode is created and the data is written to disk:

import sys, os
sys.stdout.write("hello")
sys.stdout.flush()
os.fsync(sys.stdout.fileno())

and on the parent waitpid blocks until the child finished with 0 (otherwise it fails the check):

self.assertEqual(os.waitpid(pid, 0), (pid, 0))

Unless I am missing something there is no race condition. When waitpid exists successfully, the file has to be there.

vstinner · 2018-06-19T08:58:57Z

Let's see if the commit cb97073 fixed tests on buildbots.

vstinner · 2018-06-19T09:08:53Z

PPC64 Fedora 3.x, PPC64LE Fedora 3.x, s390x Debian 3.x and s390x RHEL 3.x buildbots are back to green thanks to the commit cb97073, so I don't think that this change is still needed. I close the PR.

Fix race condition in `Lib/test/test_posix.py:TestPosixSpawn.test_ope…

5e8d5ee

…n_file`

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

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

pablogsal changed the title ~~bpo-33630: Fix race condition in `Lib/test/test_posix.py:TestPosixSpawn.test_ope…~~ bpo-33630: Fix race condition in TestPosixSpawn.test_open Jun 12, 2018

vstinner reviewed Jun 12, 2018

View reviewed changes

fixup! Fix race condition in `Lib/test/test_posix.py:TestPosixSpawn.t…

28a8ced

…est_open_file`

Fix race condition when the file is created but nothing is written in…

1ad0a08

…to it

pablogsal force-pushed the bpo33630 branch from 187e881 to 1ad0a08 Compare June 12, 2018 14:56

vstinner requested changes Jun 13, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Jun 13, 2018

bedevere-bot added awaiting change review and removed awaiting changes labels Jun 13, 2018

zhangyangyu reviewed Jun 13, 2018

View reviewed changes

pablogsal force-pushed the bpo33630 branch from 869dd1c to 2c7f50d Compare June 13, 2018 15:55

serhiy-storchaka requested changes Jun 13, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Jun 13, 2018

pablogsal force-pushed the bpo33630 branch from 2c7f50d to 1ad0a08 Compare June 13, 2018 18:19

pablogsal mentioned this pull request Jun 13, 2018

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

Merged

pablogsal force-pushed the bpo33630 branch from 56963c7 to c31c6cb Compare June 15, 2018 12:01

Use fsync in the child to force writing to disk

5110fa2

pablogsal force-pushed the bpo33630 branch from c31c6cb to 5110fa2 Compare June 15, 2018 12:11

vstinner closed this Jun 19, 2018

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

Uh oh!

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

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

pablogsal commented Jun 12, 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!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Jun 12, 2018

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jun 13, 2018

Uh oh!

pablogsal commented Jun 13, 2018

Uh oh!

pablogsal commented Jun 13, 2018

Uh oh!

bedevere-bot commented Jun 13, 2018

Uh oh!

zhangyangyu Jun 13, 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!

zhangyangyu left a comment

Choose a reason for hiding this comment

Uh oh!

pablogsal commented Jun 13, 2018

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!

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!

zhangyangyu Jun 14, 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!

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!

bedevere-bot commented Jun 13, 2018

Uh oh!

pablogsal commented Jun 13, 2018

Uh oh!

pablogsal commented Jun 13, 2018

Uh oh!

pablogsal commented Jun 15, 2018

Uh oh!

vstinner commented Jun 19, 2018

Uh oh!

vstinner commented Jun 19, 2018

Uh oh!

Reviewers

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

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

pablogsal commented Jun 12, 2018 •

edited

Loading

pablogsal commented Jun 12, 2018 •

edited

Loading

pablogsal commented Jun 12, 2018 •

edited

Loading

pablogsal commented Jun 12, 2018 •

edited

Loading

zhangyangyu Jun 13, 2018 •

edited

Loading

pablogsal commented Jun 13, 2018 •

edited

Loading

zhangyangyu Jun 14, 2018 •

edited

Loading