From e6b545b41002333b6a91dbc150c4208eb7b6afdf Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Fri, 10 Apr 2020 11:20:30 -0700 Subject: [PATCH 1/6] chain/neutrino: fix nil pointer dereference This commit fixes a potential nil pointer dereference in the neutrino chain client. Currently the Rescan method blindly overwrites the rescan member with nil _after_ reacquiring the lock. However, the field may have been populated with a new rescan while the mutex wasn't held, causing a panic later on where we assume the rescan is non-nil. To remedy, we add a simple check that asserts we are nilling the same rescan we were shutting down while the mutex was unlocked. If we find a differing value, we will continue to this process until we arrive at matching rescan pointers, at which point we know we can safely proceed in creating a new rescan. --- chain/neutrino.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/chain/neutrino.go b/chain/neutrino.go index 28987f7a32..c133aa2ba6 100644 --- a/chain/neutrino.go +++ b/chain/neutrino.go @@ -341,15 +341,21 @@ func (s *NeutrinoClient) Rescan(startHash *chainhash.Hash, addrs []btcutil.Addre return fmt.Errorf("can't do a rescan when the chain client " + "is not started") } - if s.scanning { + for s.scanning { // Restart the rescan by killing the existing rescan. close(s.rescanQuit) rescan := s.rescan s.clientMtx.Unlock() rescan.WaitForShutdown() s.clientMtx.Lock() + // If the rescan has changed since unlocking, shut down the new + // one as well. + if s.rescan != rescan { + continue + } s.rescan = nil s.rescanErr = nil + break } s.rescanQuit = make(chan struct{}) s.scanning = true From a2ac8855d4b1d4831a3945c9913bf4fb17aa10fb Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Fri, 10 Apr 2020 11:44:07 -0700 Subject: [PATCH 2/6] chain/neutrino: remove unnecessary scanning variable The scanning variable is only ever set to true, and it happens in the same paths that the rescanQuit is initialized. Since rescanQuit is also only ever set to a non-nil value, we instead use this to determine whether we are "scanning". --- chain/neutrino.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/chain/neutrino.go b/chain/neutrino.go index c133aa2ba6..31fe5369ea 100644 --- a/chain/neutrino.go +++ b/chain/neutrino.go @@ -41,13 +41,16 @@ type NeutrinoClient struct { rescanErr <-chan error wg sync.WaitGroup started bool - scanning bool finished bool isRescan bool clientMtx sync.Mutex } +func (s *NeutrinoClient) isScanning() bool { + return s.rescanQuit != nil +} + // NewNeutrinoClient creates a new NeutrinoClient struct with a backing // ChainService. func NewNeutrinoClient(chainParams *chaincfg.Params, @@ -341,7 +344,7 @@ func (s *NeutrinoClient) Rescan(startHash *chainhash.Hash, addrs []btcutil.Addre return fmt.Errorf("can't do a rescan when the chain client " + "is not started") } - for s.scanning { + for s.isScanning() { // Restart the rescan by killing the existing rescan. close(s.rescanQuit) rescan := s.rescan @@ -358,7 +361,6 @@ func (s *NeutrinoClient) Rescan(startHash *chainhash.Hash, addrs []btcutil.Addre break } s.rescanQuit = make(chan struct{}) - s.scanning = true s.finished = false s.lastProgressSent = false s.lastFilteredBlockHeader = nil @@ -441,7 +443,7 @@ func (s *NeutrinoClient) NotifyBlocks() error { s.clientMtx.Lock() // If we're scanning, we're already notifying on blocks. Otherwise, // start a rescan without watching any addresses. - if !s.scanning { + if !s.isScanning() { s.clientMtx.Unlock() return s.NotifyReceived([]btcutil.Address{}) } @@ -455,13 +457,12 @@ func (s *NeutrinoClient) NotifyReceived(addrs []btcutil.Address) error { // If we have a rescan running, we just need to add the appropriate // addresses to the watch list. - if s.scanning { + if s.isScanning() { s.clientMtx.Unlock() return s.rescan.Update(neutrino.AddAddrs(addrs...)) } s.rescanQuit = make(chan struct{}) - s.scanning = true // Don't need RescanFinished or RescanProgress notifications. s.finished = true From 676f942dd81f266ff9d2cea1efc2c7f1e5a0cefe Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Fri, 10 Apr 2020 12:02:21 -0700 Subject: [PATCH 3/6] chain/neutrino: fix potential panic due to nil rescan The rescanQuit and rescan objects are set under different mutex acquisitions, hence it's possible for rescanQuit to be non-nil while rescan is nil. This commit ensures that we only try to wait for shutdown of non-nil rescans. --- chain/neutrino.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/chain/neutrino.go b/chain/neutrino.go index 31fe5369ea..649b43527e 100644 --- a/chain/neutrino.go +++ b/chain/neutrino.go @@ -348,9 +348,11 @@ func (s *NeutrinoClient) Rescan(startHash *chainhash.Hash, addrs []btcutil.Addre // Restart the rescan by killing the existing rescan. close(s.rescanQuit) rescan := s.rescan - s.clientMtx.Unlock() - rescan.WaitForShutdown() - s.clientMtx.Lock() + if s.rescan != nil { + s.clientMtx.Unlock() + rescan.WaitForShutdown() + s.clientMtx.Lock() + } // If the rescan has changed since unlocking, shut down the new // one as well. if s.rescan != rescan { From 5bc226462d1d67bb95ec6ec5b37835a62566734f Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Fri, 10 Apr 2020 12:03:45 -0700 Subject: [PATCH 4/6] chain/neutrino: make private notifyReceived This also fixes an unprotected access of s.rescan when calling Update without the lock held. --- chain/neutrino.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/chain/neutrino.go b/chain/neutrino.go index 649b43527e..a7694ffe42 100644 --- a/chain/neutrino.go +++ b/chain/neutrino.go @@ -456,11 +456,18 @@ func (s *NeutrinoClient) NotifyBlocks() error { // NotifyReceived replicates the RPC client's NotifyReceived command. func (s *NeutrinoClient) NotifyReceived(addrs []btcutil.Address) error { s.clientMtx.Lock() + defer s.clientMtx.Unlock() + + return s.notifyReceived(addrs) +} +// notifyReceived replicates the RPC client's NotifyReceived command. +// +// NOTE: The clienMtx MUST be held when invoking. +func (s *NeutrinoClient) notifyReceived(addrs []btcutil.Address) error { // If we have a rescan running, we just need to add the appropriate // addresses to the watch list. if s.isScanning() { - s.clientMtx.Unlock() return s.rescan.Update(neutrino.AddAddrs(addrs...)) } @@ -487,7 +494,6 @@ func (s *NeutrinoClient) NotifyReceived(addrs []btcutil.Address) error { ) s.rescan = newRescan s.rescanErr = s.rescan.Start() - s.clientMtx.Unlock() return nil } From 09647c5d4299941267b275619711518357746a22 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Fri, 10 Apr 2020 12:06:05 -0700 Subject: [PATCH 5/6] chain/neutrino: fix race in NotifyBlock Currently NotifyBlock releases the clientMtx before calling a public version of NotifyReceived that reacquires clientMtx. This can have unexpected behavior because the value of isScanning() could change between lock acquisitions. We switch to using the internal notifyReceived so that our read on isScanning() is consistent for the duration of the call. --- chain/neutrino.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/chain/neutrino.go b/chain/neutrino.go index a7694ffe42..2db01ce8bb 100644 --- a/chain/neutrino.go +++ b/chain/neutrino.go @@ -443,13 +443,13 @@ func (s *NeutrinoClient) Rescan(startHash *chainhash.Hash, addrs []btcutil.Addre // NotifyBlocks replicates the RPC client's NotifyBlocks command. func (s *NeutrinoClient) NotifyBlocks() error { s.clientMtx.Lock() + defer s.clientMtx.Unlock() + // If we're scanning, we're already notifying on blocks. Otherwise, // start a rescan without watching any addresses. if !s.isScanning() { - s.clientMtx.Unlock() - return s.NotifyReceived([]btcutil.Address{}) + return s.notifyReceived([]btcutil.Address{}) } - s.clientMtx.Unlock() return nil } From faf925f7c318b51dc7a473786815318d9db4f39e Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Fri, 10 Apr 2020 12:42:32 -0700 Subject: [PATCH 6/6] chain/neutrino: only acquire mutex once in Rescan happy flow --- chain/neutrino.go | 57 +++++++++++++++++++++++------------------------ 1 file changed, 28 insertions(+), 29 deletions(-) diff --git a/chain/neutrino.go b/chain/neutrino.go index 2db01ce8bb..6f095e3ebe 100644 --- a/chain/neutrino.go +++ b/chain/neutrino.go @@ -338,6 +338,29 @@ func (s *NeutrinoClient) pollCFilter(hash *chainhash.Hash) (*gcs.Filter, error) func (s *NeutrinoClient) Rescan(startHash *chainhash.Hash, addrs []btcutil.Address, outPoints map[wire.OutPoint]btcutil.Address) error { + bestBlock, err := s.CS.BestBlock() + if err != nil { + return fmt.Errorf("Can't get chain service's best block: %s", err) + } + header, err := s.CS.GetBlockHeader(&bestBlock.Hash) + if err != nil { + return fmt.Errorf("Can't get block header for hash %v: %s", + bestBlock.Hash, err) + } + + var inputsToWatch []neutrino.InputWithScript + for op, addr := range outPoints { + addrScript, err := txscript.PayToAddrScript(addr) + if err != nil { + return err + } + + inputsToWatch = append(inputsToWatch, neutrino.InputWithScript{ + OutPoint: op, + PkScript: addrScript, + }) + } + s.clientMtx.Lock() if !s.started { s.clientMtx.Unlock() @@ -367,23 +390,11 @@ func (s *NeutrinoClient) Rescan(startHash *chainhash.Hash, addrs []btcutil.Addre s.lastProgressSent = false s.lastFilteredBlockHeader = nil s.isRescan = true - s.clientMtx.Unlock() - - bestBlock, err := s.CS.BestBlock() - if err != nil { - return fmt.Errorf("Can't get chain service's best block: %s", err) - } - header, err := s.CS.GetBlockHeader(&bestBlock.Hash) - if err != nil { - return fmt.Errorf("Can't get block header for hash %v: %s", - bestBlock.Hash, err) - } // If the wallet is already fully caught up, or the rescan has started // with state that indicates a "fresh" wallet, we'll send a // notification indicating the rescan has "finished". if header.BlockHash() == *startHash { - s.clientMtx.Lock() s.finished = true rescanQuit := s.rescanQuit s.clientMtx.Unlock() @@ -391,12 +402,14 @@ func (s *NeutrinoClient) Rescan(startHash *chainhash.Hash, addrs []btcutil.Addre // Release the lock while dispatching the notification since // it's possible for the notificationHandler to be waiting to // acquire it before receiving the notification. - select { - case s.enqueueNotification <- &RescanFinished{ + ntfn := &RescanFinished{ Hash: startHash, Height: int32(bestBlock.Height), Time: header.Timestamp, - }: + } + select { + case s.enqueueNotification <- ntfn: + s.clientMtx.Lock() case <-s.quit: return nil case <-rescanQuit: @@ -404,20 +417,6 @@ func (s *NeutrinoClient) Rescan(startHash *chainhash.Hash, addrs []btcutil.Addre } } - var inputsToWatch []neutrino.InputWithScript - for op, addr := range outPoints { - addrScript, err := txscript.PayToAddrScript(addr) - if err != nil { - return err - } - - inputsToWatch = append(inputsToWatch, neutrino.InputWithScript{ - OutPoint: op, - PkScript: addrScript, - }) - } - - s.clientMtx.Lock() newRescan := neutrino.NewRescan( &neutrino.RescanChainSource{ ChainService: s.CS,