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

Do not close provided file handles with libtiff #7199

Merged
merged 2 commits into from Jun 30, 2023

Conversation

radarhere
Copy link
Member

Resolves #7042

libtiff closes file handles, but Pillow might still want access afterwards to seek to other frames in the file. To resolve this, TiffImagePlugin duplicates the file handle.

# libtiff closes the file descriptor, so pass in a dup.
try:
fp = hasattr(self.fp, "fileno") and os.dup(self.fp.fileno())

#5936 had a problem with too many open files, so the following code was added to ensure that if libtiff did not close the file, Pillow would still close the file.

if fp:
try:
os.close(fp)
except OSError:
pass

#7042 doesn't like this behaviour, feeling that it is not thread-safe to close a file handle more than once.

Investigating how exactly libtiff closes file handles, I found that it is _tiffCloseProc that closes the file handle. This is called by TIFFClose, which we call from

TIFFClose(tiff);

Looking at the code for TIFFClose, something becomes apparent - if we didn't want to call _tiffCloseProc when a file handle is in use, then we could just call TIFFCleanup instead of TIFFClose. Then we don't need to duplicate the file handle and don't need to try and close the duplicated file handle. Testing, this shouldn't break #5936, and passes #7042's reproduction (although that is not necessarily indicative).

@@ -720,7 +720,11 @@ ImagingLibTiffDecode(
}

decode_err:
TIFFClose(tiff);
if (clientstate->fp) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a comment here explaining why we're checking this would be good.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I've added some comments.

@hugovk hugovk merged commit 0ac3677 into python-pillow:main Jun 30, 2023
60 checks passed
@radarhere radarhere deleted the tiff_close branch June 30, 2023 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"close" syscall run twice on same file descriptor when loading binary LZW-compressed TIFF
3 participants