layerStore.Diff(): don't use an unnecessary buffer

Don't ReadAll() from a Reader to create a buffer and then create another
Reader to read from that buffer.

Don't close a file and a decompressor that we're using to read the
file's contents when we we may still need to read from them after the
current function returns.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

layers.go

@@ -23,6 +23,7 @@ import (
 	"github.com/containers/storage/pkg/system"
 	"github.com/containers/storage/pkg/tarlog"
 	"github.com/containers/storage/pkg/truncindex"
+	multierror "github.com/hashicorp/go-multierror"
 	"github.com/klauspost/pgzip"
 	digest "github.com/opencontainers/go-digest"
 	"github.com/opencontainers/selinux/go-selinux/label"
@@ -1463,34 +1464,48 @@ func (r *layerStore) Diff(from, to string, options *DiffOptions) (io.ReadCloser,
 		}
 		return maybeCompressReadCloser(diff)
 	}
-	defer tsfile.Close()
 
 	decompressor, err := pgzip.NewReader(tsfile)
 	if err != nil {
-		return nil, err
-	}
-	defer decompressor.Close()
-
-	tsbytes, err := ioutil.ReadAll(decompressor)
-	if err != nil {
+		if e := tsfile.Close(); e != nil {
+			logrus.Debug(e)
+		}
 		return nil, err
 	}
 
-	metadata = storage.NewJSONUnpacker(bytes.NewBuffer(tsbytes))
+	metadata = storage.NewJSONUnpacker(decompressor)
 
 	fgetter, err := r.newFileGetter(to)
 	if err != nil {
-		return nil, err
+		errs := multierror.Append(nil, errors.Wrapf(err, "creating file-getter"))
+		if err := decompressor.Close(); err != nil {
+			errs = multierror.Append(errs, errors.Wrapf(err, "closing decompressor"))
+		}
+		if err := tsfile.Close(); err != nil {
+			errs = multierror.Append(errs, errors.Wrapf(err, "closing tarstream headers"))
+		}
+		return nil, errs.ErrorOrNil()
 	}
 
 	tarstream := asm.NewOutputTarStream(fgetter, metadata)
 	rc := ioutils.NewReadCloserWrapper(tarstream, func() error {
-		err1 := tarstream.Close()
-		err2 := fgetter.Close()
-		if err2 == nil {
-			return err1
+		var errs *multierror.Error
+		if err := decompressor.Close(); err != nil {
+			errs = multierror.Append(errs, errors.Wrapf(err, "closing decompressor"))
+		}
+		if err := tsfile.Close(); err != nil {
+			errs = multierror.Append(errs, errors.Wrapf(err, "closing tarstream headers"))
+		}
+		if err := tarstream.Close(); err != nil {
+			errs = multierror.Append(errs, errors.Wrapf(err, "closing reconstructed tarstream"))
+		}
+		if err := fgetter.Close(); err != nil {
+			errs = multierror.Append(errs, errors.Wrapf(err, "closing file-getter"))
+		}
+		if errs != nil {
+			return errs.ErrorOrNil()
 		}
-		return err2
+		return nil
 	})
 	return maybeCompressReadCloser(rc)
 }

tests/apply-junk.bats

@@ -2,24 +2,46 @@
 
 load helpers
 
-@test "applyjunk" {
+function applyjunk_main() {
 	# 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
+	compressed="$1"
+
+	storage create-layer --id layer-${compressed}
+
+	echo [[${compressed} /etc/os-release]]
+	if ! ${compressed} < /etc/os-release > junkfile ; then
+		skip "error running ${compressed}"
+	fi
+	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" ]]
+}
+
+@test "applyjunk-uncompressed" {
+	applyjunk_main cat
+}
+
+@test "applyjunk-gzip" {
+	applyjunk_main gzip
+}
+
+@test "applyjunk-bzip2" {
+	applyjunk_main bzip2
+}
+
+@test "applyjunk-xz" {
+	applyjunk_main xz
+}
+
+@test "applyjunk-zstd" {
+	applyjunk_main zstd
 }