Repository: python-pillow/Pillow

Problem statement:
Close files after loading
Draft for #2019

This PR closes files at least for files which we are opening from filename at least for not sequence formats. This breaks consistency across different codecs, though.

Is there any reliable place where we can close files for sequence formats?



Hints:

Patch:
diff --git a/PIL/DcxImagePlugin.py b/PIL/DcxImagePlugin.py
--- a/PIL/DcxImagePlugin.py
+++ b/PIL/DcxImagePlugin.py
@@ -41,7 +41,8 @@ class DcxImageFile(PcxImageFile):
 
     format = "DCX"
     format_description = "Intel DCX"
-
+    _close_exclusive_fp_after_loading = False
+    
     def _open(self):
 
         # Header
diff --git a/PIL/FliImagePlugin.py b/PIL/FliImagePlugin.py
--- a/PIL/FliImagePlugin.py
+++ b/PIL/FliImagePlugin.py
@@ -37,7 +37,8 @@ class FliImageFile(ImageFile.ImageFile):
 
     format = "FLI"
     format_description = "Autodesk FLI/FLC Animation"
-
+    _close_exclusive_fp_after_loading = False
+    
     def _open(self):
 
         # HEAD
diff --git a/PIL/GifImagePlugin.py b/PIL/GifImagePlugin.py
--- a/PIL/GifImagePlugin.py
+++ b/PIL/GifImagePlugin.py
@@ -47,6 +47,8 @@ class GifImageFile(ImageFile.ImageFile):
 
     format = "GIF"
     format_description = "Compuserve GIF"
+    _close_exclusive_fp_after_loading = False
+    
     global_palette = None
 
     def data(self):
diff --git a/PIL/ImImagePlugin.py b/PIL/ImImagePlugin.py
--- a/PIL/ImImagePlugin.py
+++ b/PIL/ImImagePlugin.py
@@ -109,6 +109,7 @@ class ImImageFile(ImageFile.ImageFile):
 
     format = "IM"
     format_description = "IFUNC Image Memory"
+    _close_exclusive_fp_after_loading = False
 
     def _open(self):
 
diff --git a/PIL/Image.py b/PIL/Image.py
--- a/PIL/Image.py
+++ b/PIL/Image.py
@@ -506,6 +506,7 @@ class Image(object):
     """
     format = None
     format_description = None
+    _close_exclusive_fp_after_loading = True
 
     def __init__(self):
         # FIXME: take "new" parameters / other image?
@@ -560,14 +561,25 @@ def close(self):
         """
         try:
             self.fp.close()
+            self.fp = None
         except Exception as msg:
             logger.debug("Error closing: %s", msg)
 
+        if getattr(self, 'map', None):
+            self.map = None
+    
         # Instead of simply setting to None, we're setting up a
         # deferred error that will better explain that the core image
         # object is gone.
         self.im = deferred_error(ValueError("Operation on closed image"))
 
+    if sys.version_info >= (3,4,0):
+        def __del__(self):
+            if (hasattr(self, 'fp') and hasattr(self, '_exclusive_fp') 
+                and self.fp and self._exclusive_fp):
+                self.fp.close()
+            self.fp = None
+
     def _copy(self):
         self.load()
         self.im = self.im.copy()
@@ -2391,6 +2403,7 @@ def open(fp, mode="r"):
     if mode != "r":
         raise ValueError("bad mode %r" % mode)
 
+    exclusive_fp = False
     filename = ""
     if isPath(fp):
         filename = fp
@@ -2404,11 +2417,13 @@ def open(fp, mode="r"):
 
     if filename:
         fp = builtins.open(filename, "rb")
+        exclusive_fp = True
 
     try:
         fp.seek(0)
     except (AttributeError, io.UnsupportedOperation):
         fp = io.BytesIO(fp.read())
+        exclusive_fp = True
 
     prefix = fp.read(16)
 
@@ -2437,8 +2452,11 @@ def _open_core(fp, filename, prefix):
             im = _open_core(fp, filename, prefix)
 
     if im:
+        im._exclusive_fp = exclusive_fp
         return im
 
+    if exclusive_fp:
+        fp.close()
     raise IOError("cannot identify image file %r"
                   % (filename if filename else fp))
 
diff --git a/PIL/ImageFile.py b/PIL/ImageFile.py
--- a/PIL/ImageFile.py
+++ b/PIL/ImageFile.py
@@ -88,10 +88,13 @@ def __init__(self, fp=None, filename=None):
             # filename
             self.fp = open(fp, "rb")
             self.filename = fp
+            self._exclusive_fp = True
         else:
             # stream
             self.fp = fp
             self.filename = filename
+            # can be overridden
+            self._exclusive_fp = None
 
         try:
             self._open()
