cli: ensure that 'snapshot save' is fsync safe and also only writes to the requested file on success (#7698)

command/snapshot/save/snapshot_save.go

@@ -3,13 +3,13 @@ package save
 import (
 	"flag"
 	"fmt"
-	"io"
 	"os"
 
 	"github.com/hashicorp/consul/api"
 	"github.com/hashicorp/consul/command/flags"
 	"github.com/hashicorp/consul/snapshot"
 	"github.com/mitchellh/cli"
+	"github.com/rboyer/safeio"
 )
 
 func New(ui cli.Ui) *cmd {
@@ -69,24 +69,16 @@ func (c *cmd) Run(args []string) int {
 	}
 	defer snap.Close()
 
-	// Save the file.
-	f, err := os.Create(file)
-	if err != nil {
-		c.UI.Error(fmt.Sprintf("Error creating snapshot file: %s", err))
-		return 1
-	}
-	if _, err := io.Copy(f, snap); err != nil {
-		f.Close()
-		c.UI.Error(fmt.Sprintf("Error writing snapshot file: %s", err))
-		return 1
-	}
-	if err := f.Close(); err != nil {
-		c.UI.Error(fmt.Sprintf("Error closing snapshot file after writing: %s", err))
+	// Save the file first.
+	unverifiedFile := file + ".unverified"
+	if _, err := safeio.WriteToFile(snap, unverifiedFile, 0666); err != nil {
+		c.UI.Error(fmt.Sprintf("Error writing unverified snapshot file: %s", err))
 		return 1
 	}
+	defer os.Remove(unverifiedFile)
 
 	// Read it back to verify.
-	f, err = os.Open(file)
+	f, err := os.Open(unverifiedFile)
 	if err != nil {
 		c.UI.Error(fmt.Sprintf("Error opening snapshot file for verify: %s", err))
 		return 1
@@ -101,6 +93,11 @@ func (c *cmd) Run(args []string) int {
 		return 1
 	}
 
+	if err := safeio.Rename(unverifiedFile, file); err != nil {
+		c.UI.Error(fmt.Sprintf("Error renaming %q to %q: %v", unverifiedFile, file, err))
+		return 1
+	}
+
 	c.UI.Info(fmt.Sprintf("Saved and verified snapshot to index %d", qm.LastIndex))
 	return 0
 }

command/snapshot/save/snapshot_save_test.go

@@ -176,6 +176,17 @@ func TestSnapshotSaveCommand_TruncatedStream(t *testing.T) {
 			output := ui.ErrorWriter.String()
 			require.Contains(t, output, "Error verifying snapshot file")
 			require.Contains(t, output, "EOF")
+
+			// file should not have been created
+
+			_, err := os.Stat(file)
+			require.Error(t, err, "file is not supposed to exist")
+			require.True(t, os.IsNotExist(err), "file is not supposed to exist")
+
+			// also check that the unverified inputs are gone as well
+			_, err = os.Stat(file + ".unverified")
+			require.Error(t, err, "unverified file is not supposed to exist")
+			require.True(t, os.IsNotExist(err), "unverified file is not supposed to exist")
 		})
 	}
 }

go.mod

