Skip to content

fix: delete the temp file when its write fails - #1168

Open
nkuprins wants to merge 3 commits into
apache:mainfrom
nkuprins:fix/encrypt-temp-file-cleanup
Open

nkuprins wants to merge 3 commits into
apache:mainfrom
nkuprins:fix/encrypt-temp-file-cleanup

Conversation

@nkuprins

Copy link
Copy Markdown
Contributor

Closed: #1137

Purpose of the pull request

As title
Out of scope: when encryption fails, finish() still writes the unencrypted workbook to the caller's stream. That is tracked separately in #1135.

What's changed?

  • WriteContextImpl#doOutputStreamEncrypt07 deletes the temp file in one outer finally.
  • The temp file's FileOutputStream is a try-with-resources, so it is closed even when workbook.close() throws.
  • The delete is skipped when the temp file doesn't exist. Without this, a failure to open it would report "Can not delete temp File!" instead of the FileNotFoundException.

When two steps fail, only one exception can be thrown. These are the cases where doOutputStreamEncrypt07 now throws a different one than on main:

What fails Thrown on main Thrown in this PR
Both writing the temp file and closing its stream the close error; the write error is lost the write error, with the close error attached as suppressed
Both writing the temp file and deleting it the write error, but the delete was never attempted, so the unencrypted temp file stayed on disk "Can not delete temp File!"; the write error is lost
Both closing the workbook and closing the stream the workbook close error the same, with the stream close error attached as suppressed

Tests

New cases in EncryptDataTest:

  • xlsxStreamPasswordWrite_tempFileWriteFails_deletesTempFile: fails on main.
  • xlsxStreamPasswordWrite_workbookCloseFails_closesTempFileStream: fails on main.
  • xlsxStreamPasswordWrite_workbookCloseFails_deletesTempFile
  • xlsxStreamPasswordWrite_encryptedWriteFails_deletesTempFile
  • xlsxStreamPasswordWrite_tempFileOpenFails_reportsOpenError

The last three pass on main too, since those paths already cleaned up. They guard against regressions.

Checklist

  • I have read the Contributor Guide.
  • I have written the necessary doc or comment.
  • I have added the necessary unit tests and all cases have passed.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] xlsx stream write leaves temp file

1 participant