From b8a9b0bf782c17d97af3fda46ddc73f7d35bd680 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 6 Nov 2018 07:39:05 +0100 Subject: [PATCH] Mempool WAL is still created by default in home directory, leads to permission errors (#2758) * only invoke InitWAL/CloseWAL if WalPath is not empty Closes #2717 * panic if WAL is not initialized when calling CloseWAL * add a changelog entry --- CHANGELOG_PENDING.md | 1 + config/config.go | 5 +++++ mempool/mempool.go | 46 ++++++++++++++++++----------------------- mempool/mempool_test.go | 7 ++----- node/node.go | 11 ++++++++-- 5 files changed, 37 insertions(+), 33 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index f5e56a123..ea0a666bd 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -25,3 +25,4 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi ### IMPROVEMENTS: ### BUG FIXES: +- [mempool] fix a bug where we create a WAL despite `wal_dir` being empty diff --git a/config/config.go b/config/config.go index ede57207c..fa6182114 100644 --- a/config/config.go +++ b/config/config.go @@ -497,6 +497,11 @@ func (cfg *MempoolConfig) WalDir() string { return rootify(cfg.WalPath, cfg.RootDir) } +// WalEnabled returns true if the WAL is enabled. +func (cfg *MempoolConfig) WalEnabled() bool { + return cfg.WalPath != "" +} + // ValidateBasic performs basic validation (checking param bounds, etc.) and // returns an error if any check fails. func (cfg *MempoolConfig) ValidateBasic() error { diff --git a/mempool/mempool.go b/mempool/mempool.go index 5e52989aa..9c9563165 100644 --- a/mempool/mempool.go +++ b/mempool/mempool.go @@ -213,39 +213,33 @@ func WithMetrics(metrics *Metrics) MempoolOption { return func(mem *Mempool) { mem.metrics = metrics } } -// CloseWAL closes and discards the underlying WAL file. -// Any further writes will not be relayed to disk. -func (mem *Mempool) CloseWAL() bool { - if mem == nil { - return false +// InitWAL creates a directory for the WAL file and opens a file itself. +// +// *panics* if can't create directory or open file. +// *not thread safe* +func (mem *Mempool) InitWAL() { + walDir := mem.config.WalDir() + err := cmn.EnsureDir(walDir, 0700) + if err != nil { + panic(errors.Wrap(err, "Error ensuring Mempool WAL dir")) + } + af, err := auto.OpenAutoFile(walDir + "/wal") + if err != nil { + panic(errors.Wrap(err, "Error opening Mempool WAL file")) } + mem.wal = af +} +// CloseWAL closes and discards the underlying WAL file. +// Any further writes will not be relayed to disk. +func (mem *Mempool) CloseWAL() { mem.proxyMtx.Lock() defer mem.proxyMtx.Unlock() - if mem.wal == nil { - return false - } - if err := mem.wal.Close(); err != nil && mem.logger != nil { - mem.logger.Error("Mempool.CloseWAL", "err", err) + if err := mem.wal.Close(); err != nil { + mem.logger.Error("Error closing WAL", "err", err) } mem.wal = nil - return true -} - -func (mem *Mempool) InitWAL() { - walDir := mem.config.WalDir() - if walDir != "" { - err := cmn.EnsureDir(walDir, 0700) - if err != nil { - cmn.PanicSanity(errors.Wrap(err, "Error ensuring Mempool wal dir")) - } - af, err := auto.OpenAutoFile(walDir + "/wal") - if err != nil { - cmn.PanicSanity(errors.Wrap(err, "Error opening Mempool wal file")) - } - mem.wal = af - } } // Lock locks the mempool. The consensus must be able to hold lock to safely update. diff --git a/mempool/mempool_test.go b/mempool/mempool_test.go index 44917afba..b8df6f737 100644 --- a/mempool/mempool_test.go +++ b/mempool/mempool_test.go @@ -369,15 +369,12 @@ func TestMempoolCloseWAL(t *testing.T) { // 7. Invoke CloseWAL() and ensure it discards the // WAL thus any other write won't go through. - require.True(t, mempool.CloseWAL(), "CloseWAL should CloseWAL") + mempool.CloseWAL() mempool.CheckTx(types.Tx([]byte("bar")), nil) sum2 := checksumFile(walFilepath, t) require.Equal(t, sum1, sum2, "expected no change to the WAL after invoking CloseWAL() since it was discarded") - // 8. Second CloseWAL should do nothing - require.False(t, mempool.CloseWAL(), "CloseWAL should CloseWAL") - - // 9. Sanity check to ensure that the WAL file still exists + // 8. Sanity check to ensure that the WAL file still exists m3, err := filepath.Glob(filepath.Join(rootDir, "*")) require.Nil(t, err, "successful globbing expected") require.Equal(t, 1, len(m3), "expecting the wal match in") diff --git a/node/node.go b/node/node.go index f62a8b472..4710397fa 100644 --- a/node/node.go +++ b/node/node.go @@ -13,8 +13,8 @@ import ( "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promhttp" - "github.com/tendermint/go-amino" + amino "github.com/tendermint/go-amino" abci "github.com/tendermint/tendermint/abci/types" bc "github.com/tendermint/tendermint/blockchain" cfg "github.com/tendermint/tendermint/config" @@ -279,7 +279,9 @@ func NewNode(config *cfg.Config, ) mempoolLogger := logger.With("module", "mempool") mempool.SetLogger(mempoolLogger) - mempool.InitWAL() // no need to have the mempool wal during tests + if config.Mempool.WalEnabled() { + mempool.InitWAL() // no need to have the mempool wal during tests + } mempoolReactor := mempl.NewMempoolReactor(config.Mempool, mempool) mempoolReactor.SetLogger(mempoolLogger) @@ -586,6 +588,11 @@ func (n *Node) OnStop() { // TODO: gracefully disconnect from peers. n.sw.Stop() + // stop mempool WAL + if n.config.Mempool.WalEnabled() { + n.mempoolReactor.Mempool.CloseWAL() + } + if err := n.transport.Close(); err != nil { n.Logger.Error("Error closing transport", "err", err) }