@@ -70,6 +70,7 @@ require (
 	github.com/pascaldekloe/goe v0.1.0
 	github.com/pkg/errors v0.8.1
 	github.com/prometheus/client_golang v1.0.0
+	github.com/rboyer/safeio v0.2.1
 	github.com/ryanuber/columnize v2.1.0+incompatible
 	github.com/shirou/gopsutil v0.0.0-20181107111621-48177ef5f880
 	github.com/shirou/w32 v0.0.0-20160930032740-bb4de0191aa4 // indirect

go.sum

@@ -396,6 +396,8 @@ github.com/prometheus/procfs v0.0.0-20190507164030-5867b95ac084/go.mod h1:TjEm7z
 github.com/prometheus/procfs v0.0.2 h1:6LJUbpNm42llc4HRCuvApCSWB/WfhuNo9K98Q9sNGfs=
 github.com/prometheus/procfs v0.0.2/go.mod h1:TjEm7ze935MbeOT/UhFTIMYKhuLP4wbCsTZCD3I8kEA=
 github.com/prometheus/tsdb v0.7.1/go.mod h1:qhTCs0VvXwvX/y3TZrWD7rabWM+ijKTux40TwIPHuXU=
+github.com/rboyer/safeio v0.2.1 h1:05xhhdRNAdS3apYm7JRjOqngf4xruaW959jmRxGDuSU=
+github.com/rboyer/safeio v0.2.1/go.mod h1:Cq/cEPK+YXFn622lsQ0K4KsPZSPtaptHHEldsy7Fmig=
 github.com/renier/xmlrpc v0.0.0-20170708154548-ce4a1a486c03 h1:Wdi9nwnhFNAlseAOekn6B5G/+GMtks9UKbvRU/CMM/o=
 github.com/renier/xmlrpc v0.0.0-20170708154548-ce4a1a486c03/go.mod h1:gRAiPF5C5Nd0eyyRdqIu9qTiFSoZzpTq727b5B8fkkU=
 github.com/rogpeppe/fastuuid v0.0.0-20150106093220-6724a57986af/go.mod h1:XWv6SoW27p1b0cqNHllgS5HIMJraePCO15w5zCzIWYg=

vendor/github.com/rboyer/safeio/.gitignore

@@ -0,0 +1,23 @@
+# Compiled Object files, Static and Dynamic libs (Shared Objects)
+*.o
+*.a
+*.so
+
+# Folders
+_obj
+_test
+
+# Architecture specific extensions/prefixes
+*.[568vq]
+[568vq].out
+
+*.cgo1.go
+*.cgo2.c
+_cgo_defun.c
+_cgo_gotypes.go
+_cgo_export.*
+
+_testmain.go
+
+*.exe
+

vendor/github.com/rboyer/safeio/.travis.yml

@@ -0,0 +1,9 @@
+language: go
+
+go:
+  - 1.6.2
+
+branches:
+  only:
+    - master
+

vendor/github.com/rboyer/safeio/LICENSE

@@ -0,0 +1,22 @@
+MIT License
+
+Copyright (c) 2020 Richard Boyer
+
+Permission is hereby granted, free of charge, to any person obtaining a copy
+of this software and associated documentation files (the "Software"), to deal
+in the Software without restriction, including without limitation the rights
+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+copies of the Software, and to permit persons to whom the Software is
+furnished to do so, subject to the following conditions:
+
+The above copyright notice and this permission notice shall be included in all
+copies or substantial portions of the Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+SOFTWARE.
+

vendor/github.com/rboyer/safeio/README.md

@@ -0,0 +1,6 @@
+Safe I/O
+========
+
+Provides functions to perform atomic, fsync-safe disk operations.
+
+[![Build Status](https://travis-ci.org/rboyer/safeio.svg?branch=master)](https://travis-ci.org/rboyer/safeio)

vendor/github.com/rboyer/safeio/file.go

@@ -0,0 +1,123 @@
+package safeio
+
+import (
+	"errors"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+)
+
+var errClosed = errors.New("file is already closed")
+
+// OpenFile is the incremental version of WriteToFile.  It opens a temp
+// file and proxies writes through to the underlying file.
+//
+// If Close is called before Commit, the temp file is closed and erased.
+//
+// If Commit is called before Close, the temp file is closed, fsynced,
+// and atomically renamed to the desired final name.
+func OpenFile(path string, perm os.FileMode) (*File, error) {
+	dir := filepath.Dir(path)
+	name := filepath.Base(path)
+
+	f, err := ioutil.TempFile(dir, name+".tmp")
+	if err != nil {
+		return nil, err
+	}
+
+	return &File{
+		name:     path,
+		tempName: f.Name(),
+		perm:     perm,
+		file:     f,
+	}, nil
+}
+
+// File is an implementation detail of OpenFile.
+type File struct {
+	name     string // track desired filename
+	tempName string // track actual filename
+	perm     os.FileMode
+	file     *os.File
+	closed   bool
+	err      error // the first error encountered
+}
+
+// Write is a thin proxy to *os.File#Write.
+//
+// If Close or Commit were called, this immediately exits with an error.
+func (f *File) Write(p []byte) (n int, err error) {
+	if f.closed {
+		return 0, errClosed
+	} else if f.err != nil {
+		return 0, f.err
+	}
+
+	n, err = f.file.Write(p)
+	if err != nil {
+		f.err = err
+	}
+
+	return n, err
+}
+
+// Commit causes the current temp file to be safely persisted to disk and atomically renamed to the desired final filename.
+//
+// It is safe to call Close after commit, so you can defer Close as
+// usual without worries about write-safey.
+func (f *File) Commit() error {
+	if f.closed {
+		return errClosed
+	} else if f.err != nil {
+		return f.err
+	}
+
+	if err := f.file.Sync(); err != nil {
+		return f.cleanup(err)
+	}
+
+	if err := f.file.Chmod(f.perm); err != nil {
+		return f.cleanup(err)
+	}
+
+	if err := f.file.Close(); err != nil {
+		return f.cleanup(err)
+	}
+
+	if err := Rename(f.tempName, f.name); err != nil {
+		return f.cleanup(err)
+	}
+
+	f.closed = true
+
+	return nil
+}
+
+// Close closes the current file and erases it, unless Commit was
+// previously called.  In that case it does nothing.
+//
+// Close is idempotent.
+//
+// After Close is called, Write and Commit will fail.
+func (f *File) Close() error {
+	if !f.closed {
+		_ = f.cleanup(nil)
+		f.closed = true
+	}
+	return f.err
+}
+
+func (f *File) cleanup(err error) error {
+	_ = f.file.Close()
+	_ = os.Remove(f.tempName)
+
+	if f.err == nil {
+		f.err = err
+	}
+	return f.err
+}
+
+// setErr is only used during testing to simulate os.File errors
+func (f *File) setErr(err error) {
+	f.err = err
+}

vendor/github.com/rboyer/safeio/go.mod

@@ -0,0 +1,5 @@
+module github.com/rboyer/safeio
+
+go 1.14
+
+require github.com/stretchr/testify v1.4.0

vendor/github.com/rboyer/safeio/go.sum

@@ -0,0 +1,11 @@
+github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8=
+github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
+github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
+github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
+github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
+github.com/stretchr/testify v1.4.0 h1:2E4SXV/wtOkTonXsotYi4li6zVWxYlZuYNCXe9XRJyk=
+github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=
+gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
+gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
+gopkg.in/yaml.v2 v2.2.2 h1:ZCJp+EgiOT7lHqUV2J862kp8Qj64Jo6az82+3Td9dZw=
+gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=

vendor/github.com/rboyer/safeio/safeio.go

@@ -0,0 +1,96 @@
+// Package safeio provides functions to perform atomic, fsync-safe disk
+// operations.
+package safeio
+
+import (
+	"io"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+)
+
+// WriteToFile consumes the provided io.Reader and writes it to a temp
+// file in the provided directory.
+func WriteToFile(src io.Reader, path string, perm os.FileMode) (written int64, err error) {
+	tempName, written, err := writeToTempFile(src, path, perm)
+
+	if err == nil {
+		err = Rename(tempName, path)
+	}
+
+	return written, err
+}
+
+// writeToTempFile consumes the provided io.Reader and writes it to a
+// temp file in the same directory as path.
+func writeToTempFile(src io.Reader, path string, perm os.FileMode) (tempName string, written int64, err error) {
+	dir := filepath.Dir(path)
+	name := filepath.Base(path)
+
+	f, err := ioutil.TempFile(dir, name+".tmp")
+	if err != nil {
+		return "", 0, err
+	}
+
+	tempName = f.Name()
+
+	cleanup := func(written int64, err error) (string, int64, error) {
+		_ = f.Close()
+		_ = os.Remove(tempName)
+		return "", written, err
+	}
+
+	if err = f.Chmod(perm); err != nil {
+		return cleanup(0, err)
+	}
+
+	written, err = io.Copy(f, src)
+	if err != nil {
+		return cleanup(written, err)
+	}
+
+	if err := f.Sync(); err != nil {
+		return cleanup(written, err)
+	}
+
+	if err := f.Close(); err != nil {
+		return cleanup(written, err)
+	}
+
+	return tempName, written, nil
+}
+
+// Remove is just like os.Remove, except this also calls sync on the
+// parent directory.
+func Remove(fn string) error {
+	err := os.Remove(fn)
+	if err != nil {
+		return err
+	}
+
+	// fsync the dir
+	return syncParentDir(fn)
+}
+
+// Rename renames the file using os.Rename and fsyncs the NEW parent
+// directory. It should only be used if both oldname and newname are in
+// the same directory.
+func Rename(oldname, newname string) error {
+	err := os.Rename(oldname, newname)
+	if err != nil {
+		return err
+	}
+
+	// fsync the dir
+	return syncParentDir(newname)
+}
+
+func syncParentDir(name string) error {
+	f, err := os.Open(filepath.Dir(name))
+	if err != nil {
+		return err
+	}
+	defer f.Close()
+
+	return f.Sync()
+}

vendor/modules.txt

@@ -344,6 +344,8 @@ github.com/prometheus/common/model
 # github.com/prometheus/procfs v0.0.2
 github.com/prometheus/procfs
 github.com/prometheus/procfs/internal/fs
+# github.com/rboyer/safeio v0.2.1
+github.com/rboyer/safeio
 # github.com/renier/xmlrpc v0.0.0-20170708154548-ce4a1a486c03
 github.com/renier/xmlrpc
 # github.com/ryanuber/columnize v2.1.0+incompatible