From 792761711466e5e6bab7135f0c76b5da02b50804 Mon Sep 17 00:00:00 2001 From: Ramkumar Chinchani Date: Fri, 13 Oct 2023 20:13:56 +0000 Subject: [PATCH 1/8] fix(compress): allow passing in compressor options Goals of this PR: 1. Allow passing in options to individual compressor 2. Do not change default behavior Signed-off-by: Ramkumar Chinchani --- mutate/compress.go | 34 +++++++++++++++++++++++++++++++--- mutate/compress_test.go | 16 ++++++++++++++++ 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/mutate/compress.go b/mutate/compress.go index 740968c17..d8a2427ec 100644 --- a/mutate/compress.go +++ b/mutate/compress.go @@ -22,8 +22,15 @@ type Compressor interface { // indicate what compression type is used, e.g. "gzip", or "" for no // compression. MediaTypeSuffix() string + + // WithOpt applies an option and can be chained. + WithOpt(CompressorOpt) Compressor } +// CompressorOpt is a compressor option which can be used to configure a +// compressor. +type CompressorOpt interface{} + type noopCompressor struct{} func (nc noopCompressor) Compress(r io.Reader) (io.ReadCloser, error) { @@ -37,16 +44,24 @@ func (nc noopCompressor) MediaTypeSuffix() string { // NoopCompressor provides no compression. var NoopCompressor Compressor = noopCompressor{} +func (nc noopCompressor) WithOpt(CompressorOpt) Compressor { + return nc +} + // GzipCompressor provides gzip compression. -var GzipCompressor Compressor = gzipCompressor{} +var GzipCompressor Compressor = gzipCompressor{blockSize: 256 << 10} + +type GzipBlockSize int -type gzipCompressor struct{} +type gzipCompressor struct { + blockSize int +} func (gz gzipCompressor) Compress(reader io.Reader) (io.ReadCloser, error) { pipeReader, pipeWriter := io.Pipe() gzw := gzip.NewWriter(pipeWriter) - if err := gzw.SetConcurrency(256<<10, 2*runtime.NumCPU()); err != nil { + if err := gzw.SetConcurrency(gz.blockSize, 2*runtime.NumCPU()); err != nil { return nil, errors.Wrapf(err, "set concurrency level to %v blocks", 2*runtime.NumCPU()) } go func() { @@ -76,6 +91,15 @@ func (gz gzipCompressor) MediaTypeSuffix() string { return "gzip" } +func (gz gzipCompressor) WithOpt(opt CompressorOpt) Compressor { + switch val := opt.(type) { + case GzipBlockSize: + gz.blockSize = int(val) + } + + return gz +} + // ZstdCompressor provides zstd compression. var ZstdCompressor Compressor = zstdCompressor{} @@ -114,3 +138,7 @@ func (zs zstdCompressor) Compress(reader io.Reader) (io.ReadCloser, error) { func (zs zstdCompressor) MediaTypeSuffix() string { return "zstd" } + +func (zs zstdCompressor) WithOpt(CompressorOpt) Compressor { + return zs +} diff --git a/mutate/compress_test.go b/mutate/compress_test.go index 6cfd47ffe..3eda48dd9 100644 --- a/mutate/compress_test.go +++ b/mutate/compress_test.go @@ -46,6 +46,22 @@ func TestGzipCompressor(t *testing.T) { assert.NoError(err) assert.Equal(string(content), fact) + + // with options + buf = bytes.NewBufferString(fact) + c = GzipCompressor.WithOpt(GzipBlockSize(256 << 12)) + + r, err = c.Compress(buf) + assert.NoError(err) + assert.Equal(c.MediaTypeSuffix(), "gzip") + + r, err = gzip.NewReader(r) + assert.NoError(err) + + content, err = ioutil.ReadAll(r) + assert.NoError(err) + + assert.Equal(string(content), fact) } func TestZstdCompressor(t *testing.T) { From c7bb24d57e9853ab61f57e5d0bdfbb78b467c2a8 Mon Sep 17 00:00:00 2001 From: Ramkumar Chinchani Date: Wed, 25 Oct 2023 17:36:56 +0000 Subject: [PATCH 2/8] fix(compress): set default copy buffer to 1 MiB Signed-off-by: Ramkumar Chinchani --- pkg/system/copy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/system/copy.go b/pkg/system/copy.go index 4d4d4ace6..219331a78 100644 --- a/pkg/system/copy.go +++ b/pkg/system/copy.go @@ -29,7 +29,7 @@ import ( func Copy(dst io.Writer, src io.Reader) (int64, error) { // Make a buffer so io.Copy doesn't make one for each iteration. var buf []byte - size := 32 * 1024 + size := 256 << 12 // 1 MiB if lr, ok := src.(*io.LimitedReader); ok && lr.N < int64(size) { if lr.N < 1 { size = 1 From 3c661c1beb44765c86f04825996ce42d86002dc2 Mon Sep 17 00:00:00 2001 From: Serge Hallyn Date: Mon, 22 Jan 2024 13:23:20 -0600 Subject: [PATCH 3/8] avoid failing when lgetxattr has no data to give Signed-off-by: Serge Hallyn --- oci/layer/tar_generate.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/oci/layer/tar_generate.go b/oci/layer/tar_generate.go index 76b35ca37..63b6b6d38 100644 --- a/oci/layer/tar_generate.go +++ b/oci/layer/tar_generate.go @@ -219,10 +219,14 @@ func (tg *tarGenerator) AddFile(name, path string) error { // (we'd need to use libcap to parse it). value, err := tg.fsEval.Lgetxattr(path, name) if err != nil { - // XXX: I'm not sure if we're unprivileged whether Lgetxattr can - // fail with EPERM. If it can, we should ignore it (like when - // we try to clear xattrs). - return errors.Wrapf(err, "get xattr: %s", name) + v := errors.Cause(err) + log.Debugf("failure reading xattr from list on %q: %q", name, v) + if v != unix.EOPNOTSUPP && v != unix.ENODATA && v != unix.EPERM { + // XXX: I'm not sure if we're unprivileged whether Lgetxattr can + // fail with EPERM. If it can, we should ignore it (like when + // we try to clear xattrs). + return errors.Wrapf(err, "get xattr: %s", name) + } } // https://golang.org/issues/20698 -- We don't just error out here // because it's not _really_ a fatal error. Currently it's unclear From 1661f8458680dbafa0b7ef2193ecca8e4c15dd28 Mon Sep 17 00:00:00 2001 From: Michael McCracken Date: Wed, 21 Feb 2024 11:18:30 -0800 Subject: [PATCH 4/8] generateinsertlayer: close the tarwriter 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 (cherry picked from commit ad29ed3b33e341767ef4b594d2de21edf9713344) --- oci/layer/generate.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/oci/layer/generate.go b/oci/layer/generate.go index 1f1ab0c09..a1589d318 100644 --- a/oci/layer/generate.go +++ b/oci/layer/generate.go @@ -142,6 +142,12 @@ func GenerateInsertLayer(root string, target string, opaque bool, opt *RepackOpt tg := newTarGenerator(writer, packOptions.MapOptions) + defer func() { + if err := tg.tw.Close(); err != nil { + log.Warnf("generate insert layer: could not close tar.Writer: %s", err) + } + }() + if opaque { if err := tg.AddOpaqueWhiteout(target); err != nil { return err From 7a7284e44b936b7a7dbc13fb5e45d541b22fdf63 Mon Sep 17 00:00:00 2001 From: Michael McCracken Date: Wed, 21 Feb 2024 11:16:51 -0800 Subject: [PATCH 5/8] generate: do not warn about err on success 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 (cherry picked from commit 653952bef9291bb9a32491fa8d6580a7fb638464) --- oci/layer/generate.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/oci/layer/generate.go b/oci/layer/generate.go index a1589d318..a231d2ea4 100644 --- a/oci/layer/generate.go +++ b/oci/layer/generate.go @@ -54,7 +54,9 @@ func GenerateLayer(path string, deltas []mtree.InodeDelta, opt *RepackOptions) ( go func() (Err error) { // Close with the returned error. defer func() { - log.Warnf("could not generate layer: %v", Err) + if Err != nil { + log.Warnf("could not generate layer: %v", Err) + } // #nosec G104 _ = writer.CloseWithError(errors.Wrap(Err, "generate layer")) }() @@ -135,7 +137,9 @@ func GenerateInsertLayer(root string, target string, opaque bool, opt *RepackOpt go func() (Err error) { defer func() { - log.Warnf("could not generate insert layer: %v", Err) + if Err != nil { + log.Warnf("could not generate insert layer: %v", Err) + } // #nosec G104 _ = writer.CloseWithError(errors.Wrap(Err, "generate insert layer")) }() From 2366a725d79fb8c2bcaf387ea40dbe53cf49ade5 Mon Sep 17 00:00:00 2001 From: Michael McCracken Date: Wed, 21 Feb 2024 11:47:56 -0800 Subject: [PATCH 6/8] test: check blobs with gnutar, add sm file insert 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 (cherry picked from commit 7bb2940b394c979dde86a0fce6cbdca0cce1c57c) --- test/helpers.bash | 16 +++++++++++++++- test/insert.bats | 5 +++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/test/helpers.bash b/test/helpers.bash index fd981ce77..9fd73a6e1 100644 --- a/test/helpers.bash +++ b/test/helpers.bash @@ -99,7 +99,21 @@ function requires() { } function image-verify() { - oci-image-tool validate --type "imageLayout" "$@" + 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" || { + continue + } + zcat $ocidir/blobs/sha256/$f | tar tvf - >/dev/null || { + rc=$? + file $ocidir/blobs/sha256/$f + echo "error untarring $f: $rc" + return $rc + } + echo $f: valid tar archive + done + oci-image-tool validate --type "imageLayout" "$ocidir" return $? } diff --git a/test/insert.bats b/test/insert.bats index 1fa1b4ca1..38206524f 100644 --- a/test/insert.bats +++ b/test/insert.bats @@ -45,6 +45,7 @@ function teardown() { touch "${INSERTDIR}/test/a" touch "${INSERTDIR}/test/b" chmod +x "${INSERTDIR}/test/b" + echo "foo" > "${INSERTDIR}/test/smallfile" # Make sure rootless mode works. mkdir -p "${INSERTDIR}/some/path" @@ -68,6 +69,10 @@ function teardown() { [ "$status" -eq 0 ] image-verify "${IMAGE}" + umoci insert --image "${IMAGE}:${TAG}" "${INSERTDIR}/test/smallfile" /tester/smallfile + [ "$status" -eq 0 ] + image-verify "${IMAGE}" + # Unpack after the inserts. new_bundle_rootfs umoci unpack --image "${IMAGE}:${TAG}-new" "$BUNDLE" From 5b03842e595ff8cb1b2e386dc26d03ddb75b690c Mon Sep 17 00:00:00 2001 From: Ramkumar Chinchani Date: Fri, 22 Mar 2024 21:47:54 +0000 Subject: [PATCH 7/8] fix: handle overlay xattr opaque bit Current behavior determines if a path is a whiteout if a overlay char dev is present. Additionally, also check the extended attrs. Signed-off-by: Ramkumar Chinchani --- oci/layer/generate.go | 4 ++-- oci/layer/utils.go | 26 +++++++++++++++++++++++--- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/oci/layer/generate.go b/oci/layer/generate.go index a231d2ea4..ffd562473 100644 --- a/oci/layer/generate.go +++ b/oci/layer/generate.go @@ -88,7 +88,7 @@ func GenerateLayer(path string, deltas []mtree.InodeDelta, opt *RepackOptions) ( return errors.Wrapf(err, "couldn't determine overlay whiteout for %s", fullPath) } - whiteout, err := isOverlayWhiteout(fi) + whiteout, err := isOverlayWhiteout(fi, fullPath, tg.fsEval) if err != nil { return err } @@ -166,7 +166,7 @@ func GenerateInsertLayer(root string, target string, opaque bool, opt *RepackOpt } pathInTar := path.Join(target, curPath[len(root):]) - whiteout, err := isOverlayWhiteout(info) + whiteout, err := isOverlayWhiteout(info, curPath, tg.fsEval) if err != nil { return err } diff --git a/oci/layer/utils.go b/oci/layer/utils.go index f22122f78..dcbbcfed4 100644 --- a/oci/layer/utils.go +++ b/oci/layer/utils.go @@ -25,6 +25,7 @@ import ( "github.com/apex/log" rspec "github.com/opencontainers/runtime-spec/specs-go" + "github.com/opencontainers/umoci/pkg/fseval" "github.com/opencontainers/umoci/pkg/idtools" "github.com/pkg/errors" rootlesscontainers "github.com/rootless-containers/proto/go-proto" @@ -230,7 +231,7 @@ func InnerErrno(err error) error { // isOverlayWhiteout returns true if the FileInfo represents an overlayfs style // whiteout (i.e. mknod c 0 0) and false otherwise. -func isOverlayWhiteout(info os.FileInfo) (bool, error) { +func isOverlayWhiteout(info os.FileInfo, fullPath string, fsEval fseval.FsEval) (bool, error) { var major, minor uint32 switch stat := info.Sys().(type) { case *unix.Stat_t: @@ -243,6 +244,25 @@ func isOverlayWhiteout(info os.FileInfo) (bool, error) { return false, errors.Errorf("[internal error] unknown stat info type %T", info.Sys()) } - return major == 0 && minor == 0 && - info.Mode()&os.ModeCharDevice != 0, nil + if major == 0 && minor == 0 && + info.Mode()&os.ModeCharDevice != 0 { + return true, nil + } + + // also evaluate xattrs which may have opaque value set + attr, err := fsEval.Lgetxattr(fullPath, "user.overlay.opaque") + if err != nil { + v := errors.Cause(err) + if !errors.Is(err, os.ErrNotExist) && v != unix.EOPNOTSUPP && v != unix.ENODATA && v != unix.EPERM { + return false, errors.Errorf("[internal error] unknown stat info type %T", info.Sys()) + } + + return false, nil + } + + if string(attr) == "y" { + return true, nil + } + + return false, nil } From 4d17e6400f7ff9f93cada7ead1c99903f2906fc2 Mon Sep 17 00:00:00 2001 From: Ramkumar Chinchani Date: Fri, 29 Mar 2024 19:40:21 +0000 Subject: [PATCH 8/8] fix: handle unix.EACCES error also CI failures indicate that this could be another error code that needs to be handled. Signed-off-by: Ramkumar Chinchani --- oci/layer/utils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/oci/layer/utils.go b/oci/layer/utils.go index dcbbcfed4..8d8e1574f 100644 --- a/oci/layer/utils.go +++ b/oci/layer/utils.go @@ -253,7 +253,7 @@ func isOverlayWhiteout(info os.FileInfo, fullPath string, fsEval fseval.FsEval) attr, err := fsEval.Lgetxattr(fullPath, "user.overlay.opaque") if err != nil { v := errors.Cause(err) - if !errors.Is(err, os.ErrNotExist) && v != unix.EOPNOTSUPP && v != unix.ENODATA && v != unix.EPERM { + if !errors.Is(err, os.ErrNotExist) && v != unix.EOPNOTSUPP && v != unix.ENODATA && v != unix.EPERM && v != unix.EACCES { return false, errors.Errorf("[internal error] unknown stat info type %T", info.Sys()) }