[patch] Fix file object leak in `aifc.open` when given invalid AIFF file.

[Uberi]

BPO	29110
Nosy	@methane, @serhiy-storchaka, @Uberi
PRs	bpo-29110: Fix file object leak in aifc.open. #162 bpo-29110: add test for Aifc_write. #293 [3.6] bpo-29110: Fix file object leak in aifc.open #310 [3.5] bpo-29110: Fix file object leak in aifc.open #311 [2.7] bpo-29110: Fix file object leak in aifc.open #356 [Do Not Merge] Sample of CPython life with blurb. #703 [Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
Files	fix_aifc_leak.patch: Fix for first issue described in issue. fix_aifc_leak_and_file_object_close.patch: Fix for second issue described in issue.

^{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 = 'https://github.com/methane'
closed_at = <Date 2017-02-28.12:03:44.083>
created_at = <Date 2016-12-30.00:55:17.377>
labels = ['3.7', 'type-bug', 'library']
title = '[patch] Fix file object leak in `aifc.open` when given invalid AIFF file.'
updated_at = <Date 2017-03-31.16:36:22.542>
user = 'https://github.com/uberi'

bugs.python.org fields:

activity = <Date 2017-03-31.16:36:22.542>
actor = 'dstufft'
assignee = 'methane'
closed = True
closed_date = <Date 2017-02-28.12:03:44.083>
closer = 'methane'
components = ['Library (Lib)']
creation = <Date 2016-12-30.00:55:17.377>
creator = 'azhang'
dependencies = []
files = ['46087', '46088']
hgrepos = []
issue_num = 29110
keywords = ['patch']
message_count = 10.0
messages = ['284299', '284371', '288340', '288611', '288622', '288705', '290358', '290375', '290376', '290394']
nosy_count = 3.0
nosy_names = ['methane', 'serhiy.storchaka', 'azhang']
pr_nums = ['162', '293', '310', '311', '356', '703', '552']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue29110'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6', 'Python 3.7']

[Uberi]

Summary
-------

This shows up as two closely-related issues:

• aifc.open leaks file object when invalid AIFF file encountered. This is probably a bug.
• aifc.close closes file object even when it didn't open the file object to begin with. While this is technically documented behaviour 1, it's inconsistent with how similar modules like wave work 2.

I have confirmed the issue is present in all the selected Python versions.

Steps to reproduce
------------------

For the first issue, run this code:

#!/usr/bin/env python3

    # to simplify this example, use this script itself as a random non-AIFF file (though any non-AIFF file will work fine)
    FILE = __file__

    # print warnings on stderr
    import warnings
    warnings.resetwarnings()

    # try to open the non-AIFF file as an AIFF file, which should fail, but shouldn't give any ResourceWarnings
    import aifc
    try: aifc.open(FILE, "rb")
    except: pass

For the second issue, run this code:

#!/usr/bin/env python3

    from os import path
    FILE = path.expanduser("~/cpython/Lib/test/audiodata/pluck-pcm8.aiff")

    # open the file as a file-like object, then open it again with ``aifc.open``
    import aifc
    with open(FILE, "rb") as f:
        assert not f.closed, "Before opening with AIFC"
        with aifc.open(f, "rb"):
            pass
        assert not f.closed, "After opening with AIFC"

Expected result
---------------

For the first code sample, code should give no output - aifc.open should throw an exception due to the passed filename being an invalid AIFF file, but that exception should be caught and suppressed. by the try/except statements.

For the second code sample, all assertions should pass - the file should be opened with open, "opened" again with aifc.open, and should not be closed when the inner context manager exits.

Actual result
-------------

For the first code sample:

    $ python3 test.py
    /home/anthony/Desktop/test.py:13: ResourceWarning: unclosed file <_io.BufferedReader name='/home/anthony/Desktop/test.py'>
      except: pass

In other words, code executes as described in "Expected result", but also prints a warning to stderr about the file object not being closed.

For the second code sample:

    $ python3 "/home/anthony/Desktop/test.py"
    Traceback (most recent call last):
      File "/home/anthony/Desktop/test.py", line 11, in <module>
        assert not f.closed, "After opening with AIFC"
    AssertionError: After opening with AIFC

In other words, code runs normally until the inner context manager exits, and the file object gets closed, even though the file object wasn't opened by aifc.

Solution
--------

Attached are patches that fix each issue separately - the first patch fixes the first mentioned issue, while the second patch fixes both at once.

Backwards compatibility:

• The first patch, "fix_aifc_leak.patch", makes no functionality changes, so there shouldn't be any backwards compatibility concerns.

• The second patch, "fix_aifc_leak_and_file_object_close.patch", slightly changes the behaviour of aifc_instance.close() so that it closes in only a subset of cases it would originally. While it is possible for this to lead to leaks in certain cases (example below), the impact shoul be low, as existing codebases seem to use context managers and similar constructs that clean everything up properly.

  #!/usr/bin/env python3

    from os import path
    FILE = path.expanduser("~/cpython/Lib/test/audiodata/pluck-pcm8.aiff")
    import aifc
    f = open(FILE, "rb")
    with aifc.open(f, "rb"):
        pass
    # with the patch applied, `f` is not closed at this point anymore

[serhiy-storchaka]

Thank you for your patches Anthony. Could you please create a unittest for the first issue?

The second issue is well known. Since the behavior is documented, I think it can be changed only in Python 3.7. The patch needs the "versionchanged" directives in the documentation and documenting the change in What's New (in the section "Porting to Python 3.7" or like). This change can cause reference leaks in existing code.

This is not the only inconsistency between aifc and similar modules.

[methane]

New changeset 03f68b6 by INADA Naoki in branch 'master':
bpo-29110: Fix file object leak in aifc.open when given invalid AIFF file. (GH-162)
03f68b6

[methane]

Anyone mind Python 2.7?

Since Python 2.7 doesn't have mock and check_no_resource_warning context manager, I can't cherry-pick easily.

Backporting is bother to me, and the issue doesn't seem so critical.
Can I just close this issue?

[serhiy-storchaka]

test.support.swap_attr() provides the functionality similar to mock. check_no_resource_warning() can be backported to 2.7.

But if there are problems with testing in 2.7 you can backport without tests.

[methane]

OK, since Aifc_write is harder to test, and the exception is very unlikely happens, I fixed only Aifc_read.

[methane]

New changeset 02eb4b0 by INADA Naoki in branch '2.7':
bpo-29110: Fix file object leak in aifc.open (GH-356)
02eb4b0

[methane]

New changeset c9131b6 by INADA Naoki in branch '3.6':
[3.6] bpo-29110: Fix file object leak in aifc.open (#310)
c9131b6