Use an xz library instead of shelling out to xz for decompression

When decompressing layers compressed with xz, use a library rather than
shelling out to the xz CLI.

Signed-off-by: Nalin Dahyabhai <[email protected]>

Author: nalind

contrib/cirrus/setup.sh

@@ -16,10 +16,13 @@ case "$OS_RELEASE_ID" in
         bash "$SCRIPT_BASE/add_second_partition.sh"
         [[ -z "$RPMS_CONFLICTING" ]] || \
             $SHORT_DNFY erase $RPMS_CONFLICTING
+        $SHORT_DNFY install zstd
         ;;
     ubuntu)
         [[ -z "$DEBS_CONFLICTING" ]] || \
             $SHORT_APTGET -q remove $DEBS_CONFLICTING
+        $SHORT_APTGET -q update
+        $SHORT_APTGET -q install zstd
         ;;
     *)
         bad_os_id_ver

go.mod

@@ -23,6 +23,7 @@ require (
 	github.com/stretchr/testify v1.7.0
 	github.com/syndtr/gocapability v0.0.0-20200815063812-42c35b437635
 	github.com/tchap/go-patricia v2.3.0+incompatible
+	github.com/ulikunitz/xz v0.5.10
 	github.com/vbatts/tar-split v0.11.1
 	golang.org/x/net v0.0.0-20201224014010-6772e930b67b
 	golang.org/x/sys v0.0.0-20210124154548-22da62e12c0c

go.sum

@@ -476,6 +476,8 @@ github.com/tchap/go-patricia v2.3.0+incompatible h1:GkY4dP3cEfEASBPPkWd+AmjYxhmD
 github.com/tchap/go-patricia v2.3.0+incompatible/go.mod h1:bmLyhP68RS6kStMGxByiQ23RP/odRBOTVjwp2cDyi6I=
 github.com/tmc/grpc-websocket-proxy v0.0.0-20170815181823-89b8d40f7ca8/go.mod h1:ncp9v5uamzpCO7NfCPTXjqaC+bZgJeR0sMTm6dMHP7U=
 github.com/tmc/grpc-websocket-proxy v0.0.0-20190109142713-0ad062ec5ee5/go.mod h1:ncp9v5uamzpCO7NfCPTXjqaC+bZgJeR0sMTm6dMHP7U=
+github.com/ulikunitz/xz v0.5.10 h1:t92gobL9l3HE202wg3rlk19F6X+JOxl9BBrCCMYEYd8=
+github.com/ulikunitz/xz v0.5.10/go.mod h1:nbz6k7qbPmH4IRqmfOplQw/tblSgqTqBwxkY0oWt/14=
 github.com/urfave/cli v0.0.0-20171014202726-7bc6a0acffa5/go.mod h1:70zkFmudgCuE/ngEzBv17Jvp/497gISqfk5gWijbERA=
 github.com/urfave/cli v1.20.0/go.mod h1:70zkFmudgCuE/ngEzBv17Jvp/497gISqfk5gWijbERA=
 github.com/urfave/cli v1.22.1/go.mod h1:Gos4lmkARVdJ6EkW0WaNv/tZAAMe9V7XWyB60NtXRu0=

pkg/archive/archive.go

@@ -9,7 +9,6 @@ import (
 	"io"
 	"io/ioutil"
 	"os"
-	"os/exec"
 	"path/filepath"
 	"runtime"
 	"strings"
@@ -18,14 +17,14 @@ import (
 
 	"github.com/containers/storage/pkg/fileutils"
 	"github.com/containers/storage/pkg/idtools"
-	"github.com/containers/storage/pkg/ioutils"
 	"github.com/containers/storage/pkg/pools"
 	"github.com/containers/storage/pkg/promise"
 	"github.com/containers/storage/pkg/system"
 	gzip "github.com/klauspost/pgzip"
 	rsystem "github.com/opencontainers/runc/libcontainer/system"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
+	"github.com/ulikunitz/xz"
 )
 
 type (
@@ -173,12 +172,6 @@ func DetectCompression(source []byte) Compression {
 	return Uncompressed
 }
 
-func xzDecompress(archive io.Reader) (io.ReadCloser, <-chan struct{}, error) {
-	args := []string{"xz", "-d", "-c", "-q"}
-
-	return cmdStream(exec.Command(args[0], args[1:]...), archive)
-}
-
 // DecompressStream decompresses the archive and returns a ReaderCloser with the decompressed archive.
 func DecompressStream(archive io.Reader) (io.ReadCloser, error) {
 	p := pools.BufioReader32KPool
@@ -211,15 +204,12 @@ func DecompressStream(archive io.Reader) (io.ReadCloser, error) {
 		readBufWrapper := p.NewReadCloserWrapper(buf, bz2Reader)
 		return readBufWrapper, nil
 	case Xz:
-		xzReader, chdone, err := xzDecompress(buf)
+		xzReader, err := xz.NewReader(buf)
 		if err != nil {
 			return nil, err
 		}
 		readBufWrapper := p.NewReadCloserWrapper(buf, xzReader)
-		return ioutils.NewReadCloserWrapper(readBufWrapper, func() error {
-			<-chdone
-			return readBufWrapper.Close()
-		}), nil
+		return readBufWrapper, nil
 	case Zstd:
 		return zstdReader(buf)
 	default:
@@ -1319,35 +1309,6 @@ func remapIDs(readIDMappings, writeIDMappings *idtools.IDMappings, chownOpts *id
 	return nil
 }
 
-// cmdStream executes a command, and returns its stdout as a stream.
-// If the command fails to run or doesn't complete successfully, an error
-// will be returned, including anything written on stderr.
-func cmdStream(cmd *exec.Cmd, input io.Reader) (io.ReadCloser, <-chan struct{}, error) {
-	chdone := make(chan struct{})
-	cmd.Stdin = input
-	pipeR, pipeW := io.Pipe()
-	cmd.Stdout = pipeW
-	var errBuf bytes.Buffer
-	cmd.Stderr = &errBuf
-
-	// Run the command and return the pipe
-	if err := cmd.Start(); err != nil {
-		return nil, nil, err
-	}
-
-	// Copy stdout to the returned pipe
-	go func() {
-		if err := cmd.Wait(); err != nil {
-			pipeW.CloseWithError(fmt.Errorf("%s: %s", err, errBuf.String()))
-		} else {
-			pipeW.Close()
-		}
-		close(chdone)
-	}()
-
-	return pipeR, chdone, nil
-}
-
 // NewTempArchive reads the content of src into a temporary file, and returns the contents
 // of that file as an archive. The archive can only be read once - as soon as reading completes,
 // the file will be deleted.

pkg/archive/archive_test.go

@@ -12,7 +12,6 @@ import (
 	"runtime"
 	"strings"
 	"testing"
-	"time"
 
 	"github.com/containers/storage/pkg/idtools"
 	"github.com/stretchr/testify/assert"
@@ -209,59 +208,6 @@ func TestExtensionXz(t *testing.T) {
 	}
 }
 
-func TestCmdStreamLargeStderr(t *testing.T) {
-	cmd := exec.Command("sh", "-c", "dd if=/dev/zero bs=1k count=1000 of=/dev/stderr; echo hello")
-	out, _, err := cmdStream(cmd, nil)
-	if err != nil {
-		t.Fatalf("Failed to start command: %s", err)
-	}
-	errCh := make(chan error)
-	go func() {
-		_, err := io.Copy(ioutil.Discard, out)
-		errCh <- err
-	}()
-	select {
-	case err := <-errCh:
-		if err != nil {
-			t.Fatalf("Command should not have failed (err=%.100s...)", err)
-		}
-	case <-time.After(5 * time.Second):
-		t.Fatalf("Command did not complete in 5 seconds; probable deadlock")
-	}
-}
-
-func TestCmdStreamBad(t *testing.T) {
-	// TODO Windows: Figure out why this is failing in CI but not locally
-	if runtime.GOOS == windows {
-		t.Skip("Failing on Windows CI machines")
-	}
-	badCmd := exec.Command("sh", "-c", "echo hello; echo >&2 error couldn\\'t reverse the phase pulser; exit 1")
-	out, _, err := cmdStream(badCmd, nil)
-	if err != nil {
-		t.Fatalf("Failed to start command: %s", err)
-	}
-	if output, err := ioutil.ReadAll(out); err == nil {
-		t.Fatalf("Command should have failed")
-	} else if err.Error() != "exit status 1: error couldn't reverse the phase pulser\n" {
-		t.Fatalf("Wrong error value (%s)", err)
-	} else if s := string(output); s != "hello\n" {
-		t.Fatalf("Command output should be '%s', not '%s'", "hello\\n", output)
-	}
-}
-
-func TestCmdStreamGood(t *testing.T) {
-	cmd := exec.Command("sh", "-c", "echo hello; exit 0")
-	out, _, err := cmdStream(cmd, nil)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if output, err := ioutil.ReadAll(out); err != nil {
-		t.Fatalf("Command should not have failed (err=%s)", err)
-	} else if s := string(output); s != "hello\n" {
-		t.Fatalf("Command output should be '%s', not '%s'", "hello\\n", output)
-	}
-}
-
 func TestUntarPathWithInvalidDest(t *testing.T) {
 	tempFolder, err := ioutil.TempDir("", "storage-archive-test")
 	require.NoError(t, err)

tests/apply-junk.bats

@@ -0,0 +1,25 @@
+#!/usr/bin/env bats
+
+load helpers
+
+@test "applyjunk" {
+	# Create and try to populate layers with... garbage.  It should be
+	# rejected cleanly.
+	for compressed in cat gzip bzip2 xz zstd ; do
+		storage create-layer --id layer-${compressed}
+
+		echo [[${compressed} /etc/os-release]]
+		${compressed} < /etc/os-release > junkfile
+		run storage apply-diff --file junkfile layer-${compressed}
+		echo "$output"
+		[[ "$status" -ne 0 ]]
+		[[ "$output" =~ "invalid tar header" ]] || [[ "$output" =~ "unexpected EOF" ]]
+
+		echo [[${compressed}]]
+		echo "sorry, not even enough info for a tar header here" | ${compressed} > junkfile
+		run storage apply-diff --file junkfile layer-${compressed}
+		echo "$output"
+		[[ "$status" -ne 0 ]]
+		[[ "$output" =~ "unexpected EOF" ]]
+	done
+}