@@ -100,6 +103,9 @@ def __init__(self, fp=None, filename=None):
                 KeyError,  # unsupported mode
                 EOFError,  # got header but not the first frame
                 struct.error) as v:
+            # close the file only if we have opened it this constructor
+            if self._exclusive_fp:
+                self.fp.close()
             raise SyntaxError(v)
 
         if not self.mode or self.size[0] <= 0:
@@ -115,6 +121,8 @@ def verify(self):
 
         # raise exception if something's wrong.  must be called
         # directly after open, and closes file when finished.
+        if self._exclusive_fp:
+            self.fp.close()
         self.fp = None
 
     def load(self):
@@ -234,14 +242,16 @@ def load(self):
         self.tile = []
         self.readonly = readonly
 
-        self.fp = None  # might be shared
+        self.load_end()
+
+        if self._exclusive_fp and self._close_exclusive_fp_after_loading:
+            self.fp.close()
+        self.fp = None
 
         if not self.map and not LOAD_TRUNCATED_IMAGES and err_code < 0:
             # still raised if decoder fails to return anything
             raise_ioerror(err_code)
 
-        self.load_end()
-
         return Image.Image.load(self)
 
     def load_prepare(self):
diff --git a/PIL/MicImagePlugin.py b/PIL/MicImagePlugin.py
--- a/PIL/MicImagePlugin.py
+++ b/PIL/MicImagePlugin.py
@@ -39,6 +39,7 @@ class MicImageFile(TiffImagePlugin.TiffImageFile):
 
     format = "MIC"
     format_description = "Microsoft Image Composer"
+    _close_exclusive_fp_after_loading = False
 
     def _open(self):
 
diff --git a/PIL/MpoImagePlugin.py b/PIL/MpoImagePlugin.py
--- a/PIL/MpoImagePlugin.py
+++ b/PIL/MpoImagePlugin.py
@@ -39,7 +39,8 @@ class MpoImageFile(JpegImagePlugin.JpegImageFile):
 
     format = "MPO"
     format_description = "MPO (CIPA DC-007)"
-
+    _close_exclusive_fp_after_loading = False
+    
     def _open(self):
         self.fp.seek(0)  # prep the fp in order to pass the JPEG test
         JpegImagePlugin.JpegImageFile._open(self)
diff --git a/PIL/SpiderImagePlugin.py b/PIL/SpiderImagePlugin.py
--- a/PIL/SpiderImagePlugin.py
+++ b/PIL/SpiderImagePlugin.py
@@ -97,6 +97,7 @@ class SpiderImageFile(ImageFile.ImageFile):
 
     format = "SPIDER"
     format_description = "Spider 2D image"
+    _close_exclusive_fp_after_loading = False
 
     def _open(self):
         # check header
diff --git a/PIL/TiffImagePlugin.py b/PIL/TiffImagePlugin.py
--- a/PIL/TiffImagePlugin.py
+++ b/PIL/TiffImagePlugin.py
@@ -884,6 +884,7 @@ class TiffImageFile(ImageFile.ImageFile):
 
     format = "TIFF"
     format_description = "Adobe TIFF"
+    _close_exclusive_fp_after_loading = False
 
     def _open(self):
         "Open the first image in a TIFF file"
@@ -969,6 +970,7 @@ def _seek(self, frame):
             self.__frame += 1
         self.fp.seek(self._frame_pos[frame])
         self.tag_v2.load(self.fp)
+        self.__next = self.tag_v2.next
         # fill the legacy tag/ifd entries
         self.tag = self.ifd = ImageFileDirectory_v1.from_v2(self.tag_v2)
         self.__frame = frame
@@ -1008,6 +1010,12 @@ def load(self):
             return self._load_libtiff()
         return super(TiffImageFile, self).load()
 
+    def load_end(self):
+        # allow closing if we're on the first frame, there's no next
+        # This is the ImageFile.load path only, libtiff specific below.
+        if self.__frame == 0 and not self.__next:
+            self._close_exclusive_fp_after_loading = True
+
     def _load_libtiff(self):
         """ Overload method triggered when we detect a compressed tiff
             Calls out to libtiff """
@@ -1085,16 +1093,14 @@ def _load_libtiff(self):
         self.tile = []
         self.readonly = 0
         # libtiff closed the fp in a, we need to close self.fp, if possible
-        if hasattr(self.fp, 'close'):
-            if not self.__next:
+        if self._exclusive_fp:
+            if self.__frame == 0 and not self.__next:
                 self.fp.close()
-        self.fp = None  # might be shared
+                self.fp = None  # might be shared
 
         if err < 0:
             raise IOError(err)
 
-        self.load_end()
-
         return Image.Image.load(self)
 
     def _setup(self):
