From 55e6f64977bf5be06d382bebb3727bbde2a8f01a Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Mon, 12 Oct 2020 17:15:42 -0700 Subject: [PATCH] internal/depsfile: Factor out our atomic file replacement logic This originated in the cliconfig code to write out credentials files. The Windows implementation of this in particular was quite onerous to get right because it needs a very specific sequence of operations to avoid running into exclusive file locks, and so by factoring this out with only cosmetic modification we can avoid repeating all of that engineering effort for other atomic file writing use-cases. --- command/cliconfig/config_unix.go | 5 -- command/cliconfig/config_windows.go | 25 -------- command/cliconfig/credentials.go | 5 +- internal/replacefile/doc.go | 12 ++++ internal/replacefile/replacefile_unix.go | 24 +++++++ internal/replacefile/replacefile_windows.go | 40 ++++++++++++ internal/replacefile/writefile.go | 71 +++++++++++++++++++++ 7 files changed, 150 insertions(+), 32 deletions(-) create mode 100644 internal/replacefile/doc.go create mode 100644 internal/replacefile/replacefile_unix.go create mode 100644 internal/replacefile/replacefile_windows.go create mode 100644 internal/replacefile/writefile.go diff --git a/command/cliconfig/config_unix.go b/command/cliconfig/config_unix.go index 36a5939da..5922c17ac 100644 --- a/command/cliconfig/config_unix.go +++ b/command/cliconfig/config_unix.go @@ -50,8 +50,3 @@ func homeDir() (string, error) { return user.HomeDir, nil } - -func replaceFileAtomic(source, destination string) error { - // On Unix systems, a rename is sufficiently atomic. - return os.Rename(source, destination) -} diff --git a/command/cliconfig/config_windows.go b/command/cliconfig/config_windows.go index ff3a10993..526b5d1de 100644 --- a/command/cliconfig/config_windows.go +++ b/command/cliconfig/config_windows.go @@ -3,12 +3,9 @@ package cliconfig import ( - "os" "path/filepath" "syscall" "unsafe" - - "golang.org/x/sys/windows" ) var ( @@ -47,25 +44,3 @@ func homeDir() (string, error) { return syscall.UTF16ToString(b), nil } - -func replaceFileAtomic(source, destination string) error { - // On Windows, renaming one file over another is not atomic and certain - // error conditions can result in having only the source file and nothing - // at the destination file. Instead, we need to call into the MoveFileEx - // Windows API function. - srcPtr, err := syscall.UTF16PtrFromString(source) - if err != nil { - return &os.LinkError{"replace", source, destination, err} - } - destPtr, err := syscall.UTF16PtrFromString(destination) - if err != nil { - return &os.LinkError{"replace", source, destination, err} - } - - flags := uint32(windows.MOVEFILE_REPLACE_EXISTING | windows.MOVEFILE_WRITE_THROUGH) - err = windows.MoveFileEx(srcPtr, destPtr, flags) - if err != nil { - return &os.LinkError{"replace", source, destination, err} - } - return nil -} diff --git a/command/cliconfig/credentials.go b/command/cliconfig/credentials.go index 9d394bf29..0b56191e2 100644 --- a/command/cliconfig/credentials.go +++ b/command/cliconfig/credentials.go @@ -12,9 +12,10 @@ import ( "github.com/zclconf/go-cty/cty" ctyjson "github.com/zclconf/go-cty/cty/json" - "github.com/hashicorp/terraform-svchost" + svchost "github.com/hashicorp/terraform-svchost" svcauth "github.com/hashicorp/terraform-svchost/auth" "github.com/hashicorp/terraform/configs/hcl2shim" + "github.com/hashicorp/terraform/internal/replacefile" pluginDiscovery "github.com/hashicorp/terraform/plugin/discovery" ) @@ -336,7 +337,7 @@ func (s *CredentialsSource) updateLocalHostCredentials(host svchost.Hostname, ne // Temporary file now replaces the original file, as atomically as // possible. (At the very least, we should not end up with a file // containing only a partial JSON object.) - err = replaceFileAtomic(tmpName, filename) + err = replacefile.AtomicRename(tmpName, filename) if err != nil { return fmt.Errorf("failed to replace %s with temporary file %s: %s", filename, tmpName, err) } diff --git a/internal/replacefile/doc.go b/internal/replacefile/doc.go new file mode 100644 index 000000000..8a1fbd993 --- /dev/null +++ b/internal/replacefile/doc.go @@ -0,0 +1,12 @@ +// Package replacefile is a small helper package focused directly at the +// problem of atomically "renaming" one file over another one. +// +// On Unix systems this is the standard behavior of the rename function, but +// the equivalent operation on Windows requires some specific operation flags +// which this package encapsulates. +// +// This package uses conditional compilation to select a different +// implementation for Windows vs. all other platforms. It may therefore +// require further fiddling in future if Terraform is ported to another +// OS that is neither Unix-like nor Windows. +package replacefile diff --git a/internal/replacefile/replacefile_unix.go b/internal/replacefile/replacefile_unix.go new file mode 100644 index 000000000..4bc92b83c --- /dev/null +++ b/internal/replacefile/replacefile_unix.go @@ -0,0 +1,24 @@ +// +build !windows + +package replacefile + +import ( + "os" +) + +// AtomicRename renames from the source path to the destination path, +// atomically replacing any file that might already exist at the destination. +// +// Typically this operation can succeed only if the source and destination +// are within the same physical filesystem, so this function is best reserved +// for cases where the source and destination exist in the same directory and +// only the local filename differs between them. +// +// The Unix implementation of AtomicRename relies on the atomicity of renaming +// that is required by the ISO C standard, which in turn assumes that Go's +// implementation of rename is calling into a system call that preserves that +// guarantee. +func AtomicRename(source, destination string) error { + // On Unix systems, a rename is sufficiently atomic. + return os.Rename(source, destination) +} diff --git a/internal/replacefile/replacefile_windows.go b/internal/replacefile/replacefile_windows.go new file mode 100644 index 000000000..690e57bc2 --- /dev/null +++ b/internal/replacefile/replacefile_windows.go @@ -0,0 +1,40 @@ +// +build windows + +package replacefile + +import ( + "os" + "syscall" + + "golang.org/x/sys/windows" +) + +// AtomicRename renames from the source path to the destination path, +// atomically replacing any file that might already exist at the destination. +// +// Typically this operation can succeed only if the source and destination +// are within the same physical filesystem, so this function is best reserved +// for cases where the source and destination exist in the same directory and +// only the local filename differs between them. +func AtomicRename(source, destination string) error { + // On Windows, renaming one file over another is not atomic and certain + // error conditions can result in having only the source file and nothing + // at the destination file. Instead, we need to call into the MoveFileEx + // Windows API function, setting two flags to opt in to replacing an + // existing file. + srcPtr, err := syscall.UTF16PtrFromString(source) + if err != nil { + return &os.LinkError{"replace", source, destination, err} + } + destPtr, err := syscall.UTF16PtrFromString(destination) + if err != nil { + return &os.LinkError{"replace", source, destination, err} + } + + flags := uint32(windows.MOVEFILE_REPLACE_EXISTING | windows.MOVEFILE_WRITE_THROUGH) + err = windows.MoveFileEx(srcPtr, destPtr, flags) + if err != nil { + return &os.LinkError{"replace", source, destination, err} + } + return nil +} diff --git a/internal/replacefile/writefile.go b/internal/replacefile/writefile.go new file mode 100644 index 000000000..23adde0fa --- /dev/null +++ b/internal/replacefile/writefile.go @@ -0,0 +1,71 @@ +package replacefile + +import ( + "fmt" + "io/ioutil" + "os" + "path/filepath" +) + +// AtomicWriteFile uses a temporary file along with this package's AtomicRename +// function in order to provide a replacement for ioutil.WriteFile that +// writes the given file into place as atomically as the underlying operating +// system can support. +// +// The sense of "atomic" meant by this function is that the file at the +// given filename will either contain the entirety of the previous contents +// or the entirety of the given data array if opened and read at any point +// during the execution of the function. +// +// On some platforms attempting to overwrite a file that has at least one +// open filehandle will produce an error. On other platforms, the overwriting +// will succeed but existing open handles will still refer to the old file, +// even though its directory entry is no longer present. +// +// Although AtomicWriteFile tries its best to avoid leaving behind its +// temporary file on error, some particularly messy error cases may result +// in a leftover temporary file. +func AtomicWriteFile(filename string, data []byte, perm os.FileMode) error { + dir, file := filepath.Split(filename) + f, err := ioutil.TempFile(dir, file) // alongside target file and with a similar name + if err != nil { + return fmt.Errorf("cannot create temporary file to update %s: %s", filename, err) + } + tmpName := f.Name() + moved := false + defer func(f *os.File, name string) { + // Remove the temporary file if it hasn't been moved yet. We're + // ignoring errors here because there's nothing we can do about + // them anyway. + if !moved { + os.Remove(name) + } + }(f, tmpName) + + // We'll try to apply the requested permissions. This may + // not be effective on all platforms, but should at least work on + // Unix-like targets and should be harmless elsewhere. + if err := os.Chmod(tmpName, perm); err != nil { + return fmt.Errorf("cannot set mode for temporary file %s: %s", tmpName, err) + } + + // Write the credentials to the temporary file, then immediately close + // it, whether or not the write succeeds. Note that closing the file here + // is required because on Windows we can't move a file while it's open. + _, err = f.Write(data) + f.Close() + if err != nil { + return fmt.Errorf("cannot write to temporary file %s: %s", tmpName, err) + } + + // Temporary file now replaces the original file, as atomically as + // possible. (At the very least, we should not end up with a file + // containing only a partial JSON object.) + err = AtomicRename(tmpName, filename) + if err != nil { + return fmt.Errorf("failed to replace %s with temporary file %s: %s", filename, tmpName, err) + } + + moved = true + return nil +}