Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use "with" to avoid possible fd leaks #67020

Closed
serhiy-storchaka opened this issue Nov 9, 2014 · 23 comments
Closed

Use "with" to avoid possible fd leaks #67020

serhiy-storchaka opened this issue Nov 9, 2014 · 23 comments
Labels
3.8 only security fixes 3.9 only security fixes build The build process and cross-build stdlib Python modules in the Lib dir tests Tests in the Lib/test dir type-security A security issue

Comments

@serhiy-storchaka
Copy link
Member

BPO 22831
PRs
  • bpo-22831: Use "with" to avoid possible fd leaks in distutils. #10921
  • bpo-22831: Use "with" to avoid possible fd leaks in tools (part 1). #10926
  • bpo-22831: Use "with" to avoid possible fd leaks in tools (part 2). #10927
  • bpo-22831: Use "with" to avoid possible fd leaks in tests (part 1). #10928
  • bpo-22831: Use "with" to avoid possible fd leaks in tests (part 2). #10929
  • Files
  • fd_leaks.diff
  • fd_leaks.ignore-all-space.diff
  • fd_leaks_distutils.patch
  • fd_leaks_lib.patch
  • fd_leaks_tests1.patch
  • fd_leaks_tools2.patch
  • fd_leaks_tests2.patch
  • fd_leaks_tools1.patch
  • fd_leaks_tests1_2.patch
  • fd_leaks_tools1_2.patch
  • fd_leaks_tools2_2.patch
  • fd_leaks_tests2_2.patch
  • 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 = None
    closed_at = <Date 2019-03-30.06:34:13.702>
    created_at = <Date 2014-11-09.19:22:27.681>
    labels = ['type-security', '3.8', '3.9', 'library', 'build', 'tests']
    title = 'Use "with" to avoid possible fd leaks'
    updated_at = <Date 2020-01-28.07:18:39.356>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2020-01-28.07:18:39.356>
    actor = 'Hinsonlcrystal'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-03-30.06:34:13.702>
    closer = 'serhiy.storchaka'
    components = ['Build', 'Demos and Tools', 'Library (Lib)', 'Tests']
    creation = <Date 2014-11-09.19:22:27.681>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = ['37158', '38589', '38592', '38593', '38594', '38595', '38596', '38597', '38601', '38602', '38603', '41480']
    hgrepos = []
    issue_num = 22831
    keywords = ['patch']
    message_count = 23.0
    messages = ['230901', '238602', '238625', '238627', '238643', '238655', '238670', '238671', '238898', '238901', '240050', '257400', '257402', '257587', '331130', '331134', '331137', '332193', '332240', '337169', '337170', '339179', '339180']
    nosy_count = 1.0
    nosy_names = ['Hinsonlcrystal']
    pr_nums = ['10921', '10926', '10927', '10928', '10929']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'security'
    url = 'https://bugs.python.org/issue22831'
    versions = ['Python 3.8', 'Python 3.9']

    @serhiy-storchaka
    Copy link
    Member Author

    Here is large patch which convert a code which potentially can leak file descriptor to use the with statement so files are always closed. This can make effect mainly on alternative Python implementation without reference counting. But even on CPython this will get rid from resource leaking warnings.

    @serhiy-storchaka serhiy-storchaka added performance Performance or resource usage stdlib Python modules in the Lib dir tests Tests in the Lib/test dir labels Nov 9, 2014
    @berkerpeksag
    Copy link
    Member

    Looks good. A couple of comments:

    • Doc/includes/email-* changes have already been committed.

    • There is a commented-out line in Tools/scripts/nm2def.py:

      • f.close()
        + # f.close()
    • The patch is huge, I'd commit Tools/ and Lib/test/ parts separately.

    • Most of the open(..., 'r') usages can be changed to open(...)

    • It would be good to add context management protocol support to http.client.HTTPConnection.

    @vstinner
    Copy link
    Member

    fd_leaks.diff is impossible to review, even Rietveld gave up, it's too big :-p

    Could you please your patch into smaller patches? Split by directory for example.

    @vadmium
    Copy link
    Member

    vadmium commented Mar 20, 2015

    Posting a version of Serhiy’s patch made with “hg diff -p --ignore-all-space”. It is a bit shorter, and should not include all the re-indented lines, which may help reviewing.

    @serhiy-storchaka
    Copy link
    Member Author

    The patch is synchronized with the tip and divided on smaller parts.

    There is a commented-out line in Tools/scripts/nm2def.py:

    It is in a pair to commented out open() two lines above.

    Most of the open(..., 'r') usages can be changed to open(...)

    Done.

    • It would be good to add context management protocol support to
      http.client.HTTPConnection.

    In separate issue.

    @vadmium
    Copy link
    Member

    vadmium commented Mar 20, 2015

    There were a couple mistakes I found; see Reitveld. I was going to say to leave the open(mode="r") parameter as it is, since an explicit "r" is more readable, but since you already took some out, I’m not going to complain.

    While many of the changes look worthwhile, I do worry that some of them will never be tested (at least until say Python 3.14 or something). E.g. I just tried running Tools/scripts/dutree.py cos it looked interesting, but was confronted with the “unorderable types: int() < NoneType()” Python 2-ism.

    @serhiy-storchaka
    Copy link
    Member Author

    Here are updated patches with fixed mistakes found by Martin.

    While many of the changes look worthwhile, I do worry that some of them will
    never be tested (at least until say Python 3.14 or something). E.g. I just
    tried running Tools/scripts/dutree.py cos it looked interesting, but was
    confronted with the “unorderable types: int() < NoneType()” Python 2-ism.

    Yes, sad, but Tools/scripts/ is full of Python 2-isms.

    @serhiy-storchaka
    Copy link
    Member Author

    п'ятниця, 20-бер-2015 12:36:25 ви написали:

    Martin Panter added the comment:

    There were a couple mistakes I found; see Reitveld. I was going to say to
    leave the open(mode="r") parameter as it is, since an explicit "r" is more
    readable, but since you already took some out, I’m not going to complain.

    While many of the changes look worthwhile, I do worry that some of them will
    never be tested (at least until say Python 3.14 or something). E.g. I just
    tried running Tools/scripts/dutree.py cos it looked interesting, but was
    confronted with the “unorderable types: int() < NoneType()” Python 2-ism.

    ----------


    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue22831\>


    @vadmium
    Copy link
    Member

    vadmium commented Mar 22, 2015

    The new fd_leaks_tools1_2.patch seems to have dropped the changes for Tools/scripts/treesync.py, compared to the previous fd_leaks_tools1.patch.

    @serhiy-storchaka
    Copy link
    Member Author

    Oh, I forgot to publish my comments. I dropped the changes for treesync.py because treesync.py is not work with Python 3. I have written separate patch for it and will open separate issue after writing tests.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 4, 2015

    New changeset ea94f6c87f5d by Serhiy Storchaka in branch 'default':
    Issue bpo-22831: Use "with" to avoid possible fd leaks.
    https://hg.python.org/cpython/rev/ea94f6c87f5d

    @ezio-melotti
    Copy link
    Member

    Serhiy, what's the status of this?

    @serhiy-storchaka
    Copy link
    Member Author

    Only the most important patch for the stdlib was committed.

    Patches for distutils, tests and tools are not committed still.

    fd_leaks_distutils.patch
    fd_leaks_tools1_2.patch
    fd_leaks_tools2_2.patch
    fd_leaks_tests1_2.patch
    fd_leaks_tests2_2.patch

    @vadmium
    Copy link
    Member

    vadmium commented Jan 6, 2016

    I had another look at the five patches you mentioned. I made a couple review comments about expanding the scope of some “with” statements.

    There are a couple changes that add explicit file closing, where it was previously up to the garbage collector. I.e. code like open(...).read(). I think those changes are the most important, although they are scattered over the various patches.

    On the other hand, some of the changes in the test suite, particularly test_dbm_dumb.py and test_xmlrpc.py, hardly seem worth it. The main benefit of the “with” statement would be if the test code fails, which hopefully won’t happen that often. :)

    In the test suite, perhaps it would be better to call self.addCleanup(f.close) or similar in many cases. That way you wouldn’t need contextlib.closing() as much, and there would be less file history clutter and “cavern code”, due to the extra indentation.

    @serhiy-storchaka
    Copy link
    Member Author

    PR 10921 is based on fd_leaks_distutils.patch.

    @serhiy-storchaka serhiy-storchaka added the 3.8 only security fixes label Dec 5, 2018
    @serhiy-storchaka
    Copy link
    Member Author

    PR 10926 is based on fd_leaks_tools1_2.patch.
    PR 10927 is based on fd_leaks_tools2_2.patch.
    PR 10928 is based on fd_leaks_tests1_2.patch.
    PR 10929 is based on fd_leaks_tests2_2.patch.

    Some of changes in these patches were already applied in other issues.

    @serhiy-storchaka
    Copy link
    Member Author

    self.addCleanup(f.close) can not be used if the same file should be opened several times in the test. It can not be used also if the file is deleted in the test or in tearDown().

    @serhiy-storchaka
    Copy link
    Member Author

    Éric, could you please take a look at PR 10921?

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset c5d5dfd by Serhiy Storchaka in branch 'master':
    bpo-22831: Use "with" to avoid possible fd leaks in distutils. (GH-10921)
    c5d5dfd

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 9e4861f by Serhiy Storchaka in branch 'master':
    bpo-22831: Use "with" to avoid possible fd leaks in tests (part 1). (GH-10928)
    9e4861f

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 5b10b98 by Serhiy Storchaka in branch 'master':
    bpo-22831: Use "with" to avoid possible fd leaks in tests (part 2). (GH-10929)
    5b10b98

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset afbb7a3 by Serhiy Storchaka in branch 'master':
    bpo-22831: Use "with" to avoid possible fd leaks in tools (part 1). (GH-10926)
    afbb7a3

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 172bb39 by Serhiy Storchaka in branch 'master':
    bpo-22831: Use "with" to avoid possible fd leaks in tools (part 2). (GH-10927)
    172bb39

    @Hinsonlcrystal Hinsonlcrystal mannequin added build The build process and cross-build 3.9 only security fixes labels Jan 28, 2020
    @Hinsonlcrystal Hinsonlcrystal mannequin added type-security A security issue and removed performance Performance or resource usage labels Jan 28, 2020
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes 3.9 only security fixes build The build process and cross-build stdlib Python modules in the Lib dir tests Tests in the Lib/test dir type-security A security issue
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants