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

ValueError: I/O operation on closed file. in ZipFile destructor #81954

Open
joernheissler mannequin opened this issue Aug 6, 2019 · 11 comments
Open

ValueError: I/O operation on closed file. in ZipFile destructor #81954

joernheissler mannequin opened this issue Aug 6, 2019 · 11 comments
Labels
3.11 only security fixes 3.12 bugs and security fixes 3.13 bugs and security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@joernheissler
Copy link
Mannequin

joernheissler mannequin commented Aug 6, 2019

BPO 37773
Nosy @pitrou, @vstinner, @serhiy-storchaka, @joernheissler, @tirkarthi, @akulakov

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 = None
created_at = <Date 2019-08-06.09:35:50.904>
labels = ['3.10', '3.9', 'type-crash', '3.11']
title = 'ValueError: I/O operation on closed file. in ZipFile destructor'
updated_at = <Date 2022-01-23.20:02:33.133>
user = 'https://github.com/joernheissler'

bugs.python.org fields:

activity = <Date 2022-01-23.20:02:33.133>
actor = 'iritkatriel'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = []
creation = <Date 2019-08-06.09:35:50.904>
creator = 'joernheissler'
dependencies = []
files = []
hgrepos = []
issue_num = 37773
keywords = ['3.8regression']
message_count = 10.0
messages = ['349104', '349105', '389594', '389605', '389611', '389689', '389690', '389691', '396649', '396657']
nosy_count = 6.0
nosy_names = ['pitrou', 'vstinner', 'serhiy.storchaka', 'joernheissler', 'xtreak', 'andrei.avk']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue37773'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

@joernheissler
Copy link
Mannequin Author

joernheissler mannequin commented Aug 6, 2019

When running this code:

from zipfile import ZipFile
import io

def foo():
    pass

data = io.BytesIO()
zf = ZipFile(data, "w")

I get this message:

Exception ignored in: <function ZipFile.__del__ at 0x7f9005caa160>
Traceback (most recent call last):
  File "/home/user/git/oss/cpython/Lib/zipfile.py", line 1800, in __del__
  File "/home/user/git/oss/cpython/Lib/zipfile.py", line 1817, in close
ValueError: I/O operation on closed file.

Comment out def foo: pass, and there is no error.

It looks like the bug was introduced with commit ada319b (bpo-32388).

@joernheissler joernheissler mannequin added 3.8 (EOL) end of life 3.9 only security fixes type-crash A hard crash of the interpreter, possibly with a core dump labels Aug 6, 2019
@tirkarthi
Copy link
Member

This looks like a regression in 3.8 so I have added 3.8 regression tag.

@joernheissler
Copy link
Mannequin Author

joernheissler mannequin commented Mar 27, 2021

Still reproducible in cpython 3.10.0a3 (debian unstable) and 3.10.0a6 (pyenv).

@joernheissler joernheissler mannequin added the 3.10 only security fixes label Mar 27, 2021
@serhiy-storchaka
Copy link
Member

That's what happened. Function foo creates a reference loop. It has reference to the module dict, and the dict has reference to the function. The dict has also references to BytesIO and ZipFile objects. At shutdown stage the garbage collector is called. It founds the reference loop which contains the module dict, function foo and BytesIO and ZipFile objects. It tries to break the loop by calling the destructor of arbitrary object in the loop. In that case it is the BytesIO object. It does not help, but finally the destructor of the ZipFile object is called (either directly or after breaking the loop and destroying the module dict). The ZipFile destructor tries to close the BytesIO object, but it is already closed in its destructor.

It is a dangerous situation which potentially can cause data corruption (if the underlying stream is closed before flushing buffers). Clearing the module dict before invoking garbage collecting would solve this issue. Making the garbage collector more clever and and avoid destroying objects if it does not break the loop would help too, but it is more complex problem and such change could not be backported to 3.8.

Victor, were there any changes in the garbage collector or interpreter shutdown code in 3.8?

@joernheissler
Copy link
Mannequin Author

joernheissler mannequin commented Mar 27, 2021

Thanks Serhiy for the explanation, it's making sense now.

