scryptenc: free the expanded AES key when a write fails - #430
Open
woahwhattheheck wants to merge 2 commits into
Open
scryptenc: free the expanded AES key when a write fails#430woahwhattheheck wants to merge 2 commits into
woahwhattheheck wants to merge 2 commits into
Conversation
scryptenc_file() and scryptdec_file_copy() both free the crypto_aesctr stream when a write to the output file fails partway through the data, but not the struct crypto_aes_key it was built from. err1 in scryptenc_file() only zeroes dk, and err0 in scryptdec_file_copy() only returns, so key_enc_exp never reaches crypto_aes_key_free(). The crypto_aesctr_init() failure path a few lines above each of these already frees it; make the write-error path match. scryptenc_buf() and scryptdec_buf() have no fwrite() and are unaffected. Add tests/13-write-error.sh, which drives both loops into a failing write via /dev/full -- these paths had no coverage, which is why "make test USE_VALGRIND=1" did not notice. The test uses an input larger than stdio's buffer so the failure lands inside the loop rather than at the final flush, and skips the /dev/full part on platforms which do not have it.
The previous commit was uploaded through the GitHub API by a helper which dropped the final newline of every text file it sent. Rewrite those files with their trailing newline intact; no other change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #429.
scryptenc_file()andscryptdec_file_copy()free thecrypto_aesctrstreamwhen a write to the output file fails partway through, but not the
struct crypto_aes_key *it was built from:err1inscryptenc_file()only zeroesdk, anderr0inscryptdec_file_copy()only returns, sokey_enc_expnever reachescrypto_aes_key_free(). Thecrypto_aesctr_init()failure path a few linesabove each of these already frees it; this just makes the write-error path
match. The
_bufvariants have nofwrite()and are unaffected.The change
Two lines, one in each function. After it, every exit from the scope of a
crypto_aes_key_expand()in this file frees the result.Test
tests/13-write-error.shis new. The write-error path in these two loops hasno coverage today, which is why
make test USE_VALGRIND=1does not catch this.The test builds an input larger than stdio's buffer — so the failure lands
inside the loop rather than at the final flush — and points the output at
/dev/full, checking exit status 1 and theError writing filemessage forboth
encanddec. It also confirms that encrypting the same large input toa normal file still succeeds.
/dev/fullis not portable, so the test does that part only if[ -c /dev/full ]and[ -w /dev/full ]; elsewhere it runs the plainlarge-file encryption check and returns. Explicit parameters plus
-fkeepthe key derivation cheap so the test does not measure CPU speed on every run.
Note on the test number
Numbered
13so it does not collide withtests/11-info.shfrom #426 ortests/12-same-file.shfrom #428. The ordering works whichever of the threelands first; the only overlap between them is the adjacent line each adds to
EXTRA_DISTinMakefile.am. Happy to renumber.