From a83eed104c13813fb5b4f9b94e40470f88f2f863 Mon Sep 17 00:00:00 2001 From: ValarDragon Date: Mon, 30 Jul 2018 16:19:33 -0700 Subject: [PATCH] libs/cmn: Remove Tempfile, Tempdir, switch to ioutil variants (#2114) Our Tempfile was just a wrapper on ioutil that panicked instead of error. Our Tempdir was a less safe variant of ioutil's Tempdir. --- CHANGELOG_PENDING.md | 2 +- consensus/common_test.go | 7 +++++-- libs/autofile/autofile_test.go | 9 +++++++-- libs/common/tempfile.go | 24 ------------------------ libs/db/backend_test.go | 5 +++-- libs/db/common_test.go | 6 +++--- privval/priv_validator_test.go | 32 +++++++++++++++++++------------- 7 files changed, 38 insertions(+), 47 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 481f5b452..a65c6e45c 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -22,6 +22,6 @@ IMPROVEMENTS: - [common] bit array functions which take in another parameter are now thread safe BUG FIXES: -- [common] \#2109 Safely handle cases where atomic write files already exist +- [common] Safely handle cases where atomic write files already exist [#2109](https://github.com/tendermint/tendermint/issues/2109) - [privval] fix a deadline for accepting new connections in socket private validator. diff --git a/consensus/common_test.go b/consensus/common_test.go index 992b6fcd6..f4855992d 100644 --- a/consensus/common_test.go +++ b/consensus/common_test.go @@ -378,8 +378,11 @@ func randConsensusNetWithPeers(nValidators, nPeers int, testName string, tickerF if i < nValidators { privVal = privVals[i] } else { - _, tempFilePath := cmn.Tempfile("priv_validator_") - privVal = privval.GenFilePV(tempFilePath) + tempFile, err := ioutil.TempFile("", "priv_validator_") + if err != nil { + panic(err) + } + privVal = privval.GenFilePV(tempFile.Name()) } app := appFunc() diff --git a/libs/autofile/autofile_test.go b/libs/autofile/autofile_test.go index b39fb7cf3..67397380b 100644 --- a/libs/autofile/autofile_test.go +++ b/libs/autofile/autofile_test.go @@ -1,6 +1,7 @@ package autofile import ( + "io/ioutil" "os" "sync/atomic" "syscall" @@ -13,10 +14,14 @@ import ( func TestSIGHUP(t *testing.T) { // First, create an AutoFile writing to a tempfile dir - file, name := cmn.Tempfile("sighup_test") - if err := file.Close(); err != nil { + file, err := ioutil.TempFile("", "sighup_test") + if err != nil { t.Fatalf("Error creating tempfile: %v", err) } + if err := file.Close(); err != nil { + t.Fatalf("Error closing tempfile: %v", err) + } + name := file.Name() // Here is the actual AutoFile af, err := OpenAutoFile(name) if err != nil { diff --git a/libs/common/tempfile.go b/libs/common/tempfile.go index bc1343e5d..a5bb7a5b5 100644 --- a/libs/common/tempfile.go +++ b/libs/common/tempfile.go @@ -3,7 +3,6 @@ package common import ( fmt "fmt" "io" - "io/ioutil" "os" "path/filepath" "strconv" @@ -127,26 +126,3 @@ func WriteFileAtomic(filename string, data []byte, perm os.FileMode) (err error) return os.Rename(f.Name(), filename) } - -//-------------------------------------------------------------------------------- - -func Tempfile(prefix string) (*os.File, string) { - file, err := ioutil.TempFile("", prefix) - if err != nil { - PanicCrisis(err) - } - return file, file.Name() -} - -func Tempdir(prefix string) (*os.File, string) { - tempDir := os.TempDir() + "/" + prefix + RandStr(12) - err := EnsureDir(tempDir, 0700) - if err != nil { - panic(Fmt("Error creating temp dir: %v", err)) - } - dir, err := os.Open(tempDir) - if err != nil { - panic(Fmt("Error opening temp dir: %v", err)) - } - return dir, tempDir -} diff --git a/libs/db/backend_test.go b/libs/db/backend_test.go index 493ed83f9..b31b4d74a 100644 --- a/libs/db/backend_test.go +++ b/libs/db/backend_test.go @@ -2,6 +2,7 @@ package db import ( "fmt" + "io/ioutil" "os" "path/filepath" "testing" @@ -17,8 +18,8 @@ func cleanupDBDir(dir, name string) { func testBackendGetSetDelete(t *testing.T, backend DBBackendType) { // Default - dir, dirname := cmn.Tempdir(fmt.Sprintf("test_backend_%s_", backend)) - defer dir.Close() + dirname, err := ioutil.TempDir("", fmt.Sprintf("test_backend_%s_", backend)) + require.Nil(t, err) db := NewDB("testdb", backend, dirname) // A nonexistent key should return nil, even if the key is empty diff --git a/libs/db/common_test.go b/libs/db/common_test.go index 027b8ee53..68420cd2b 100644 --- a/libs/db/common_test.go +++ b/libs/db/common_test.go @@ -2,12 +2,12 @@ package db import ( "fmt" + "io/ioutil" "sync" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - cmn "github.com/tendermint/tendermint/libs/common" ) //---------------------------------------- @@ -61,9 +61,9 @@ func checkValuePanics(t *testing.T, itr Iterator) { } func newTempDB(t *testing.T, backend DBBackendType) (db DB) { - dir, dirname := cmn.Tempdir("db_common_test") + dirname, err := ioutil.TempDir("", "db_common_test") + require.Nil(t, err) db = NewDB("testdb", backend, dirname) - dir.Close() return db } diff --git a/privval/priv_validator_test.go b/privval/priv_validator_test.go index 7c9c93fcf..127f5c1f3 100644 --- a/privval/priv_validator_test.go +++ b/privval/priv_validator_test.go @@ -3,6 +3,7 @@ package privval import ( "encoding/base64" "fmt" + "io/ioutil" "os" "testing" "time" @@ -11,22 +12,22 @@ import ( "github.com/stretchr/testify/require" "github.com/tendermint/tendermint/crypto" "github.com/tendermint/tendermint/crypto/ed25519" - cmn "github.com/tendermint/tendermint/libs/common" "github.com/tendermint/tendermint/types" ) func TestGenLoadValidator(t *testing.T) { assert := assert.New(t) - _, tempFilePath := cmn.Tempfile("priv_validator_") - privVal := GenFilePV(tempFilePath) + tempFile, err := ioutil.TempFile("", "priv_validator_") + require.Nil(t, err) + privVal := GenFilePV(tempFile.Name()) height := int64(100) privVal.LastHeight = height privVal.Save() addr := privVal.GetAddress() - privVal = LoadFilePV(tempFilePath) + privVal = LoadFilePV(tempFile.Name()) assert.Equal(addr, privVal.GetAddress(), "expected privval addr to be the same") assert.Equal(height, privVal.LastHeight, "expected privval.LastHeight to have been saved") } @@ -34,7 +35,9 @@ func TestGenLoadValidator(t *testing.T) { func TestLoadOrGenValidator(t *testing.T) { assert := assert.New(t) - _, tempFilePath := cmn.Tempfile("priv_validator_") + tempFile, err := ioutil.TempFile("", "priv_validator_") + require.Nil(t, err) + tempFilePath := tempFile.Name() if err := os.Remove(tempFilePath); err != nil { t.Error(err) } @@ -91,8 +94,9 @@ func TestUnmarshalValidator(t *testing.T) { func TestSignVote(t *testing.T) { assert := assert.New(t) - _, tempFilePath := cmn.Tempfile("priv_validator_") - privVal := GenFilePV(tempFilePath) + tempFile, err := ioutil.TempFile("", "priv_validator_") + require.Nil(t, err) + privVal := GenFilePV(tempFile.Name()) block1 := types.BlockID{[]byte{1, 2, 3}, types.PartSetHeader{}} block2 := types.BlockID{[]byte{3, 2, 1}, types.PartSetHeader{}} @@ -101,7 +105,7 @@ func TestSignVote(t *testing.T) { // sign a vote for first time vote := newVote(privVal.Address, 0, height, round, voteType, block1) - err := privVal.SignVote("mychainid", vote) + err = privVal.SignVote("mychainid", vote) assert.NoError(err, "expected no error signing vote") // try to sign the same vote again; should be fine @@ -132,8 +136,9 @@ func TestSignVote(t *testing.T) { func TestSignProposal(t *testing.T) { assert := assert.New(t) - _, tempFilePath := cmn.Tempfile("priv_validator_") - privVal := GenFilePV(tempFilePath) + tempFile, err := ioutil.TempFile("", "priv_validator_") + require.Nil(t, err) + privVal := GenFilePV(tempFile.Name()) block1 := types.PartSetHeader{5, []byte{1, 2, 3}} block2 := types.PartSetHeader{10, []byte{3, 2, 1}} @@ -141,7 +146,7 @@ func TestSignProposal(t *testing.T) { // sign a proposal for first time proposal := newProposal(height, round, block1) - err := privVal.SignProposal("mychainid", proposal) + err = privVal.SignProposal("mychainid", proposal) assert.NoError(err, "expected no error signing proposal") // try to sign the same proposal again; should be fine @@ -170,8 +175,9 @@ func TestSignProposal(t *testing.T) { } func TestDifferByTimestamp(t *testing.T) { - _, tempFilePath := cmn.Tempfile("priv_validator_") - privVal := GenFilePV(tempFilePath) + tempFile, err := ioutil.TempFile("", "priv_validator_") + require.Nil(t, err) + privVal := GenFilePV(tempFile.Name()) block1 := types.PartSetHeader{5, []byte{1, 2, 3}} height, round := int64(10), 1