classification
Title: [EASY (C)] test_execve_invalid_env() of test_os leaks references
Type: resource usage Stage: resolved
Components: Tests Versions: Python 3.7, Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: emilyemorehouse, ericvw, serhiy.storchaka, vstinner
Priority: normal Keywords: easy (C)

Created on 2017-06-26 16:41 by vstinner, last changed 2017-06-27 16:36 by emilyemorehouse. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 2416 merged ericvw, 2017-06-26 17:46
PR 2425 merged emilyemorehouse, 2017-06-27 01:41
PR 2447 merged emilyemorehouse, 2017-06-27 15:15
Messages (17)
msg296915 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-26 16:41
haypo@selma$ ./python -m test -R 3:3 -m test_execve_invalid_env test_os
test_os leaked [2, 2, 2] references, sum=6
test_os leaked [2, 2, 2] memory blocks, sum=6

I don't understand if it's related to bpo-30602 or not?
msg296916 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-26 16:46
The leak can reproduce only using this test:

    def test_execve_invalid_env(self):
        args = [sys.executable, '-c', 'pass']

        # equal character in the enviroment variable name
        newenv = os.environ.copy()
        newenv["FRUIT=ORANGE"] = "lemon"
        with self.assertRaises(ValueError):
            os.execve(args[0], args, newenv)

It seems like the bug is in parse_envlist() function of Modules/posixmodule.c, when a key contains the '=' character.
msg296917 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-26 16:49
FYI I found this leak using my bisect_test.py script:
https://mail.python.org/pipermail/python-dev/2017-June/148368.html

Another hint: it seems like the leak was introduced recently ;-) Try "git log Modules/posixmodule.c".

Reminder: core developers, please don't fix the issue, but *explain how to fix it* ;-)
msg296918 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-26 16:50
Oops, wrong link to my email:
https://mail.python.org/pipermail/python-dev/2017-June/148489.html

I used this command:

 ./python bisect_test.py -R 3:3 test_os

Where the script comes from:
https://github.com/haypo/misc/blob/master/python/bisect_test.py
msg296920 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-26 17:31
Oh, my bad. Thank you for finding this leak Victor.
msg296921 - (view) Author: Eric N. Vander Weele (ericvw) * Date: 2017-06-26 17:36
I think I may have found it.

$ git show 77703942c5997dff00c48f10df1b29b11645624c

Appears to indicate key2 and val2 are *not* decremented in the error conditions.  Should I PR a fix for this or let Serhiy resolve?
msg296922 - (view) Author: Emily Morehouse (emilyemorehouse) * (Python committer) Date: 2017-06-26 17:39
I also found what Eric did, specifically lines 4913-4918 in the commit he mentioned introduced the bug:

    if (PyBytes_GET_SIZE(key2) == 0 ||
        strchr(PyBytes_AS_STRING(key2) + 1, '=') != NULL)
    {
        PyErr_SetString(PyExc_ValueError, "illegal environment variable name");
        goto error;
    }

One of the tricks is that the test that fails was added after the commit that introduced the bug.
msg296985 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-27 01:35
New changeset a7874c73c0c729bbec2fd4b077bd0eec276cfff4 by Victor Stinner (Eric N. Vander Weele) in branch 'master':
bpo-30769: Fix reference leak introduced in 77703942c59 (#2416)
https://github.com/python/cpython/commit/a7874c73c0c729bbec2fd4b077bd0eec276cfff4
msg296999 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-27 04:58
The code in 3.5 differs from the code in master and 3.6, but it contains the same bug.
msg297000 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-27 04:59
New changeset 2d348f7a723db839aa18ce8213b8663ccb0a3d35 by Serhiy Storchaka (Emily Morehouse) in branch '3.6':
[3.6] bpo-30769: Fix reference leak introduced in 77703942c59 (GH-2416) (#2425)
https://github.com/python/cpython/commit/2d348f7a723db839aa18ce8213b8663ccb0a3d35
msg297040 - (view) Author: Emily Morehouse (emilyemorehouse) * (Python committer) Date: 2017-06-27 14:33
I think I need a bit more direction for the 3.5 backport. The original test below passes:
    ./python -m test -R 3:3 -m test_execve_invalid_env test_os
msg297047 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-27 15:05
Just read the code. Find the function parse_envlist() in posixmodule.c and find similar code checking for '='.

Even if the leak is not detected by the test (but it is detected on Linux, maybe you use Windows?) it exists.

os.spawnve() also should be affected by this bug and fixed by the same patch, because it uses the same code.
msg297051 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-27 16:08
New changeset eb3c52a0d273491e745e0cbff2b73900bb96aa45 by Victor Stinner (Emily Morehouse) in branch '3.5':
[3.5] bpo-30769: Fix reference leak introduced in 7770394 (GH-2416) (#2447)
https://github.com/python/cpython/commit/eb3c52a0d273491e745e0cbff2b73900bb96aa45
msg297052 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-27 16:10
Ooookay, the bug is now fix on all branches. Thanks Eric & Emily for your PR and thanks Serhiy for the help ;-)
msg297054 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-27 16:17
Many thanks for you Victor for your wonderful tool.
msg297056 - (view) Author: Emily Morehouse (emilyemorehouse) * (Python committer) Date: 2017-06-27 16:34
Resolving this since the fix and backports have been merged in.
msg297057 - (view) Author: Emily Morehouse (emilyemorehouse) * (Python committer) Date: 2017-06-27 16:36
And thank you to Serhiy and Victor for the assistance and attention to this issue!
History
Date User Action Args
2017-06-27 16:36:11emilyemorehousesetmessages: + msg297057
2017-06-27 16:34:51emilyemorehousesetmessages: + msg297056
2017-06-27 16:17:01serhiy.storchakasetmessages: + msg297054
2017-06-27 16:10:00vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg297052

stage: backport needed -> resolved
2017-06-27 16:08:50vstinnersetmessages: + msg297051
2017-06-27 15:15:14emilyemorehousesetpull_requests: + pull_request2503
2017-06-27 15:05:17serhiy.storchakasetmessages: + msg297047
2017-06-27 14:33:34emilyemorehousesetmessages: + msg297040
2017-06-27 05:00:02serhiy.storchakasetstage: commit review -> backport needed
2017-06-27 04:59:27serhiy.storchakasetmessages: + msg297000
2017-06-27 04:58:47serhiy.storchakasetmessages: + msg296999
2017-06-27 01:41:55emilyemorehousesetpull_requests: + pull_request2477
2017-06-27 01:35:22vstinnersetmessages: + msg296985
2017-06-27 00:04:25emilyemorehousesetstage: commit review
2017-06-26 17:46:50ericvwsetpull_requests: + pull_request2464
2017-06-26 17:39:12emilyemorehousesetnosy: + emilyemorehouse
messages: + msg296922
2017-06-26 17:36:06ericvwsetnosy: + ericvw
messages: + msg296921
2017-06-26 17:31:36serhiy.storchakasetmessages: + msg296920
2017-06-26 16:50:08vstinnersetmessages: + msg296918
2017-06-26 16:49:13vstinnersetnosy: + serhiy.storchaka
messages: + msg296917
2017-06-26 16:46:06vstinnersetkeywords: + easy (C)

messages: + msg296916
title: test_execve_invalid_env() of test_os leaks references -> [EASY (C)] test_execve_invalid_env() of test_os leaks references
2017-06-26 16:41:42vstinnercreate