Guess whatever I did back then (no idea what I was working on) was basically a mistake; I should have closed my ZipFile properly, e.g. by using context managers. So maybe it's not really a cpython bug.

I ran a git-bisect back then and came up with https://hg.python.org/lookup/ada319bb6d0ebcc68d3e0ef2b4279ea061877ac8 as cause.

@vstinner
Copy link
Member

Victor, were there any changes in the garbage collector or interpreter shutdown code in 3.8?

I took some notes there: https://pythondev.readthedocs.io/finalization.html

I proposed bpo-42671 "Make the Python finalization more deterministic" but it will break some use cases, so I'm not comfortable with this idea yet :-(

@vstinner
Copy link
Member

IMO ZipFile destructor should emit a ResourceWarning if it's not closed explicitly.

Nothing in the Python specification gives you any warranty that destructors will be ever called... Depending on the Python implementation and depending on many things, you never know when a destructor is called.

Just call the close() method explicitly.

@vstinner
Copy link
Member

FYI TextIOWrapper has a similar issue: bpo-17852. There was an attempt to fix the issue in 2017, but it had to be reverted.

@akulakov
Copy link
Contributor

In my testing on 3.9.1 and the current main branch, I found:

  • I can reproduce this issue if a function or a lambda or class with a method are defined.
  • There is no error if a class is defined with no methods. (I'm not sure why).

The test code runs without errors if the following check is done before seeking the fp in close():

    if not self.fp.closed:

here:
https://github.com/python/cpython/blob/main/Lib/zipfile.py#L1823

The fix also requires adding closed property to zipfile._Tellable

With these two small changes, all test_zipfile tests pass.

I'm not sure this is the proper fix, because this is a complex issue..

I will go ahead and put up a PR if this seems like a good approach.

@akulakov
Copy link
Contributor

I forgot to add, to fix the tests I also had to add closed=False attr to test_zipfile.Tellable.

@iritkatriel iritkatriel added 3.11 only security fixes and removed 3.8 (EOL) end of life labels Jan 23, 2022
@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
@AlexWaygood AlexWaygood added type-bug An unexpected behavior, bug, or error and removed type-crash A hard crash of the interpreter, possibly with a core dump 3.9 only security fixes labels Jul 11, 2022
@ChrisBarker-NOAA
Copy link
Contributor

(Python 3.10.10 -- excuse the noise if it's been fixed in 3.11)

What's the status of this? I'm getting similar warnings in my tests:

  /Users/chris.barker/miniconda3/envs/gnome/lib/python3.10/site-packages/_pytest/unraisableexception.py:78: PytestUnraisableExceptionWarning: Exception ignored in: <function ZipFile.__del__ at 0x100aade10>
  
  Traceback (most recent call last):
    File "/Users/chris.barker/miniconda3/envs/gnome/lib/python3.10/zipfile.py", line 1819, in __del__
      self.close()
    File "/Users/chris.barker/miniconda3/envs/gnome/lib/python3.10/zipfile.py", line 1836, in close
      self.fp.seek(self.start_dir)
    File "/Users/chris.barker/miniconda3/envs/gnome/lib/python3.10/tempfile.py", line 779, in seek
      return self._file.seek(*args)
  ValueError: I/O operation on closed file.
  
    warnings.warn(pytest.PytestUnraisableExceptionWarning(msg))

This is pretty buried -- I'm not using zipfile directly, it's being used by numpy.savez(), which I am using to store a cache in a temp dir.

The odd thing is that this warning does not pop up when I run that test by itself, only when it is run as part of the suite -- which makes me thing it's maybe related to this issue with the GC and old references, but ???

Anyway, I think @akulakov has the right idea -- if you put a guard around the close() call, the warning will go away. Maybe that would hide another more important error, but it seems not unlikely that an object may be deleted having been partially closed already.

@iritkatriel iritkatriel added 3.12 bugs and security fixes 3.13 bugs and security fixes and removed 3.10 only security fixes labels Oct 27, 2023
@iritkatriel iritkatriel added the stdlib Python modules in the Lib dir label Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.11 only security fixes 3.12 bugs and security fixes 3.13 bugs and security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
Projects
Status: No status
Development

No branches or pull requests

7 participants