Skip to content

Running the Python test suite leaks .pem files in /tmp #93353

Closed
Closed
Running the Python test suite leaks .pem files in /tmp#93353
Labels
3.12only security fixestestsTests in the Lib/test dirtype-bugAn unexpected behavior, bug, or error

Description

@vstinner
vstinner
opened on May 30, 2022

Running the Python test suite has two issues:

  • A test leaks 3 .pem files in /tmp
  • The test suite is not marked as failed when it leaks temporary files (in /tmp)

Moreover, test_tools enters an unlimited loop and fills the $TMPDIR directory if the $TMPDIR is a sub-directory of the Python source code directory. Example:

  • /home/vstinner/python/main/ : Python source code
  • /home/vstinner/python/main/TMP/ : Temporary directory ($TMPDIR)

Running test_freeze_simple_script() of test_tools copies TMP/ into TMP/TMP/ and then into TMP/TMP/TMP/, etc. Quickly, it fills TMP/ with a "loop" of files :-)

Linked PRs

Activity

  1. vstinner
    added
    type-bugAn unexpected behavior, bug, or error
    on May 30, 2022
  2. AA-Turner
    added
    testsTests in the Lib/test dir
    3.12only security fixes
    on May 30, 2022

  3. vstinner commented on May 30, 2022

    @vstinner
    vstinner
    on May 30, 2022
    MemberAuthor

    The problem comes from pip bootstrap when running the test_with_pip() test of test_venv.

    It seems like pip creates a .pem file in the temporary directory (like /tmp), but only when pip is imported from a ZIP file, which is the case when I run get-pip.py: https://bootstrap.pypa.io/get-pip.py

  4. vstinner commented on May 30, 2022

    @vstinner
    vstinner
    on May 30, 2022
    MemberAuthor

    I can reproduced the issue in the 3.9, 3.10, 3.11 and main branches (I didn't test older branches).

    • In Python 3.9, I cannot reproduce the issue in Python 3.9 at commit 4b4d60f: pip 20.2.3.
    • But I can reproduce the issue at commit d962b00: pip 21.1.

    It seems to be a change between pip 20.2.3 and pip 21.1.

  5. vstinner commented on May 30, 2022

    @vstinner
    vstinner
    on May 30, 2022
    MemberAuthor

    In get-pip.py, from pip._internal.commands.install import InstallCommand is enough to create the .pem file.

  6. vstinner commented on May 30, 2022

    @vstinner
    vstinner
    on May 30, 2022
    MemberAuthor

    Shell script to reproduce the issue:

    set -e -x

DIR=$PWD/TMP/
rm -rf $DIR
mkdir $DIR

rm -rf env
TMPDIR=$DIR TEMPDIR=$DIR ./python -m venv env --without-pip
TMPDIR=$DIR TEMPDIR=$DIR env/bin/python -m ensurepip -v

echo
echo
echo "=== TMPDIR ==="
find $DIR

    Output:

    (...)
=== TMPDIR ===
++ find /home/vstinner/python/main/TMP/
/home/vstinner/python/main/TMP/
/home/vstinner/python/main/TMP/tmpmb5ndxc7cacert.pem

  7. vstinner commented on May 30, 2022

    @vstinner
    vstinner
    on May 30, 2022
    MemberAuthor

    The PEM file is created indirectly by importlib.resources on import pip._vendor.requests.

    It's created by this code:

    >>> import sys; sys.path.insert(0, 'pip.zip')
>>> import pip._vendor.certifi
>>> pip._vendor.certifi.where()
pip.zip/pip/_vendor/certifi/core.py:50: DeprecationWarning: path is deprecated. Use files() instead. Refer to https://importlib-resources.readthedocs.io/en/latest/using.html#migrating-from-legacy for migration advice.
'/home/vstinner/python/main/TMP/tmp9osgb6wrcacert.pem'

    Debugger:

    Lib/importlib/resources/_common.py, line 84, in _tempfile (suffix='cacert.pem', fd=4, raw_path='/home/vstinner/python/main/TMP/tmpv7fwzaoocacert.pem')

    If Python exits normally, the garbage collector deletes the temporary PEM file. When running ensurepip / get-pip.py, it's not deleted. I don't know why.

    importlib.resources only creates a temporary file if pip is imported from a ZIP file.

  8. vstinner commented on May 30, 2022

    @vstinner
    vstinner
    on May 30, 2022
    MemberAuthor

    Workaround to manually delete the temporaryfile:

    pip._vendor.certifi.core._CACERT_CTX = None

  9. vstinner commented on May 30, 2022

    @vstinner
    vstinner
    on May 30, 2022
    MemberAuthor

    OpenSSL requires a filename to a PEM file. certifi uses importlib.resources to create a temporary named file if pip._vendor.certifi is imported from a ZIP file. That's convenient.

    The problem is more that the pip._vendor.certifi.core._CACERT_CTX context manager which keeps the temporary file alive until it's destroyed. Maybe sometimes _CACERT_CTX is part of a complex reference cycle, it's never destroyed, and so the temporary file is not deleted.

  10. serhiy-storchaka commented on May 31, 2022

    @serhiy-storchaka
    serhiy-storchaka
    on May 31, 2022
    Member

    Nice. How did you find this?

  11. vstinner commented on May 31, 2022

    @vstinner
    vstinner
    on May 31, 2022
    MemberAuthor

    I used a shell script similar to #93353 (comment) and I ran manually a bisection on the test list (./python -m test --list-tests) using a script taking a random.sample() of half of tests. I repeated the operation until I found a single test file, then I looked at the code.

    The leaked files was reported by more and more owners of buildbot workers.

    Once the PEM issue is solved, my plan is to enhance test.regrtest to do something similar than the shell script: define TMPFILE env var. Maybe in the main process spawning test processes.

  12. vstinner commented on May 31, 2022

    @vstinner
    vstinner
    on May 31, 2022
    MemberAuthor

    For pip/certifi, I hacked get-pip.py to add breakpoint() and I followed the code flow. It's not easy to identify which part of pip created the file. It's not just an import. It's an import + call to where() function, done in the module body that I failed to find which module. Maybe requests. It would be nice to be able to postpone when where() is called to workaround the issue: only call it when a filename to cacert.pem is needed.

  13. vstinner commented on May 31, 2022

    @vstinner
    vstinner
    on May 31, 2022
    MemberAuthor

    Reproducer without pip nor certifi, attached run.py and bug.py scripts:

    $ ./python run.py 
bug.py: temporary directory: ['tmp1wkab03ycacert.pem']
run.py: temporary directory: ['tmp1wkab03ycacert.pem']

    The expected output is an empty list in run.py (parent process). bug.py never calls _tempfile() finally block.

    I'm still bisecting the issue. It smells like logging prevents deleting an object somehow.

  14. vstinner commented on May 31, 2022

    @vstinner
    vstinner
    on May 31, 2022
    MemberAuthor

    Ok, this problem is quite complicated:

    • pip imports requests
    • requests imports certifi
    • requests calls certifi.where()
    • certifi.where() uses importlib.resources to create a temporary file using a context manager.
    • logging is imported by pip
    • logging calls os.register_at_fork() which keeps the whole logging namespace alive
    • somehow, the logging module keeps the whole pip package (!) and all of its sub-modules (!) alive until Python clears os.register_at_fork(): that happens VERY LATE, like the last function call before the last GC collection.
    • Python clears os.register_at_fork() callbacks: the context manager finally: block is called as expected....
    • Oops, os.remove is None at this point, the call fails.

    Fixing importlib.resources is simple: keep a reference to os.remove() in importlib.resources implementation to make sure that we can remove the file very late during Python finalization.

    This problem is tricky because:

    • importlib.resources only creates a named temporary file if pip is imported from a ZIP file. When pip is installed on disk as a file, the bug is gone.
    • the code is valid.
    • certifi relies on the garbage collector to gently calls the context manager finalizer which removes the temporary file.
    • Python doesn't log any error when a bug occurs very late during Python finalization.

    I'm working on a fix for importlib.resources.

  15. 12 remaining items

  16. vstinner
    added a commit that references this issue on Jun 14, 2022

    gh-93353: regrtest checks for leaked temporary files (#93776)

    e566ce5

  17. tiran commented on Jun 14, 2022

    @tiran
    tiran
    on Jun 14, 2022
    Member

    PR GH-93776 broke testing on WASI:

    _PYTHON_HOSTRUNNER='wasmtime run --env PYTHONPATH=/build_oot/host/build/lib.wasi-wasm32-3.12:/Lib --mapdir /::../.. --' _PYTHON_PROJECT_BASE=/buildmaster-config/master/workers/local-worker/3.x.local-worker.wasi/build/build_oot/host _PYTHON_HOST_PLATFORM=wasi-wasm32 PYTHONPATH=/buildmaster-config/master/workers/local-worker/3.x.local-worker.wasi/build/build_oot/host/build/lib.wasi-wasm32-3.12:../../Lib _PYTHON_SYSCONFIGDATA_NAME=_sysconfigdata_d_wasi_wasm32-wasi ../build/python  ../../Tools/scripts/run_tests.py -j 1 -u all -W --slowest --fail-env-changed --timeout=900 -j2 --junit-xml test-results.xml 
== CPython 3.12.0a0 (heads/main-dirty:a338e106b6, Jun 14 2022, 13:20:18) [GCC 9.4.0]
== Linux-5.17.13-300.fc36.x86_64-x86_64-with-glibc2.31 little-endian
== cwd: /buildmaster-config/master/workers/local-worker/3.x.local-worker.wasi/build/build_oot/host/build/test_python_111802æ
== CPU count: 8
== encodings: locale=UTF-8, FS=utf-8
Using random seed 6754872
0:00:00 load avg: 5.20 Run tests in parallel using 2 child processes (timeout: 15 min, worker timeout: 20 min)
Warning -- regrtest worker thread failed: Traceback (most recent call last):
Warning --   File "/buildmaster-config/master/workers/local-worker/3.x.local-worker.wasi/build/Lib/test/libregrtest/runtest_mp.py", line 325, in run
Warning --     mp_result = self._runtest(test_name)
Warning --                 ^^^^^^^^^^^^^^^^^^^^^^^^
Warning --   File "/buildmaster-config/master/workers/local-worker/3.x.local-worker.wasi/build/Lib/test/libregrtest/runtest_mp.py", line 280, in _runtest
Warning --     os.mkdir(tmp_dir)
Warning --     ^^^^^^^^^^^^^^^^^
Warning -- FileExistsError: [Errno 17] File exists: '/buildmaster-config/master/workers/local-worker/3.x.local-worker.wasi/build/build_oot/host/build/test_python_111802æ_tmpdir'
Kill <TestWorkerProcess #1 running test=test_typing pid=111806 time=2 ms> process group

  18. tiran commented on Jun 14, 2022

    @tiran
    tiran
    on Jun 14, 2022
    Member

    The commit e566ce5 relies on an environment variable to create unique temp directories per instance. The approach does not work on WASI for two reasons:

    1. WASI runtimes do not pass environment variables to WASM programs. python.wasm process does not see the TMPDIR variable.
    2. wasmtime uses an explicit, sandboxed mapping of host directories to runtime directories (sort of a chroot, but better). The TMPDIR outside would not be the same as the TMPDIR inside.

  19. vstinner commented on Jun 14, 2022

    @vstinner
    vstinner
    on Jun 14, 2022
    MemberAuthor

    WASI runtimes do not pass environment variables to WASM programs. python.wasm process does not see the TMPDIR variable.

    Ah, so this feature is not supported on WASI. Can you please test #93810 on WASI?

  20. tiran
    added a commit that references this issue on Jun 14, 2022

    gh-93353: Fix regrtest for -jN with N >= 2 (GH-93813)

    36934a1

  21. vstinner commented on Jun 15, 2022

    @vstinner
    vstinner
    on Jun 15, 2022
    MemberAuthor

    The initial issue (leaked PEM files) has been fixed.

    regrtest now detects leaked temporary files in the main branch. I don't want to backport this change since it causes some CI issues, and I'm still fixing regrtest. I have a few more changes to enhance regrtest which will use this issue number, but IMO the issue can be closed since the initial issue has been fixed.

  22. vstinner
    closed this as completedon Jun 15, 2022
  23. vstinner
    added a commit that references this issue on Jun 16, 2022

    gh-93353: regrtest supports checking tmp files with -j2 (#93909)

    4f85cec

  24. vstinner commented on Jun 16, 2022

    @vstinner
    vstinner
    on Jun 16, 2022
    MemberAuthor

    Follow-up: issue #93919 "On Windows, test_distutils leaks a temporary file: Microsoft".

  25. serhiy-storchaka
    added a commit that references this issue on Jun 26, 2022

    Revert 30935 defer bpo45162 to 312 (#94285)

    1ffde3e
  26. jaraco
    added a commit that references this issue on Feb 17, 2023

    pythongh-93353: Fix importlib.resources._tempfile() finalizer (python…

    4025963
  27. vstinner
    added 4 commits that reference this issue on Sep 2, 2023

    pythongh-93353: regrtest checks for leaked temporary files (python#93776

    9583edd

    pythongh-93353: Fix regrtest for -jN with N >= 2 (pythonGH-93813)

    7f3f0b3

    pythongh-93353: regrtest supports checking tmp files with -j2 (python…

    9007571

    [3.11] gh-108822: Backport libregrtest changes from the main branch (#…

    79f7a4c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    3.12only security fixestestsTests in the Lib/test dirtype-bugAn unexpected behavior, bug, or error

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions