generateinsertlayer: ensure that we close the tarwriter #534
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #534 +/- ##
==========================================
- Coverage 73.44% 73.34% -0.11%
==========================================
Files 60 60
Lines 4891 4895 +4
==========================================
- Hits 3592 3590 -2
- Misses 940 944 +4
- Partials 359 361 +2
|
|
Everything looks good to me, except you have,
in your commit message, which is a mystery hash? |
generatelayer closes the tarwriter, but generateinsertlayer forgets to. Closing the tarwriter writes the required footer of 1k of zeros. This results in tar files that are complete but invalid, and different reading tools will behave differently: - bsdtar doesn't complain and exits 0 - gnu tar (and security scanning tools that use it) will exit 2 with an unexpected EOF message - python's tarfile library will raise an Unexpected EOF error - golang's archive/tar library can raise an unexpected EOF error, but for some files created by generateinsertlayer, it just raises a plain EOF error, which means golang based tools generally ignore this and work fine, this includes umoci. Signed-off-by: Michael McCracken <[email protected]>
This warning is a little confusing when err is nil, and doesn't add any info in that case. Let's clean that up. Signed-off-by: Michael McCracken <[email protected]>
add a check to image-verify to ensure that all generated tar blobs are valid and do not cause gnu tar to exit nonzero add an insert test that adds a very small file to trigger unexpected EOF in the case where GenerateInsertLayer forgets to close the TarWriter. Signed-off-by: Michael McCracken <[email protected]>
tych0
force-pushed
the
2024.02.15/tardebug
branch
from
831be38 to
7bb2940
Compare
|
I just cleaned it up, will merge once that all passes CI |
| local ocidir="$@" | ||
| # test that each generated targz file is valid according to gnutar: | ||
| for f in $(ls $ocidir/blobs/sha256/); do | ||
| file $ocidir/blobs/sha256/$f | grep "gzip" || { |
There was a problem hiding this comment.
Nit: It would be nice if this check was more "strictly correct" (i.e. it actually detected if the file was actually a tar archive rather than a gzip stream) but given this is just for tests that's not too important.
There was a problem hiding this comment.
I agree... I actually thought maybe the right thing was to have the oci-image-tool do this validation, but that seems to be abandoned.
Is there a different tool for standards validation out there somewhere that we should move this to?
Given the recent changes with artifacts, etc, it is probably extra stale now.
There was a problem hiding this comment.
There were discussions about moving the image-validation infrastructure to umoci, but nothing has happened with regards to that topic since then. I can take a look at doing this after I do an umoci 0.5 release.
d'oh- thanks for taking care of this, that hash was a commit on top of the stacker fork of umoci, I should've cleaned up the message. Thanks for the quick review guys! |
This fixes #436.
I added a test that shows the issue - the commit with the fix has the full explanation, repeated here:
generatelayer closes the tarwriter, but generateinsertlayer forgets to.
Closing the tarwriter writes the required footer of 1k of zeros.
This results in tar files that are complete but invalid, and different
reading tools will behave differently:
unexpected EOF message
for some files created by generateinsertlayer, it just raises a
plain EOF error, which means golang based tools generally ignore this
and work fine, this includes umoci.