classification
Title: [Windows] os.spawn*() tests of test_os leak references on Windows
Type: resource usage Stage: resolved
Components: Tests, Windows Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: eryksun, haypo, paul.moore, steve.dower, tim.golden, zach.ware
Priority: normal Keywords:

Created on 2017-06-08 16:18 by haypo, last changed 2017-06-29 08:53 by haypo. This issue is now closed.

Files
File name Uploaded Description Edit
test_os.py haypo, 2017-06-08 16:18
Pull Requests
URL Status Linked Edit
PR 2184 merged haypo, 2017-06-14 09:54
PR 2212 merged haypo, 2017-06-15 11:20
PR 2287 merged haypo, 2017-06-20 10:17
PR 2357 merged haypo, 2017-06-23 13:05
PR 2486 merged haypo, 2017-06-29 08:31
Messages (12)
msg295451 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-06-08 16:18
The leak can be reproduced with the minimum attached test_os.py:

C:\haypo\python>PCbuild\amd64\python_d.exe -m test -R 3:3 test_os
Run tests sequentially
0:00:00 [1/1] test_os
beginning 6 repetitions
123456
......
test_os leaked [2, 2, 2] memory blocks, sum=6
test_os failed

1 test failed:
    test_os

Total duration: 172 ms
Tests result: FAILURE
msg295462 - (view) Author: Eryk Sun (eryksun) * Date: 2017-06-08 20:09
The memory leak is in os_spawnv_impl and os_spawnve_impl in Modules/posixmodule.c. The call fails with a ValueError when the first argument in the argv list is an empty string, in which case these functions both mistakenly pass i (0) to free_string_array() as the count of strings to free. But the conversion was successful, so it needs to include the current string in the count, i.e. `i + 1`.

The fix for this could also address the following two issues, which are mostly cosmetic in nature. os_spawnve_impl needs its TypeError message to be special cased the same as in os_spawnv_impl, i.e. "spawnve() arg 2 must contain only strings". Currently it uses the default message from the failed conversion: "expected str, bytes or os.PathLike object, not %.200s". Also, its ValueError message needs to reference "spawnve()" instead of "spawnv()".
msg295508 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-06-09 09:39
Eryk: do you want to work on a PR to fix it? It seems like you understand well the bug!
msg295999 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-06-14 12:26
New changeset 526b22657cb18fe79118c2ea68511aca09430c2c by Victor Stinner in branch 'master':
bpo-30602: Fix refleak in os.spawnve() (#2184)
https://github.com/python/cpython/commit/526b22657cb18fe79118c2ea68511aca09430c2c
msg296082 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-06-15 11:26
Oh, I fixed a leak in test_spawnve_noargs() of test_os, but test_spawnv_noargs() still leaks :-) os.spawnv() is very similar to os.spawnve(). https://github.com/python/cpython/pull/2212 fixes os.spawnv() as well.

I tested that "python -m test -R 3:3 test_os" doesn't leak anymore with this second fix.
msg296086 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-06-15 13:30
New changeset 8acb4cf2b3436652568d7a70228b166316181466 by Victor Stinner in branch 'master':
bpo-30602: Fix refleak in os.spawnv() (#2212)
https://github.com/python/cpython/commit/8acb4cf2b3436652568d7a70228b166316181466
msg296103 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-06-15 14:47
It seems like I introduced a bug:
https://github.com/python/cpython/pull/2184#pullrequestreview-44308536
msg296408 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-06-20 10:12
The fixes must be backported to Python 3.6, test_os still leaks references on this branch:

test_os leaked [6, 6, 6] memory blocks, sum=18
msg296707 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-06-23 13:04
New changeset c8d6ab2e25ff212702d387e516e258b1d8c52910 by Victor Stinner in branch 'master':
bpo-30602: Fix lastarg in os.spawnve() (#2287)
https://github.com/python/cpython/commit/c8d6ab2e25ff212702d387e516e258b1d8c52910
msg296714 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-06-23 13:21
New changeset c472fb6b2744b36c7a0823c20e0d5ac9be3ea623 by Victor Stinner in branch '3.6':
bpo-30602: Fix lastarg in os.spawnve() (#2287) (#2357)
https://github.com/python/cpython/commit/c472fb6b2744b36c7a0823c20e0d5ac9be3ea623
msg296717 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-06-23 13:23
Thank you Eryk Sun for the careful reviews! All known issues on os.spawn*() on Windows are now be fixed, so I closed this issue.
msg297264 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-06-29 08:53
New changeset b78fbaaeab9df8cfbbdae3d5faf2d1537d73e43b by Victor Stinner in branch '3.6':
bpo-30602: Fix refleak in os.spawnv() (#2212) (#2486)
https://github.com/python/cpython/commit/b78fbaaeab9df8cfbbdae3d5faf2d1537d73e43b
History
Date User Action Args
2017-06-29 08:53:25hayposetmessages: + msg297264
2017-06-29 08:31:07hayposetpull_requests: + pull_request2545
2017-06-23 13:23:03hayposetstatus: open -> closed
resolution: fixed
messages: + msg296717

stage: resolved
2017-06-23 13:21:26hayposetmessages: + msg296714
2017-06-23 13:05:36hayposetpull_requests: + pull_request2404
2017-06-23 13:04:49hayposetmessages: + msg296707
2017-06-20 10:17:56hayposetpull_requests: + pull_request2335
2017-06-20 10:12:19hayposetmessages: + msg296408
2017-06-15 14:47:02hayposetmessages: + msg296103
2017-06-15 13:30:43hayposetmessages: + msg296086
2017-06-15 11:26:44hayposetmessages: + msg296082
2017-06-15 11:20:39hayposetpull_requests: + pull_request2256
2017-06-14 12:26:25hayposetmessages: + msg295999
2017-06-14 09:54:29hayposetpull_requests: + pull_request2234
2017-06-09 09:39:11hayposetmessages: + msg295508
2017-06-08 20:09:36eryksunsetnosy: + eryksun
messages: + msg295462
2017-06-08 16:18:47haypocreate