feature request: atomic writes to (binary) files

[zhijie-yang]

When chisel is called by a bash instance and installs bash_bins into --root /, it will encounter an ETXTBSY error: error: cannot extract from package "bash": open /usr/bin/bash: text file busy. This is because Chisel tries to open the running /usr/bin/bash file, which triggers this error.

We want Chisel to be able to handle such cases, just like how dpkg is handling rewriting the /usr/bin/bash binary file by writing to a temporary file, and move this temporary file to overwrite the previous one, such that we can safely overwrite the running binary files.

References:

1. https://github.com/guillemj/dpkg/blob/0fa25abf11871c75de58e14743810609518e647e/src/main/unpack.c#L1471
2. Log from inotifywait on dpkg -i bash:

/proc/942119/root/usr/bin/ MOVED_FROM bash.dpkg-new
/proc/942119/root/usr/bin/ MOVED_TO bash

[letFunny]

Hey @zhijie-yang thanks for issue, this is definitely interesting. Can you give more details about the use-case? By reading the above I wonder about people that use the tool and mistakenly override files instead of getting an error, so I wonder if for your concrete use-case there is a better way to use it.

[zhijie-yang]

Sure! The use case is to include Chisel as a package management tool in a bare-based rock. When users of such a rock launch this rock with an entry command docker run ... exec bash or docker exec ... bash, and install a slice that depends on bash_bins (e.g., rabbitmq-server_bins), Chisel errors out upon write to /usr/bin/bash.

This should also benefit the data integrity of normal files. The current behaviour of Chisel opens the file and writes to the normal files, where race conditions can occur when this file is already opened by other processes, since they are accessing the file with the same inode.

[letFunny]

That is interesting indeed because I would have assumed that Chisel is best used working in two stages, staging and the final image which is conceptually much simpler than swapping things. I can image there are several edge cases which are tricky to get right when running two versions of bash at the same time, or any process for that manner. If one makes assumptions about being able to spawn identical process and it's tricky if we break that assumption.

With that said, what you are asking for is not that difficult to implement. Instead of writing we have to unlink the old version, write the new one and process which have already opened the old inode will still have access to it. If we were to prioritize it we would have to check for race conditions, portability, etc. but it seems doable.

What I would like to know as well is if there is a simpler way to reason about the problem that avoids us changing files that are being executed.

[zhijie-yang]

I'd argue that these edge cases can rarely happen. Otherwise, applications with such assumptions also won't work when dpkg reinstalls a package.

I've also noticed there are changes [1] in the Linux kernel's master branch that are trying to loosen such constraints, so I don't see that there are many caveats in Chisel to implement so.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0357ef03c94ef835bd44a0658b8edb672a9dbf51

[letFunny]

Update, I sent @zhijie-yang a patch offline to test with his concrete use-case and it does solve the issue. To avoid the complexity of creating temporary files we are unlinking + creating a file at the moment which is NOT atomic in the sense that rename(2) is. If we were to prioritize this request, then we would need to discuss both guarantees, but because the main users are containers/ROCKS, I would argue unlink is enough.

For context, the patch for commit ba4c08bcae905e20e4f4a6ab1dde3910244ef507 is:

