Remove failed decompressed files #564
Conversation
When untarring a tarball, attempt to create files initially with the umask file permissions and delete the file if it fails to copy correctly. This prevents a paritally written file from remaining on disk with permissions that didn't include the umask. Adds a unit test and a invalid tarball (created by truncating a large file generated from /dev/urandom), which fails to decompress but errors partially written.
allisonlarson
force-pushed
the
b-chmod-failed-tar
branch
from
dbd8903 to
d1113d8
Compare
|
Looks like the Windows unit test in the CI are not happy with the We should consider adding a case |
allisonlarson
dismissed stale reviews from CreatorHead and dduzgun-security
via
c0ef527
|
@CreatorHead @dduzgun-security I was able to get the tests to pass on windows by explicitly closing the file (whoops!). I'd like to avoid splitting the tests since we expect the same behavior regardless of the underlying OS. Ready for another look! |
allisonlarson
changed the title
| _, err = io.Copy(dstF, src) | ||
| if err != nil { | ||
| // Close & remove the file in case of partial write | ||
| _ = dstF.Close() |
There was a problem hiding this comment.
We already call close in a deferred function on line 42, so do we need this? In any case, having a second call won't cause any errors as we are throwing away the returned error.
There was a problem hiding this comment.
The windows test case would fail here because the file was reported as being in use at the time of removal. Adding in the explicit file closed fixed it and allowed the file to be removed, so it seems like windows has a different requirement/handling that needs the file to be explicitly closed before removing it. I figured it's better to close the file an extra time as opposed to trying to check.
| count, err := Copy(ctx, dstF, srcF) | ||
| if err != nil { | ||
| // Close & remove the file in case of partial write | ||
| _ = dstF.Close() |
There was a problem hiding this comment.
We already call close in a deferred function on line 84, so do we need this? In any case, having a second call won't cause any errors as we are throwing away the returned error.
There was a problem hiding this comment.
Same reasoning as above, keeping the way these are handled in sync.
|
@CreatorHead Can you |
4 participants
When untarring a tarball, attempt to create files initially with the
umask file permissions and delete the file if it fails to copy
correctly. This prevents a partially written file from remaining on disk
with permissions that didn't include the umask.
Adds a unit test and a invalid tarball (created by truncating tarball containing a large
file generated from /dev/urandom), which fails to decompress but errors
partially written.
PCI review checklist
I have documented a clear reason for, and description of, the change I am making.
If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
If applicable, I've documented the impact of any changes to security controls.
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.