From 02c1264c536f1461da7a324f95981d0eefdea76c Mon Sep 17 00:00:00 2001 From: ziggie Date: Sat, 27 Jul 2024 14:39:46 +0200 Subject: [PATCH] multi: prevent nil panics in stop methods. With this PR we might call the stop method even when the start method of a subsystem did not successfully finish therefore we need to make sure we guard the stop methods for potential panics if some variables are not initialized in the contructors of the subsystems. --- chainntnfs/bitcoindnotify/bitcoind.go | 7 ++++++- chainntnfs/neutrinonotify/neutrino.go | 7 ++++++- chanfitness/chaneventstore.go | 10 ++++++++-- discovery/gossiper.go | 7 ++++++- htlcswitch/interceptable_switch.go | 6 +++++- htlcswitch/link.go | 14 +++++++++----- invoices/invoiceregistry.go | 10 ++++++++-- lnwallet/chainfee/estimator.go | 4 +++- server.go | 3 +++ tor/controller.go | 4 ++++ 10 files changed, 58 insertions(+), 14 deletions(-) diff --git a/chainntnfs/bitcoindnotify/bitcoind.go b/chainntnfs/bitcoindnotify/bitcoind.go index 2bffefdbe..f0547e4df 100644 --- a/chainntnfs/bitcoindnotify/bitcoind.go +++ b/chainntnfs/bitcoindnotify/bitcoind.go @@ -151,7 +151,12 @@ func (b *BitcoindNotifier) Stop() error { close(epochClient.epochChan) } - b.txNotifier.TearDown() + + // The txNotifier is only initialized in the start method therefore we + // need to make sure we don't access a nil pointer here. + if b.txNotifier != nil { + b.txNotifier.TearDown() + } // Stop the mempool notifier. b.memNotifier.TearDown() diff --git a/chainntnfs/neutrinonotify/neutrino.go b/chainntnfs/neutrinonotify/neutrino.go index 80e210c2f..55d923577 100644 --- a/chainntnfs/neutrinonotify/neutrino.go +++ b/chainntnfs/neutrinonotify/neutrino.go @@ -152,7 +152,12 @@ func (n *NeutrinoNotifier) Stop() error { close(epochClient.epochChan) } - n.txNotifier.TearDown() + + // The txNotifier is only initialized in the start method therefore we + // need to make sure we don't access a nil pointer here. + if n.txNotifier != nil { + n.txNotifier.TearDown() + } return nil } diff --git a/chanfitness/chaneventstore.go b/chanfitness/chaneventstore.go index 4b1dc1ddd..29a1df917 100644 --- a/chanfitness/chaneventstore.go +++ b/chanfitness/chaneventstore.go @@ -226,11 +226,17 @@ func (c *ChannelEventStore) Stop() error { // Stop the ticker after the goroutine reading from it has exited, to // avoid a race. - c.cfg.FlapCountTicker.Stop() + var err error + if c.cfg.FlapCountTicker == nil { + err = fmt.Errorf("ChannelEventStore FlapCountTicker not " + + "initialized") + } else { + c.cfg.FlapCountTicker.Stop() + } log.Debugf("ChannelEventStore shutdown complete") - return nil + return err } // addChannel checks whether we are already tracking a channel's peer, creates a diff --git a/discovery/gossiper.go b/discovery/gossiper.go index 53bfd38c2..bb0aa652c 100644 --- a/discovery/gossiper.go +++ b/discovery/gossiper.go @@ -753,7 +753,12 @@ func (d *AuthenticatedGossiper) stop() { log.Debug("Authenticated Gossiper is stopping") defer log.Debug("Authenticated Gossiper stopped") - d.blockEpochs.Cancel() + // `blockEpochs` is only initialized in the start routine so we make + // sure we don't panic here in the case where the `Stop` method is + // called when the `Start` method does not complete. + if d.blockEpochs != nil { + d.blockEpochs.Cancel() + } d.syncMgr.Stop() diff --git a/htlcswitch/interceptable_switch.go b/htlcswitch/interceptable_switch.go index 144af814a..0ac19a36c 100644 --- a/htlcswitch/interceptable_switch.go +++ b/htlcswitch/interceptable_switch.go @@ -242,7 +242,11 @@ func (s *InterceptableSwitch) Stop() error { close(s.quit) s.wg.Wait() - s.blockEpochStream.Cancel() + // We need to check whether the start routine run and initialized the + // `blockEpochStream`. + if s.blockEpochStream != nil { + s.blockEpochStream.Cancel() + } log.Debug("InterceptableSwitch shutdown complete") diff --git a/htlcswitch/link.go b/htlcswitch/link.go index f39a12b2b..897449a38 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -562,14 +562,18 @@ func (l *channelLink) Stop() { } // Ensure the channel for the timer is drained. - if !l.updateFeeTimer.Stop() { - select { - case <-l.updateFeeTimer.C: - default: + if l.updateFeeTimer != nil { + if !l.updateFeeTimer.Stop() { + select { + case <-l.updateFeeTimer.C: + default: + } } } - l.hodlQueue.Stop() + if l.hodlQueue != nil { + l.hodlQueue.Stop() + } close(l.quit) l.wg.Wait() diff --git a/invoices/invoiceregistry.go b/invoices/invoiceregistry.go index c7f845b27..472c55048 100644 --- a/invoices/invoiceregistry.go +++ b/invoices/invoiceregistry.go @@ -261,7 +261,13 @@ func (i *InvoiceRegistry) Stop() error { log.Info("InvoiceRegistry shutting down...") defer log.Debug("InvoiceRegistry shutdown complete") - i.expiryWatcher.Stop() + var err error + if i.expiryWatcher == nil { + err = fmt.Errorf("InvoiceRegistry expiryWatcher is not " + + "initialized") + } else { + i.expiryWatcher.Stop() + } close(i.quit) @@ -269,7 +275,7 @@ func (i *InvoiceRegistry) Stop() error { log.Debug("InvoiceRegistry shutdown complete") - return nil + return err } // invoiceEvent represents a new event that has modified on invoice on disk. diff --git a/lnwallet/chainfee/estimator.go b/lnwallet/chainfee/estimator.go index d9a402964..6f59c3f7f 100644 --- a/lnwallet/chainfee/estimator.go +++ b/lnwallet/chainfee/estimator.go @@ -884,7 +884,9 @@ func (w *WebAPIEstimator) Stop() error { return nil } - w.updateFeeTicker.Stop() + if w.updateFeeTicker != nil { + w.updateFeeTicker.Stop() + } close(w.quit) w.wg.Wait() diff --git a/server.go b/server.go index 4fb719518..1569224f6 100644 --- a/server.go +++ b/server.go @@ -2105,6 +2105,9 @@ func (s *server) Start() error { } } + // chanSubSwapper must be started after the `channelNotifier` + // because it depends on channel events as a synchronization + // point. cleanup = cleanup.add(s.chanSubSwapper.Stop) if err := s.chanSubSwapper.Start(); err != nil { startErr = err diff --git a/tor/controller.go b/tor/controller.go index 47ea6e129..f997a8697 100644 --- a/tor/controller.go +++ b/tor/controller.go @@ -186,6 +186,10 @@ func (c *Controller) Stop() error { // Reset service ID. c.activeServiceID = "" + if c.conn == nil { + return fmt.Errorf("no connection available to the tor server") + } + return c.conn.Close() }