oci: tar extract: remove non-dir targets on unpack

CHANGELOG.md

@@ -20,6 +20,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
   aren't `flock(2)`ed, thus ensuring that any possible OCI image-spec
   extensions or other users of an image being operated on will no longer
   break.  openSUSE/umoci#198
+- `umoci unpack --rootless` will now correctly handle regular file unpacking
+  when overwriting a file that `umoci` doesn't have write access to. In
+  addition, the semantics of pre-existing hardlinks to a clobbered file are
+  clarified (the hard-links will not refer to the new layer's inode).
+  openSUSE/umoci#222 openSUSE/umoci#223
 
 ### Added
 - `umoci repack` now supports `--refresh-bundle` which will update the

oci/layer/tar_extract.go

@@ -275,14 +275,32 @@ func (te *tarExtractor) unpackEntry(root string, hdr *tar.Header, r io.Reader) (
 		return errors.Wrap(err, "mkdir parent")
 	}
 
+	// At this moment, we have sorted out all other special cases of the path.
+	// In order to avoid potential permission or clobbering issues, just do the
+	// noble thing and massacre whatever path points to (*unless* it's a
+	// directory). This is necessary to avoid permission errors with os.Create
+	// and similar issues. Note that this technically might break (the
+	// currently undefined) hardlink semantics for replaced inodes (a new layer
+	// will cause old hard-links to no longer be hard-links to the new inode).
+	//
+	// Ideally we would also handle the case where a TarLink entry exists
+	// before its target entry in the same layer (as this logic would
+	// technically break that), but it's not clear whether such an archive
+	// would be technically considered a valid tar archive.
+	if hdr.Typeflag != tar.TypeDir {
+		if err := te.fsEval.RemoveAll(path); err != nil {
+			return errors.Wrap(err, "clobber old")
+		}
+	}
+
 	// Now create or otherwise modify the state of the path. Right now, either
 	// the type of path matches hdr or the path doesn't exist. Note that we
 	// don't care about umasks or the initial mode here, since applyMetadata
 	// will fix all of that for us.
 	switch hdr.Typeflag {
 	// regular file
 	case tar.TypeReg, tar.TypeRegA:
-		// Truncate file, then just copy the data.
+		// Create a new file, then just copy the data.
 		fh, err := te.fsEval.Create(path)
 		if err != nil {
 			return errors.Wrap(err, "create regular")
@@ -333,11 +351,6 @@ func (te *tarExtractor) unpackEntry(root string, hdr *tar.Header, r io.Reader) (
 			linkFn = te.fsEval.Symlink
 		}
 
-		// Unlink the old path, and ignore it if the path didn't exist.
-		if err := te.fsEval.RemoveAll(path); err != nil {
-			return errors.Wrap(err, "remove link old")
-		}
-
 		// Link the new one.
 		if err := linkFn(linkname, path); err != nil {
 			// FIXME: Currently this can break if tar hardlink entries occur
@@ -376,11 +389,6 @@ func (te *tarExtractor) unpackEntry(root string, hdr *tar.Header, r io.Reader) (
 		mode := system.Tarmode(hdr.Typeflag)
 		dev := unix.Mkdev(uint32(hdr.Devmajor), uint32(hdr.Devminor))
 
-		// Unlink the old path, and ignore it if the path didn't exist.
-		if err := te.fsEval.RemoveAll(path); err != nil {
-			return errors.Wrap(err, "remove block old")
-		}
-
 		// Create the node.
 		if err := te.fsEval.Mknod(path, os.FileMode(int64(mode)|hdr.Mode), dev); err != nil {
 			return errors.Wrap(err, "mknod")

test/repack.bats

@@ -324,6 +324,7 @@ function teardown() {
 @test "umoci {un,re}pack [unpriv]" {
 	BUNDLE_A="$(setup_tmpdir)"
 	BUNDLE_B="$(setup_tmpdir)"
+	BUNDLE_C="$(setup_tmpdir)"
 
 	image-verify "${IMAGE}"
 
@@ -337,10 +338,14 @@ function teardown() {
 
 	# mkfifo and some other stuff
 	mkfifo "$BUNDLE_A/rootfs/some/directory/path/fifo"
-	echo "some contents" >> "$BUNDLE_A/rootfs/some/directory/path/file"
+	echo "some contents" > "$BUNDLE_A/rootfs/some/directory/path/file"
 	mkdir "$BUNDLE_A/rootfs/some/directory/path/dir"
 	ln -s "/../././././/../../../../etc/shadow" "$BUNDLE_A/rootfs/some/directory/path/link"
 
+	# Make sure that replacing a file we don't have write access to works.
+	echo "another file" > "$BUNDLE_A/rootfs/some/directory/NOWRITE"
+	chmod 0000 "$BUNDLE_A/rootfs/some/directory/NOWRITE"
+
 	# Chmod.
 	chmod 0000 "$BUNDLE_A/rootfs/some/directory/path"
 	chmod 0000 "$BUNDLE_A/rootfs/some/directory"
@@ -360,13 +365,44 @@ function teardown() {
 	chmod +rwx "$BUNDLE_B/rootfs/some"
 	chmod +rwx "$BUNDLE_B/rootfs/some/directory"
 	chmod +rwx "$BUNDLE_B/rootfs/some/directory/path"
+	chmod +rwx "$BUNDLE_B/rootfs/some/directory/NOWRITE"
 
 	# Make sure the types are right.
 	[[ "$(stat -c '%F' "$BUNDLE_B/rootfs/some/directory/path/fifo")" == "fifo" ]]
 	[[ "$(stat -c '%F' "$BUNDLE_B/rootfs/some/directory/path/file")" == "regular file" ]]
+	[ -f "$BUNDLE_B/rootfs/some/directory/NOWRITE" ]
 	[[ "$(stat -c '%F' "$BUNDLE_B/rootfs/some/directory/path/dir")" == "directory" ]]
 	[[ "$(stat -c '%F' "$BUNDLE_B/rootfs/some/directory/path/link")" == "symbolic link" ]]
 
+	# Try to overwite the NOWRITE file.
+	echo "different data" > "$BUNDLE_B/rootfs/some/directory/NOWRITE"
+	chmod 0000 "$BUNDLE_B/rootfs/some/directory/NOWRITE"
+
+	# Chmod.
+	chmod 0000 "$BUNDLE_B/rootfs/some/directory/path"
+	chmod 0000 "$BUNDLE_B/rootfs/some/directory"
+	chmod 0000 "$BUNDLE_B/rootfs/some"
+
+	# Repack the image again.
+	umoci repack --image "${IMAGE}" "$BUNDLE_B"
+	[ "$status" -eq 0 ]
+	image-verify "${IMAGE}"
+
+	# Unpack the image again.
+	umoci unpack --image "${IMAGE}" "$BUNDLE_C"
+	[ "$status" -eq 0 ]
+	bundle-verify "$BUNDLE_C"
+
+	# Undo the chmodding.
+	chmod +rwx "$BUNDLE_C/rootfs/some"
+	chmod +rwx "$BUNDLE_C/rootfs/some/directory"
+	chmod +rwx "$BUNDLE_C/rootfs/some/directory/path"
+	chmod +rwx "$BUNDLE_C/rootfs/some/directory/NOWRITE"
+
+	# Check NOWRITE.
+	[ -f "$BUNDLE_C/rootfs/some/directory/NOWRITE" ]
+	[[ "$(cat "$BUNDLE_C/rootfs/some/directory/NOWRITE")" == "different data" ]]
+
 	image-verify "${IMAGE}"
 }