FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-29110: Fix file object leak in `aifc.open`. by Uberi · Pull Request #162 · python/cpython · GitHub

Repository navigation

bpo-29110: Fix file object leak in aifc.open. - #162

Merged
methane merged 1 commit into
python:masterfrom
Uberi:bpo-29110
Feb 22, 2017
Merged

methane merged 1 commit into
python:masterfrom
Uberi:bpo-29110

Conversation

Uberi commented Feb 18, 2017

Copy link
Copy Markdown
Contributor

When given an invalid AIFF file, aifc.open doesn't clean up after itself. This patch makes it close the file object if it was opened by the library.

Also, it adds one unit test, test.aifc_test.AifcMiscTest.test_close_opened_files_on_error. The patch can be verified with ./python -m test -j0 test_aifc.

serhiy-storchaka added the type-bug An unexpected behavior, bug, or error label Feb 19, 2017
Comment thread Lib/test/test_aifc.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Could you use with self.assertRaises(aifc.Error): instead of try ~ except: pass?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done!

methane left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please add an entry at Lib section in Misc/NEWS.

Comment thread Lib/test/test_aifc.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please wrap at 79 columns

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It seems to be 77 characters over here. Is there something I'm missing?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Uh, I'm sorry. no problem.

Comment thread Lib/test/test_aifc.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

same here.

Uberi commented Feb 22, 2017

Copy link
Copy Markdown
Contributor Author

Rebased and squashed on upstream, added NEWS entry, and fixed the style thing noted above!

Comment thread Misc/NEWS Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please add credit like Patch by <your name> at the end.

Uberi commented Feb 22, 2017

Copy link
Copy Markdown
Contributor Author

Added and squashed!

methane commented Feb 22, 2017

Copy link
Copy Markdown
Member

We use "Squash and merge" in Github. So squash is not needed for next time.
Normal commit make it easy to review changes from last commit.

BTW, since this is bugfix, would you create backport (cherry-pick) pull request for 3.5 and 3.6 branch?
I can do it for you, but "cherry pick + pull request + squash merge" workflow drops original author.

methane merged commit 03f68b6 into python:master Feb 22, 2017
Comment thread Lib/aifc.py
raise

# treat .aiff file extensions as non-compressed audio
if f.endswith('.aiff'):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is unrelated change and it introduces a regression. self._aifc is not defined if the file have non .aiff extension.

methane Feb 23, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

self.initfp(file_object) initializes self._aifc = 1.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ah, sorry, I missed this. It would be better to write just self._aifc = not f.endswith('.aiff') if you want to modernize this code.

But now it looks to me that initfp() never fails. All changes of Aifc_write were not needed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

MemoryError and MaxRecursionError can happen, but that's very unlikely and hard to test.
That's why I didn't request testing Aifc_write.

Comment thread Lib/test/test_aifc.py
#This file contains chunk types aifc doesn't recognize.
self.f = aifc.open(findfile('Sine-1000Hz-300ms.aif'))

def test_close_opened_files_on_error(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The test tests only Aifc_read. Needed a test for Aifc_write.

Is it possible to make tests common for aifc, sunau and wave?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

#293 adds test for Aifc_write.

Since this type of test is depending on implementation (raise Exception on Edge case),
I didn't tried to make this generic.

methane added a commit that referenced this pull request Feb 26, 2017
methane pushed a commit to methane/cpython that referenced this pull request Feb 26, 2017
methane added a commit to methane/cpython that referenced this pull request Feb 26, 2017
methane pushed a commit to methane/cpython that referenced this pull request Feb 26, 2017
methane added a commit to methane/cpython that referenced this pull request Feb 26, 2017
methane added a commit that referenced this pull request Feb 26, 2017
(cherry picked from commit 03f68b6) (GH-162)
(cherry picked from commit 5dc33ee) (GH-293)
methane added a commit that referenced this pull request Feb 26, 2017
(cherry picked from commit 03f68b6) (GH-162)
(cherry picked from commit 5dc33ee) (GH-293)
lazka pushed a commit to lazka/cpython that referenced this pull request Feb 10, 2024
When building shared modules (*.pyd) for MinGW, they link against
the import library for the main python (e.g. -lpython3.11). When
linking shared modules on Linux, this doesn't happen, but those
shared libraries are linked with undefined references which
then are fulfilled by the host process when they are loaded.

Add a dependency to make sure that $(LDLIBRARY), e.g.
libpython$(LDVERSION).dll.a, exists before starting to build
the shared modules.

Previously, if libpython$(LDVERSION).dll.a didn't exist when
the shared modules were linked, some of them failed to link,
but this wasn't reported as an error in the build, and a
later invocation of setup.py would retry building them, which
then would succeed. E.g. there was a seemingly benign race
condition in the build.

However, in some rare cases, the race condition no longer
was benign. The build also produces a corresponding static library,
$(LIBRARY), e.g. libpython$(VERSION)$(ABIFLAGS).a - which in
the end would be named similarly, libpython3.11.a, when
$(LDLIBRARY) would be libpython3.11.dll.a.

If the static library exists but the import library doesn't, then
most modules would still fail to link, but a few ones wouldn't
(select, _multiprocessing, _overlapped and _socket). The ones
that did succeed linking with the static library would crash at
runtime though.

By making sure that the right, intended library to fulfill the
linker parameter "-lpython3.11" exists before building the shared
modules, we avoid the whole race condition that in some rare
cases could produce a Python build that crashes at runtime.

For Linux targets, this extra dependency is unnecessary, but should
not cause other problems ($(LDLIBRARY) is set to the same as
$(LIBRARY) if not building any shared library).

This fixes MSYS2 cpython-mingw issue python#162.
lazka pushed a commit to lazka/cpython that referenced this pull request Aug 25, 2024
When building shared modules (*.pyd) for MinGW, they link against
the import library for the main python (e.g. -lpython3.11). When
linking shared modules on Linux, this doesn't happen, but those
shared libraries are linked with undefined references which
then are fulfilled by the host process when they are loaded.

Add a dependency to make sure that $(LDLIBRARY), e.g.
libpython$(LDVERSION).dll.a, exists before starting to build
the shared modules.

Previously, if libpython$(LDVERSION).dll.a didn't exist when
the shared modules were linked, some of them failed to link,
but this wasn't reported as an error in the build, and a
later invocation of setup.py would retry building them, which
then would succeed. E.g. there was a seemingly benign race
condition in the build.

However, in some rare cases, the race condition no longer
was benign. The build also produces a corresponding static library,
$(LIBRARY), e.g. libpython$(VERSION)$(ABIFLAGS).a - which in
the end would be named similarly, libpython3.11.a, when
$(LDLIBRARY) would be libpython3.11.dll.a.

If the static library exists but the import library doesn't, then
most modules would still fail to link, but a few ones wouldn't
(select, _multiprocessing, _overlapped and _socket). The ones
that did succeed linking with the static library would crash at
runtime though.

By making sure that the right, intended library to fulfill the
linker parameter "-lpython3.11" exists before building the shared
modules, we avoid the whole race condition that in some rare
cases could produce a Python build that crashes at runtime.

For Linux targets, this extra dependency is unnecessary, but should
not cause other problems ($(LDLIBRARY) is set to the same as
$(LIBRARY) if not building any shared library).

This fixes MSYS2 cpython-mingw issue python#162.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type-bug An unexpected behavior, bug, or error

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL