From 82702e24fdf99b6692fbdc86c96e10f23c429d82 Mon Sep 17 00:00:00 2001
From: Jackson Owens <jackson@cockroachlabs.com>
Date: Thu, 29 Apr 2021 10:16:30 -0400
Subject: [PATCH] pkg/storage: sync file in SafeWriteToFile

Previously, `storage.SafeWriteToFile` did not guarantee that the file it
wrote was synced to the storage media.

Release note (bug fix): Fix a bug where encryption-at-rest metadata was
not synced and might become corrupted during a hard reset.
---
 pkg/storage/file_util.go      | 11 +++---
 pkg/storage/file_util_test.go | 66 +++++++++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+), 4 deletions(-)
 create mode 100644 pkg/storage/file_util_test.go

diff --git a/pkg/storage/file_util.go b/pkg/storage/file_util.go
index 2e546bf6e25b..1420d495c8d4 100644
--- a/pkg/storage/file_util.go
+++ b/pkg/storage/file_util.go
@@ -12,19 +12,18 @@ package storage
 
 import (
 	"bytes"
-	"fmt"
 	"io"
 
 	"github.com/cockroachdb/pebble/vfs"
 )
 
-// SafeWriteToFile writes the byte slice to the filename, contained in dir, using the given fs.
-// It returns after both the file and the containing directory are synced.
+// SafeWriteToFile writes the byte slice to the filename, contained in dir,
+// using the given fs.  It returns after both the file and the containing
+// directory are synced.
 func SafeWriteToFile(fs vfs.FS, dir string, filename string, b []byte) error {
 	tempName := filename + ".crdbtmp"
 	f, err := fs.Create(tempName)
 	if err != nil {
-		fmt.Printf("%v\n", err)
 		return err
 	}
 	bReader := bytes.NewReader(b)
@@ -32,6 +31,10 @@ func SafeWriteToFile(fs vfs.FS, dir string, filename string, b []byte) error {
 		f.Close()
 		return err
 	}
+	if err = f.Sync(); err != nil {
+		f.Close()
+		return err
+	}
 	if err = f.Close(); err != nil {
 		return err
 	}
diff --git a/pkg/storage/file_util_test.go b/pkg/storage/file_util_test.go
new file mode 100644
index 000000000000..df9483e526af
--- /dev/null
+++ b/pkg/storage/file_util_test.go
@@ -0,0 +1,66 @@
+// Copyright 2021 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package storage
+
+import (
+	"io"
+	"io/ioutil"
+	"os"
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+	"github.com/cockroachdb/pebble/vfs"
+	"github.com/stretchr/testify/require"
+)
+
+func TestSafeWriteToFile(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	// Use an in-memory FS that strictly enforces syncs.
+	mem := vfs.NewStrictMem()
+	syncDir := func(dir string) {
+		fdir, err := mem.OpenDir(dir)
+		require.NoError(t, err)
+		require.NoError(t, fdir.Sync())
+		require.NoError(t, fdir.Close())
+	}
+	readFile := func(filename string) []byte {
+		f, err := mem.Open("foo/bar")
+		require.NoError(t, err)
+		b, err := ioutil.ReadAll(f)
+		require.NoError(t, err)
+		require.NoError(t, f.Close())
+		return b
+	}
+
+	require.NoError(t, mem.MkdirAll("foo", os.ModePerm))
+	syncDir("")
+	f, err := mem.Create("foo/bar")
+	require.NoError(t, err)
+	_, err = io.WriteString(f, "Hello world")
+	require.NoError(t, err)
+	require.NoError(t, f.Sync())
+	require.NoError(t, f.Close())
+	syncDir("foo")
+
+	// Discard any unsynced writes to make sure we set up the test
+	// preconditions correctly.
+	mem.ResetToSyncedState()
+	require.Equal(t, []byte("Hello world"), readFile("foo/bar"))
+
+	// Use SafeWriteToFile to atomically, durably change the contents of the
+	// file.
+	require.NoError(t, SafeWriteToFile(mem, "foo", "foo/bar", []byte("Hello everyone")))
+
+	// Discard any unsynced writes.
+	mem.ResetToSyncedState()
+	require.Equal(t, []byte("Hello everyone"), readFile("foo/bar"))
+}