From 8a51210efca6ecd881d3336c8d7b4f6ef0ff30e2 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Fri, 28 Jul 2017 11:22:48 -0400 Subject: [PATCH 1/3] [common] use temp intead of {filePath}.new The problem with {filePath}.new is that it is not safe for concurrent use! Calling this function with the same params results in the following error: ``` panic: Panicked on a Crisis: rename /root/.tendermint_test/consensus_replay_test/priv_validator.json.new /root/.tendermint_test/consensus_replay_test/priv_validator.json: no such file or directory goroutine 47860 [running]: github.com/tendermint/tendermint/vendor/github.com/tendermint/tmlibs/common.PanicCrisis(0xcba800, 0xc42152d640) /go/src/github.com/tendermint/tendermint/vendor/github.com/tendermint/tmlibs/common/errors.go:33 +0x10f github.com/tendermint/tendermint/types.(*PrivValidator).save(0xc42235f2c0) /go/src/github.com/tendermint/tendermint/types/priv_validator.go:165 +0x159 github.com/tendermint/tendermint/types.(*PrivValidator).signBytesHRS(0xc42235f2c0, 0x6, 0x0, 0xc424e88f03, 0xc429908580, 0xca, 0x155, 0x80, 0xc424e88f00, 0x7f4ecafc88d0, ...) /go/src/github.com/tendermint/tendermint/types/priv_validator.go:249 +0x2bb github.com/tendermint/tendermint/types.(*PrivValidator).SignVote(0xc42235f2c0, 0xc4228c7460, 0xf, 0xc424e88f00, 0x0, 0x0) /go/src/github.com/tendermint/tendermint/types/priv_validator.go:186 +0x1a2 github.com/tendermint/tendermint/consensus.(*ConsensusState).signVote(0xc424efd520, 0xc400000002, 0xc422d5e3c0, 0x14, 0x20, 0x1, 0xc4247b6560, 0x14, 0x20, 0x0, ...) github.com/tendermint/tendermint/consensus/_test/_obj_test/state.go:1556 +0x35e github.com/tendermint/tendermint/consensus.(*ConsensusState).signAddVote(0xc424efd520, 0x2, 0xc422d5e3c0, 0x14, 0x20, 0x1, 0xc4247b6560, 0x14, 0x20, 0xc42001b300) github.com/tendermint/tendermint/consensus/_test/_obj_test/state.go:1568 +0x200 github.com/tendermint/tendermint/consensus.(*ConsensusState).enterPrecommit(0xc424efd520, 0x6, 0x0) github.com/tendermint/tendermint/consensus/_test/_obj_test/state.go:1082 +0x13a4 github.com/tendermint/tendermint/consensus.(*ConsensusState).addVote(0xc424efd520, 0xc424e88780, 0x0, 0x0, 0x39, 0x1dc, 0x28c) github.com/tendermint/tendermint/consensus/_test/_obj_test/state.go:1477 +0x1be5 github.com/tendermint/tendermint/consensus.(*ConsensusState).tryAddVote(0xc424efd520, 0xc424e88780, 0x0, 0x0, 0xd7fb00, 0xc42152ce00) github.com/tendermint/tendermint/consensus/_test/_obj_test/state.go:1382 +0x93 github.com/tendermint/tendermint/consensus.(*ConsensusState).handleMsg(0xc424efd520, 0xcb58e0, 0xc42547feb8, 0x0, 0x0, 0x6, 0x0, 0x4, 0xed10ca07e, 0x3077bfea, ...) github.com/tendermint/tendermint/consensus/_test/_obj_test/state.go:660 +0x9fc github.com/tendermint/tendermint/consensus.(*ConsensusState).receiveRoutine(0xc424efd520, 0x0) github.com/tendermint/tendermint/consensus/_test/_obj_test/state.go:615 +0x5f5 created by github.com/tendermint/tendermint/consensus.(*ConsensusState).OnStart github.com/tendermint/tendermint/consensus/_test/_obj_test/state.go:332 +0x4a7 exit status 2 FAIL github.com/tendermint/tendermint/consensus 76.644s make: *** [test_integrations] Error 1 ``` See https://github.com/tendermint/tendermint/pull/568 --- common/os.go | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/common/os.go b/common/os.go index 9dc81c579..ae2ed087d 100644 --- a/common/os.go +++ b/common/os.go @@ -93,9 +93,8 @@ func MustWriteFile(filePath string, contents []byte, mode os.FileMode) { } } -// Writes to newBytes to filePath. -// Guaranteed not to lose *both* oldBytes and newBytes, -// (assuming that the OS is perfect) +// WriteFileAtomic writes newBytes to temp and atomically moves to filePath +// when everything else succeeds. func WriteFileAtomic(filePath string, newBytes []byte, mode os.FileMode) error { // If a file already exists there, copy to filePath+".bak" (overwrite anything) if _, err := os.Stat(filePath); !os.IsNotExist(err) { @@ -108,13 +107,27 @@ func WriteFileAtomic(filePath string, newBytes []byte, mode os.FileMode) error { return fmt.Errorf("Could not write file %v. %v", filePath+".bak", err) } } - // Write newBytes to filePath.new - err := ioutil.WriteFile(filePath+".new", newBytes, mode) + f, err := ioutil.TempFile("", "") if err != nil { - return fmt.Errorf("Could not write file %v. %v", filePath+".new", err) + return err + } + _, err = f.Write(newBytes) + if err == nil { + err = f.Sync() + } + if closeErr := f.Close(); err == nil { + err = closeErr + } + if permErr := os.Chmod(f.Name(), mode); err == nil { + err = permErr + } + if err == nil { + err = os.Rename(f.Name(), filePath) + } + // any err should result in full cleanup + if err != nil { + os.Remove(f.Name()) } - // Move filePath.new to filePath - err = os.Rename(filePath+".new", filePath) return err } From b25aa3b472f67638710954460ef4c77e28dd9e8f Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Fri, 28 Jul 2017 11:26:04 -0400 Subject: [PATCH 2/3] [common] do not create {filePath}.bak in WriteFileAtomic We use WriteFileAtomic in two places: ``` p2p/addrbook.go 338: err = cmn.WriteFileAtomic(filePath, jsonBytes, 0644) types/priv_validator.go 162: err = WriteFileAtomic(privVal.filePath, jsonBytes, 0600) ``` and we don't need .bak in any of the above. We save priv_validator every 10ms and addrbook every 2 min. --- common/os.go | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/common/os.go b/common/os.go index ae2ed087d..b1e778977 100644 --- a/common/os.go +++ b/common/os.go @@ -96,17 +96,6 @@ func MustWriteFile(filePath string, contents []byte, mode os.FileMode) { // WriteFileAtomic writes newBytes to temp and atomically moves to filePath // when everything else succeeds. func WriteFileAtomic(filePath string, newBytes []byte, mode os.FileMode) error { - // If a file already exists there, copy to filePath+".bak" (overwrite anything) - if _, err := os.Stat(filePath); !os.IsNotExist(err) { - fileBytes, err := ioutil.ReadFile(filePath) - if err != nil { - return fmt.Errorf("Could not read file %v. %v", filePath, err) - } - err = ioutil.WriteFile(filePath+".bak", fileBytes, mode) - if err != nil { - return fmt.Errorf("Could not write file %v. %v", filePath+".bak", err) - } - } f, err := ioutil.TempFile("", "") if err != nil { return err From d1ca2c6f838fe7428b7da6f62ebf293d4d91b44b Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Fri, 28 Jul 2017 11:31:50 -0400 Subject: [PATCH 3/3] [common] add a test for WriteFileAtomic --- common/os_test.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 common/os_test.go diff --git a/common/os_test.go b/common/os_test.go new file mode 100644 index 000000000..05359e36e --- /dev/null +++ b/common/os_test.go @@ -0,0 +1,29 @@ +package common + +import ( + "bytes" + "fmt" + "io/ioutil" + "os" + "testing" + "time" +) + +func TestWriteFileAtomic(t *testing.T) { + data := []byte("Becatron") + fname := fmt.Sprintf("/tmp/write-file-atomic-test-%v.txt", time.Now().UnixNano()) + err := WriteFileAtomic(fname, data, 0664) + if err != nil { + t.Fatal(err) + } + rData, err := ioutil.ReadFile(fname) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(data, rData) { + t.Fatalf("data mismatch: %v != %v", data, rData) + } + if err := os.Remove(fname); err != nil { + t.Fatal(err) + } +}