diff --git a/Tests/test_file_libtiff.py b/Tests/test_file_libtiff.py index bdedf94f35c..6c749a62f4c 100644 --- a/Tests/test_file_libtiff.py +++ b/Tests/test_file_libtiff.py @@ -1,6 +1,7 @@ from __future__ import annotations import base64 +import gc import io import itertools import os @@ -1268,6 +1269,21 @@ def test_save_zero(self, compression: str | None, tmp_path: Path) -> None: with pytest.raises(ValueError, match="cannot write empty image"): im.save(out, compression=compression) + def test_save_error_cleanup(self, tmp_path: Path) -> None: + with (tmp_path / "temp.tif").open("w+b", buffering=0) as output: + with pytest.raises(ValueError, match="cannot write empty image") as exc: + Image.new("RGB", (0, 0)).save( + output, format="TIFF", compression="tiff_adobe_deflate" + ) + + # The traceback keeps the encoder alive. Collecting it used to move + # the file position, corrupting unrelated reads if the fd was reused. + # Cleanup must happen during save, not later during GC. + output.seek(1234) + del exc + gc.collect() + assert output.tell() == 1234 + @pytest.mark.skipif(sys.platform != "win32", reason="Checks a Windows handle limit") def test_save_many_compressed(self, tmp_path: Path) -> None: im = hopper() diff --git a/Tests/test_image_tobytes.py b/Tests/test_image_tobytes.py index d32b6c09ba0..e6c64397b39 100644 --- a/Tests/test_image_tobytes.py +++ b/Tests/test_image_tobytes.py @@ -1,8 +1,52 @@ from __future__ import annotations +import pytest + +from PIL import Image, ImageFile + from .helper import hopper def test_sanity() -> None: data = hopper().tobytes() assert isinstance(data, bytes) + + +@pytest.mark.parametrize("failure", (None, "setimage", "encode", "status")) +def test_encoder_cleanup(failure: str | None, monkeypatch: pytest.MonkeyPatch) -> None: + cleanup_called = False + + class TestPyEncoder(ImageFile.PyEncoder): + def encode(self, bufsize: int) -> tuple[int, int, bytes]: + if failure == "encode": + raise ValueError(failure) + if failure == "status": + return (0, -2, b"") + return (6, 1, b"pixels") + + def setimage( + self, + im: Image.core.ImagingCore, + extents: tuple[int, int, int, int] | None = None, + ) -> None: + if failure == "setimage": + raise ValueError(failure) + return super().setimage(im, extents) + + def cleanup(self) -> None: + nonlocal cleanup_called + cleanup_called = True + + im = Image.new("RGB", (1, 1)) + + monkeypatch.setattr(Image, "ENCODERS", {"raw": TestPyEncoder}) + if failure in ("setimage", "encode"): + with pytest.raises(ValueError, match=failure): + im.tobytes() + elif failure == "status": + with pytest.raises(RuntimeError, match="encoder error -2 in tobytes"): + im.tobytes() + else: + assert im.tobytes() == b"pixels" + + assert cleanup_called diff --git a/docs/releasenotes/13.0.0.rst b/docs/releasenotes/13.0.0.rst index 694093b01ab..17aa74152b7 100644 --- a/docs/releasenotes/13.0.0.rst +++ b/docs/releasenotes/13.0.0.rst @@ -134,6 +134,17 @@ escape sequences. Other changes ============= +Clean up TIFF encoders before closing their output files +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +TIFF saving now explicitly cleans up the libtiff encoder, including when saving +raises an exception. Previously, delayed encoder destruction could access a closed +file descriptor after it had been reused for another file, disturbing that file's +position or contents. + +:py:meth:`~PIL.Image.Image.tobytes` also now explicitly cleans up its encoder on +both success and failure, rather than relying on object destruction. + PNG palettes are no longer padded when saving with the ``bits`` argument ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/src/PIL/Image.py b/src/PIL/Image.py index 873de8607bf..d793e2eaa35 100644 --- a/src/PIL/Image.py +++ b/src/PIL/Image.py @@ -883,21 +883,24 @@ def tobytes(self, encoder_name: str = "raw", *args: Any) -> bytes: # unpack data e = _getencoder(self.mode, encoder_name, encoder_args) - e.setimage(self.im, (0, 0, *self.size)) + try: + e.setimage(self.im, (0, 0, *self.size)) - from . import ImageFile + from . import ImageFile + + bufsize = max(ImageFile.MAXBLOCK, self.size[0] * 4) # see RawEncode.c - bufsize = max(ImageFile.MAXBLOCK, self.size[0] * 4) # see RawEncode.c - - output = [] - while True: - bytes_consumed, errcode, data = e.encode(bufsize) - output.append(data) - if errcode: - break - if errcode < 0: - msg = f"encoder error {errcode} in tobytes" - raise RuntimeError(msg) + output = [] + while True: + bytes_consumed, errcode, data = e.encode(bufsize) + output.append(data) + if errcode: + break + if errcode < 0: + msg = f"encoder error {errcode} in tobytes" + raise RuntimeError(msg) + finally: + e.cleanup() return b"".join(output) diff --git a/src/PIL/TiffImagePlugin.py b/src/PIL/TiffImagePlugin.py index d41dddcd2f4..a93c073c0ad 100644 --- a/src/PIL/TiffImagePlugin.py +++ b/src/PIL/TiffImagePlugin.py @@ -1998,16 +1998,19 @@ def _save(im: Image.Image, fp: IO[bytes], filename: str | bytes) -> None: tags.sort() a = (rawmode, compression, _fp, filename, tags, types) encoder = Image._getencoder(im.mode, "libtiff", a, encoderconfig) - encoder.setimage(im.im, (0, 0, *im.size)) - while True: - errcode, data = encoder.encode(ImageFile.MAXBLOCK)[1:] - if not _fp: - fp.write(data) - if errcode: - break - if errcode < 0: - msg = f"encoder error {errcode} when writing image file" - raise OSError(msg) + try: + encoder.setimage(im.im, (0, 0, *im.size)) + while True: + errcode, data = encoder.encode(ImageFile.MAXBLOCK)[1:] + if not _fp: + fp.write(data) + if errcode: + break + if errcode < 0: + msg = f"encoder error {errcode} when writing image file" + raise OSError(msg) + finally: + encoder.cleanup() else: for tag in blocklist: diff --git a/src/libImaging/TiffDecode.c b/src/libImaging/TiffDecode.c index 8a57c523dad..c206998da9d 100644 --- a/src/libImaging/TiffDecode.c +++ b/src/libImaging/TiffDecode.c @@ -954,6 +954,7 @@ ImagingLibTiffEncodeCleanup(ImagingCodecState state) { // that is fine, as it does not close the file TIFFClose(tiff); } + clientstate->tiff = NULL; return 0; }