Trying to save a (0, 0) image creates an unreadable file

[NewUserHa]

What did you do?

from PIL import Image
Image.new("RGB",(0,0)).save('test.png') # `.png` or `.jpg` or `.tiff`

What did you expect to happen?

no file should be created on disk because there's an SystemError: tile cannot extend outside image error.
(#5931 (comment) for jpg, libjpeg seems not support size 0 0)

What actually happened?

throw SystemError: tile cannot extend outside image
a ~33 bytes file is on disk, which is unable to read via pillow and other viewer No matter the extension is .png or .jpg or .tiff

What are your OS, Python and Pillow versions?

• OS: win10 1803
• Python: 3.8.10
• Pillow: 9.0.0

[radarhere]

I've created PR #5947 to resolve this.

[NewUserHa]

rather than removing file, why not create file only if there's no error? (i.e. don't open file if there's any known error)

[radarhere]

Ok, i've updated #5947 to use that method, rather than my original suggestion of removing the saved image after an error.

[radarhere]

I've restored my original idea of removing the file after the error as a new PR, #6134. I'm skeptical about the idea of forcing files to live in memory simply to handle this edge case.

[NewUserHa]

What happens if there's user's file with that filename??

btw, shouldn't created = not os.path.exists(filename) from your PR be after the open probably?

[radarhere]

Yes, if the user is asking Pillow to overwrite an existing file with the image, a corrupted image will still be saved.
But I think that this is a rare situation, and forcing all users to keep their images in memory to handle this is an extreme solution.

I don't understand your comment about created = not os.path.exists(filename). It is before builtins.open because it is checking whether or not Pillow is creating the file. If Pillow is creating the file, then I feel ok about removing it. If the file was already on disk, then I think the user would not expect their file to be deleted after Pillow failed to save to it.

[NewUserHa]

what about: "before modifying on disk, check if it is saveable" OR
"use temp file then move to the place, like that the browsers download files does"?

ok. I thought it was to check if the file is created by the pillow

[radarhere]

It's unclear what "check if it is saveable" means without a significant change to how Pillow plugins work.

Using a temporary file is possible, but again, it seems like a sizeable change to how Pillow operates, and this issue seems like a rare situation. Feel free to argue otherwise if you think this is a common situation, and my PR doesn't address it.

[NewUserHa]

about "check if saveable before saving", because most formats don't support (0,0) image, what about if img.size == (0,0) and format != the supported ones just before saving at the lines in your PR ?

[radarhere]

There is room for users to create their own plugins and add them to Pillow. Your suggestion could conceivably break plugins written by users.

[NewUserHa]

ok. This issue then probably could be a low priority.

[radarhere]

#6134 has now been merged, meaning that the broken file will be removed after the error has been raised. That should be part of Pillow 9.1.0, due out on April 1.

If you don't think this is resolved, could you explain a bit more? I don't understand why it is important that a broken file not exist temporarily on disk.

[NewUserHa]

I think because:

• it has risks that potentially can remove users' files
• and it can throw errors before creating an actual file on disk since it is an unnecessary operation:
  • it may encounter new unknown disk errors while deleting the empty file
  • maybe because of single thread pillow and disk io, it increases latency too

so this issue probably could be a low priority, wait until probably found a better solution I guess

[radarhere]

It only removes the file if it didn't exist before Plllow started saving, so there should not be a risk of removing existing files.

[za3k]

A common method I've seen in cases like this, is saving to a temporary file, and moving that file over the requested save path once it's successful. This also prevents partial saves (if pillow crashes halfway, you still end up with a valid file).

My poor understanding is that there are different tradeoffs per operating system with this method. Ex. what happens if pillow crashes, does a temp file get left around? Who can access the temp file before it's deleted, and can they do anything different than with the requested save file path?

[radarhere]

@hugovk did you have any thoughts on this one?

Essentially, the request is for Pillow plugins to be able to raise an error before the file is created. Our current code doesn't allow for this because the plugin code is only called after the file is created.

Suggested solutions include

1. Keeping new files in memory until they are finished (Do not write new images to disk until they have successfully completed #5947)
2. Creating new files as temporary files (Trying to save a (0, 0) image creates an unreadable file #5944 (comment) and Trying to save a (0, 0) image creates an unreadable file #5944 (comment)), and move them when they are finished
3. "check if it is saveable" - adding another plugin method before opening the file and _save. _prepare_save, I guess, that would run everything in _save up until actual file access.

My only other idea is creating a wrapper around fp so that the file is only created when it is called. However, trying this out I found that it ties it to Python internals.

3 is the solution I like best, but it seems like a sizeable change for a minor improvement, and errors can still be raised after a file has started to be written. It does resolve this issue, but perhaps not the larger problem.

I'm inclined to close this as a wontfix.

[hugovk]

I'm also so inclined.

Does it even make sense to create a (0, 0) image in the first place, or can we validate and raise something like a ValueError?

Or on the save with a (0, 0)?

[radarhere]

#6159 has been merged, so a ValueError will now be raised when saving a JPEG with zero width or height.

In summary, #6134 was merged, so newly created files will be removed after an error. There is a request here that Pillow not even start to write a (0, 0) JPEG at the destination location, since that will never succeed. However, the current Pillow plugin API does not allow for this to be detected before starting to save, and sweeping changes to file saving to avoid this seem disruptive.