diff --git a/PIL/XpmImagePlugin.py b/PIL/XpmImagePlugin.py
--- a/PIL/XpmImagePlugin.py
+++ b/PIL/XpmImagePlugin.py
@@ -116,8 +116,6 @@ def load_read(self, bytes):
         for i in range(ysize):
             s[i] = self.fp.readline()[1:xsize+1].ljust(xsize)
 
-        self.fp = None
-
         return b"".join(s)
 
 #


Test patch:
diff --git a/Tests/test_file_jpeg.py b/Tests/test_file_jpeg.py
--- a/Tests/test_file_jpeg.py
+++ b/Tests/test_file_jpeg.py
@@ -3,6 +3,7 @@
 
 from io import BytesIO
 import os
+import sys
 
 from PIL import Image
 from PIL import ImageFile
@@ -501,5 +502,27 @@ def test_save_tiff_with_dpi(self):
         self.assertEqual(im.info['dpi'], reloaded.info['dpi'])
 
 
+@unittest.skipUnless(sys.platform.startswith('win32'), "Windows only")
+class TestFileCloseW32(PillowTestCase):
+    def setUp(self):
+        if "jpeg_encoder" not in codecs or "jpeg_decoder" not in codecs:
+            self.skipTest("jpeg support not available")
+            
+    def test_fd_leak(self):
+        tmpfile = self.tempfile("temp.jpg")
+        import os
+
+        with Image.open("Tests/images/hopper.jpg") as im:
+            im.save(tmpfile)
+
+        im = Image.open(tmpfile)
+        fp = im.fp
+        self.assertFalse(fp.closed)
+        self.assertRaises(Exception, lambda: os.remove(tmpfile))
+        im.load()
+        self.assertTrue(fp.closed)
+        # this should not fail, as load should have closed the file. 
+        os.remove(tmpfile)
+
 if __name__ == '__main__':
     unittest.main()
diff --git a/Tests/test_file_libtiff.py b/Tests/test_file_libtiff.py
--- a/Tests/test_file_libtiff.py
+++ b/Tests/test_file_libtiff.py
@@ -401,6 +401,19 @@ def test_multipage(self):
 
         TiffImagePlugin.READ_LIBTIFF = False
 
+    def test_multipage_nframes(self):
+        # issue #862
+        TiffImagePlugin.READ_LIBTIFF = True
+        im = Image.open('Tests/images/multipage.tiff')
+        frames = im.n_frames
+        self.assertEqual(frames, 3)
+        for idx in range(frames):
+            im.seek(0)
+            # Should not raise ValueError: I/O operation on closed file
+            im.load()
+
+        TiffImagePlugin.READ_LIBTIFF = False
+
     def test__next(self):
         TiffImagePlugin.READ_LIBTIFF = True
         im = Image.open('Tests/images/hopper.tif')
diff --git a/Tests/test_file_tiff.py b/Tests/test_file_tiff.py
--- a/Tests/test_file_tiff.py
+++ b/Tests/test_file_tiff.py
@@ -1,6 +1,7 @@
 import logging
 from io import BytesIO
 import struct
+import sys
 
 from helper import unittest, PillowTestCase, hopper, py3
 
@@ -468,6 +469,44 @@ def test_saving_icc_profile(self):
 
         self.assertEqual(b'Dummy value', reloaded.info['icc_profile'])
 
+    def test_close_on_load(self):
+        # same as test_fd_leak, but runs on unixlike os
+        tmpfile = self.tempfile("temp.tif")
+
+        with Image.open("Tests/images/uint16_1_4660.tif") as im:
+            im.save(tmpfile)
+
+        im = Image.open(tmpfile)
+        fp = im.fp
+        self.assertFalse(fp.closed)
+        im.load()
+        self.assertTrue(fp.closed)
+
+        
+
+@unittest.skipUnless(sys.platform.startswith('win32'), "Windows only")
+class TestFileTiffW32(PillowTestCase):
+    def test_fd_leak(self):
+        tmpfile = self.tempfile("temp.tif")
+        import os
+
+        # this is an mmaped file. 
+        with Image.open("Tests/images/uint16_1_4660.tif") as im:
+            im.save(tmpfile)
+
+        im = Image.open(tmpfile)
+        fp = im.fp
+        self.assertFalse(fp.closed)
+        self.assertRaises(Exception, lambda: os.remove(tmpfile))
+        im.load()
+        self.assertTrue(fp.closed)
+
+        # this closes the mmap
+        im.close()
+        
+        # this should not fail, as load should have closed the file pointer,
+        # and close should have closed the mmap
+        os.remove(tmpfile)
 
 if __name__ == '__main__':
     unittest.main()