diff --git a/internal/deb/extract.go b/internal/deb/extract.go
index 5f1f09d6..ed321f51 100644
--- a/internal/deb/extract.go
+++ b/internal/deb/extract.go
@@ -238,6 +238,7 @@ func extractData(pkgReader io.ReadSeeker, options *ExtractOptions) error {
 				Link:         link,
 				MakeParents:  true,
 				OverrideMode: true,
+				Unlink:       true,
 			}
 			err := options.Create(extractInfos, createOptions)
 			if err != nil && os.IsNotExist(err) && tarHeader.Typeflag == tar.TypeLink {
diff --git a/internal/fsutil/create.go b/internal/fsutil/create.go
index 0503c96f..71c4b6c4 100644
--- a/internal/fsutil/create.go
+++ b/internal/fsutil/create.go
@@ -10,6 +10,7 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
+	"syscall"
 )
 
 type CreateOptions struct {
@@ -27,6 +28,7 @@ type CreateOptions struct {
 	// If OverrideMode is true and entry already exists, update the mode. Does
 	// not affect symlinks.
 	OverrideMode bool
+	Unlink       bool
 }
 
 type Entry struct {
@@ -179,6 +181,12 @@ func createFile(o *CreateOptions) error {
 	if err != nil {
 		return err
 	}
+	if o.Unlink {
+		err = syscall.Unlink(path)
+		if err != nil && !os.IsNotExist(err) {
+			return err
+		}
+	}
 	file, err := os.OpenFile(path, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, o.Mode)
 	if err != nil {
 		return err

Please note that this is not the final patch just a PoC with no tests.

[polarathene]

We want Chisel to be able to handle such cases, just like how dpkg is handling rewriting the /usr/bin/bash binary file by writing to a temporary file, and move this temporary file to overwrite the previous one, such that we can safely overwrite the running binary files.

I don't think this would work for some environments when files are bind mounts (in Docker a container has implicit /etc/hostname, /etc/hosts, /etc/resolv.conf mounts, which may also be present in RUN layers during a build). It's possible to modify those files provided the inode doesn't change, but the atomic write mv swap with a temporary file as replacement would replace the inode, so that'd fail 😓

When I've needed to modify these files in a container instance with something like sed -i (which also uses the atomic write approach), I've needed to install the moreutils package for the sponge command which was a way to resolve the issue IIRC.

[upils]

I understand this issue is in fact a consequence of chisel not officially supporting "re-cutting" an existing rootfs. So trying to solve that outside of the work for this feature seems dangerous.

Moreover, the proposed solution here would break hardlinks because unlinking the existing file and recreating it would re-create an inode. So any hardlink previously extracted from the package would be broken, because they would now point at the "old" inode, with the "old" content. I suspect our approach to replace the existing content of the rootfs will be very similar to what dpkg does.

@polarathene In your case, since the files you mentioned are shared from the host, they should probably not be managed by chisel.

[letFunny]

I understand this issue is in fact a consequence of chisel not officially supporting "re-cutting" an existing rootfs. So trying to solve that outside of the work for this feature seems dangerous.

Moreover, the proposed solution here would break hardlinks because unlinking the existing file and recreating it would re-create an inode. So any hardlink previously extracted from the package would be broken, because they would now point at the "old" inode, with the "old" content. I suspect our approach to replace the existing content of the rootfs will be very similar to what dpkg does.

We discussed this offline the other day. Re-cutting would solve some of the issues IF the previous file system was created by Chisel. For re-cutting we will probably have to extract the new files to a temp folder and then rename which will solve the issue (rename will happily replace the file).

My claim is that even if we end up not writing to a temporary folder, we should still fix this issue with something like unlink. What we want is consistency and removing only some of the files because of an implementation detail is not ideal. Chisel will happily override all the files that exists in the target directory except if the files are currently being executed, regardless of whether Chisel created the filesystem or not. The reason is what I consider an implementation detail: Chisel uses open + write to override files instead of rename / mv or unlink.

Lastly, for completeness and for recording everything in the discussion,

Moreover, the proposed solution here would break hardlinks because unlinking the existing file and recreating it would re-create an inode. So any hardlink previously extracted from the package would be broken, because they would now point at the "old" inode, with the "old" content.

This is not completely true because the current implementation will create one file, it does not matter if open+write or unlink, and then create the rest of the hardlinks by using the link syscall. What is true is it will affect links created outside of Chisel, which will also happen with rename depending on how we use it. That is a discussion that we need to have and we need to decide on the UX separately.

We also agreed that rename is very similar to unlink + creating the new file. The only difference is the amount of syscalls and the guarantees of atomicity if the system crashes in the middle of the operation, which is not very relevant to us.

---

Summary: Ideally we will use rename to fix this issue as it will also be used for re-cutting. If not I think we should still fix it by using something like unlink.