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

ImageMagick leaves temp files on disk after a crash #139

Closed
moshekaplan opened this issue Feb 24, 2016 · 5 comments
Closed

ImageMagick leaves temp files on disk after a crash #139

moshekaplan opened this issue Feb 24, 2016 · 5 comments

Comments

@moshekaplan
Copy link

If ImageMagick crashes while processing a file, the temporary files created by ImageMagick remain on the disk after ImageMagick crashes.

For long-running machines, it would be beneficial if ImageMagick did not leave behind any temporary files. It may be possible to use tmpfile(3) or mkstemp(3) to prevent the temporary files from remaining after ImageMagick crashes.

@moshekaplan moshekaplan changed the title ImageMagick crashes leave temp files on disk ImageMagick leaves temp files on disk after a crash Feb 24, 2016
@mikayla-grace
Copy link

ImageMagick does utilize mkstemp(). See magick/resource.c/AcquireUniqueFileResource()/472. It also removes any temporary files when the program exits normally or due to a signal it can catch. If its a signal that is not catchable, such as KILL, ImageMagick exits and any temporary files may remain. If you provide a method that we can reproduce such that ImageMagick exits normally or as a result of a catchable signal and temporary files remain, that would be a bug and we'll investigate further.

@moshekaplan
Copy link
Author

It appears that I initially made the same mistake that the original developer did. mkstemp() only creates the unique temporary file. It does not ensure that the file is removed after the program exits. This mistake is easy to make, because the similar tmpfile() automatically deletes the temporary file when the program exits.

The solution is to add a call to unlink() immediately after the successful call to mkstemp(). Adding the call to unlink() ensures that the temporary file is deleted after ImageMagick closes, no matter what happens. This solution is described in detail here: http://www.thegeekstuff.com/2012/06/c-temporary-files/ .

I just submitted a pull request that should make the necessary change.

@mikayla-grace
Copy link

We were aware of using unlink() after mkstemp() for ensuring a temporary file does not persist after ImageMagick exits. However, a number of algorithms within ImageMagick assume that the temporary file persists on disk in order to function properly. In addition, the patch changes the behavior of the algorithm, that is, on Posix systems the file is removed immediately but the descriptor stays in memory. However, under Windows you cannot unlink a file if the file descriptor has not been released. Consistent behavior across different platforms is a requirement of ImageMagick.

@moshekaplan
Copy link
Author

However, a number of algorithms within ImageMagick assume that the temporary file persists on disk in order to function properly.

unlink() only causes the file to be deleted after all handles have been closed. As long as ImageMagick still has the open handle, ImageMagick can read and write the file's contents. (Source: http://linux.die.net/man/2/unlink ) . Other programs would not be able to open the file after it's unlink()ed.

In addition, the patch changes the behavior of the algorithm, that is, on Posix systems the file is removed immediately but the descriptor stays in memory. However, under Windows you cannot unlink a file if the file descriptor has not been released. Consistent behavior across different platforms is a requirement of ImageMagick.

On Windows, the file can be opened with the flags set to _O_CREAT | _O_TEMPORARY. _O_CREAT | _O_TEMPORARY causes the file to be deleted when all handles are closed. I don't know if the temporary file is readable by other programs. Source: https://msdn.microsoft.com/en-us/library/z0kc8e3z.aspx

@mikayla-grace
Copy link

This does not resolve the use case of where some ImageMagick processes require the temporary file to remain after it has been closed such as the delegate subsystem. Files associated with the delegate subsystem (see delegates.xml) are not accessed directly for security reasons. We must first create a temporary file on disk and then permit the delegate to read / write it through a symbolic link, rather than directly. Temporary pixel caches are another example. When processing multiple images, sometimes we need to close the file handle of one or more images to prevent exceeding the system open filehandle limit. At a later time we need to reopen the temporary file to complete processing. If we unlink a temporary file immediately after we create it, at least in these two cases, ImageMagick would fail.

Many years ago we had the unlink() in ImageMagick as you recommended and it caused problems for a number of ImageMagick workflows and that is why we settled on the current way temporary files are handled. It works well except in the case of an uncatchable signal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants