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

Close tmpfile after writing #685

Merged
merged 3 commits into from Jul 14, 2020
Merged

Close tmpfile after writing #685

merged 3 commits into from Jul 14, 2020

Conversation

lbonanomi
Copy link
Contributor

This PR closes #624

Windows will not allow for deletion of a file that has an open handle.

Close tmpfile after writing to prevent unencrypted tmpfiles out-living
the execution of sops.

Windows will not allow for deletion of a file with an open handle, 
close tmpfile after writing to prevent unencrypted tmpfiles out-living
the execution
@codecov-commenter
Copy link

codecov-commenter commented Jun 28, 2020

Codecov Report

Merging #685 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #685   +/-   ##
========================================
  Coverage    38.23%   38.23%           
========================================
  Files           23       23           
  Lines         3329     3329           
========================================
  Hits          1273     1273           
  Misses        1927     1927           
  Partials       129      129           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ae1968...836ad33. Read the comment docs.

@autrilla
Copy link
Contributor

autrilla commented Jul 6, 2020

Does it actually fix the issue completely? We should probably delete the temp file entirely.

@lbonanomi
Copy link
Contributor Author

Does it actually fix the issue completely?

I think so, but I didn't find the original problem and would like to get a confirmation from yourself (as a maintainer) and @xtrasolver (who opened the issue) before I say "done" with certainty.

We should probably delete the temp file entirely.

Yes indeed! Here's my 3-cent analysis: Linux lets you delete a file from disk while its open in an editor but Windows protects a file while it is open in an editor. Adding a close statement lets the downstream os.RemoveAll here to run to completion in git bash.

cmd/sops/edit.go Outdated Show resolved Hide resolved
@autrilla
Copy link
Contributor

autrilla commented Jul 6, 2020

Thanks for your explanation, I missed that we were actually deleting the entire directory.

However, you're currently closing the file before we're done using it. I'd add a defer before closing so the file is closed after the function has fully run. LGTM otherwise.

lbonanomi and others added 2 commits July 6, 2020 15:58
Co-authored-by: Adrian Utrilla <adrianutrilla@gmail.com>
@lbonanomi
Copy link
Contributor Author

Thanks for your explanation, I missed that we were actually deleting the entire directory.

However, you're currently closing the file before we're done using it. I'd add a defer before closing so the file is closed after the function has fully run. LGTM otherwise.

Per your instruction I added a defer. Just to be complete: this didn't influence my test results in git-bash. Thank you!

@lbonanomi lbonanomi requested a review from autrilla July 13, 2020 12:38
@autrilla
Copy link
Contributor

Thanks!

@autrilla autrilla merged commit 09d511f into getsops:develop Jul 14, 2020
clelange pushed a commit to clelange/sops that referenced this pull request Jul 19, 2020
* Close tmpfile after writing

Windows will not allow for deletion of a file with an open handle, 
close tmpfile after writing to prevent unencrypted tmpfiles out-living
the execution

* Update cmd/sops/edit.go

Co-authored-by: Adrian Utrilla <adrianutrilla@gmail.com>

* defer edited file close

Co-authored-by: Adrian Utrilla <adrianutrilla@gmail.com>
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.

None yet

3 participants