From de80fffa6c6f39cbc44750e48aa7c12eed255cda Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Thu, 30 Mar 2023 12:29:06 +0200 Subject: [PATCH 1/5] wtclient: divide addr_iter tests into subtests Split out the existing tests into logical subtests. --- watchtower/wtclient/addr_iterator_test.go | 318 ++++++++++++---------- 1 file changed, 172 insertions(+), 146 deletions(-) diff --git a/watchtower/wtclient/addr_iterator_test.go b/watchtower/wtclient/addr_iterator_test.go index 89a35c4cb..1e58471dc 100644 --- a/watchtower/wtclient/addr_iterator_test.go +++ b/watchtower/wtclient/addr_iterator_test.go @@ -10,184 +10,210 @@ import ( // TestAddrIterator tests the behaviour of the addressIterator. func TestAddrIterator(t *testing.T) { - // Assert that an iterator can't be initialised with an empty address - // list. - _, err := newAddressIterator() - require.ErrorContains(t, err, "must have at least one address") + t.Parallel() + // Prep some addresses to use in the subtests. addr1, err := net.ResolveTCPAddr("tcp", "1.2.3.4:8000") require.NoError(t, err) - // Initialise the iterator with addr1. - iter, err := newAddressIterator(addr1) - require.NoError(t, err) - - // Attempting to remove addr1 should fail now since it is the only - // address in the iterator. - iter.Add(addr1) - err = iter.Remove(addr1) - require.ErrorIs(t, err, wtdb.ErrLastTowerAddr) - - // Adding a duplicate of addr1 and then calling Remove should still - // return an error. - err = iter.Remove(addr1) - require.ErrorIs(t, err, wtdb.ErrLastTowerAddr) - addr2, err := net.ResolveTCPAddr("tcp", "1.2.3.4:8001") require.NoError(t, err) - // Add addr2 to the iterator. - iter.Add(addr2) - - // Check that peek returns addr1. - a1 := iter.Peek() - require.NoError(t, err) - require.Equal(t, addr1, a1) - - // Calling peek multiple times should return the same result. - a1 = iter.Peek() - require.Equal(t, addr1, a1) - - // Calling Next should now return addr2. - a2, err := iter.Next() - require.NoError(t, err) - require.Equal(t, addr2, a2) - - // Assert that Peek now returns addr2. - a2 = iter.Peek() - require.NoError(t, err) - require.Equal(t, addr2, a2) - - // Calling Next should result in reaching the end of th list. - _, err = iter.Next() - require.ErrorIs(t, err, ErrAddressesExhausted) - - // Calling Peek now should reset the queue and return addr1. - a1 = iter.Peek() - require.Equal(t, addr1, a1) - - // Wind the list to the end again so that we can test the Reset func. - _, err = iter.Next() - require.NoError(t, err) - - _, err = iter.Next() - require.ErrorIs(t, err, ErrAddressesExhausted) - - iter.Reset() - - // Now Next should return addr 2. - a2, err = iter.Next() - require.NoError(t, err) - require.Equal(t, addr2, a2) - addr3, err := net.ResolveTCPAddr("tcp", "1.2.3.4:8002") require.NoError(t, err) - // Add addr3 now to ensure that the iteration works even if we are - // midway through the queue. - iter.Add(addr3) + t.Run("always at least one address", func(t *testing.T) { + t.Parallel() - // Now Next should return addr 3. - a3, err := iter.Next() - require.NoError(t, err) - require.Equal(t, addr3, a3) + // Assert that an iterator can't be initialised with an empty + // address list. + _, err := newAddressIterator() + require.ErrorContains(t, err, "must have at least one address") - // Quickly test that GetAll correctly returns a copy of all the - // addresses in the iterator. - addrList := iter.GetAll() - require.ElementsMatch(t, addrList, []net.Addr{addr1, addr2, addr3}) + // Initialise the iterator with addr1. + iter, err := newAddressIterator(addr1) + require.NoError(t, err) - // Also check that an iterator constructed via the Copy method, also - // contains all the expected addresses. - newIterAddrs := iter.Copy().GetAll() - require.ElementsMatch(t, newIterAddrs, []net.Addr{addr1, addr2, addr3}) + // Attempting to remove addr1 should fail now since it is the + // only address in the iterator. + iter.Add(addr1) + err = iter.Remove(addr1) + require.ErrorIs(t, err, wtdb.ErrLastTowerAddr) - // Let's now remove addr3. - err = iter.Remove(addr3) - require.NoError(t, err) + // Adding a duplicate of addr1 and then calling Remove should + // still return an error. + err = iter.Remove(addr1) + require.ErrorIs(t, err, wtdb.ErrLastTowerAddr) + }) - // Since addr3 is gone, Peek should return addr1. - a1 = iter.Peek() - require.Equal(t, addr1, a1) + t.Run("address iteration", func(t *testing.T) { + t.Parallel() - // Lastly, we will test the "locking" of addresses. + // Initialise the iterator with addr1. + iter, err := newAddressIterator(addr1) + require.NoError(t, err) - // First we test the locking of an address via the PeekAndLock function. - a1 = iter.PeekAndLock() - require.Equal(t, addr1, a1) - require.True(t, iter.HasLocked()) + // Add addr2 to the iterator. + iter.Add(addr2) - // Assert that we can't remove addr1 if there is a lock on it. - err = iter.Remove(addr1) - require.ErrorIs(t, err, ErrAddrInUse) + // Check that peek returns addr1. + a1 := iter.Peek() + require.NoError(t, err) + require.Equal(t, addr1, a1) - // Now release the lock on addr1. - iter.ReleaseLock(addr1) - require.False(t, iter.HasLocked()) + // Calling peek multiple times should return the same result. + a1 = iter.Peek() + require.Equal(t, addr1, a1) - // Since the lock has been released, we should now be able to remove - // addr1. - err = iter.Remove(addr1) - require.NoError(t, err) + // Calling Next should now return addr2. + a2, err := iter.Next() + require.NoError(t, err) + require.Equal(t, addr2, a2) - // Now we test the locking of an address via the NextAndLock function. - // To do this, we first re-add addr3. - iter.Add(addr3) + // Assert that Peek now returns addr2. + a2 = iter.Peek() + require.NoError(t, err) + require.Equal(t, addr2, a2) - a2, err = iter.NextAndLock() - require.NoError(t, err) - require.Equal(t, addr2, a2) - require.True(t, iter.HasLocked()) + // Calling Next should result in reaching the end of th list. + _, err = iter.Next() + require.ErrorIs(t, err, ErrAddressesExhausted) - // Assert that we can't remove addr2 if there is a lock on it. - err = iter.Remove(addr2) - require.ErrorIs(t, err, ErrAddrInUse) + // Calling Peek now should reset the queue and return addr1. + a1 = iter.Peek() + require.Equal(t, addr1, a1) - // Now release the lock on addr2. - iter.ReleaseLock(addr2) - require.False(t, iter.HasLocked()) + // Wind the list to the end again so that we can test the Reset + // func. + _, err = iter.Next() + require.NoError(t, err) - // Since the lock has been released, we should now be able to remove - // addr1. - err = iter.Remove(addr2) - require.NoError(t, err) + _, err = iter.Next() + require.ErrorIs(t, err, ErrAddressesExhausted) - // Only addr3 should still be left in the iterator. - addrList = iter.GetAll() - require.Len(t, addrList, 1) - require.Contains(t, addrList, addr3) + iter.Reset() - // Ensure that HasLocked acts correctly in the case where more than one - // address is being locked and unlock as well as the case where the same - // address is locked more than once. + // Now Next should return addr 2. + a2, err = iter.Next() + require.NoError(t, err) + require.Equal(t, addr2, a2) - require.False(t, iter.HasLocked()) + // Add addr3 now to ensure that the iteration works even if we + // are midway through the queue. + iter.Add(addr3) - a3 = iter.PeekAndLock() - require.Equal(t, addr3, a3) - require.True(t, iter.HasLocked()) + // Now Next should return addr 3. + a3, err := iter.Next() + require.NoError(t, err) + require.Equal(t, addr3, a3) - a3 = iter.PeekAndLock() - require.Equal(t, addr3, a3) - require.True(t, iter.HasLocked()) + // Quickly test that GetAll correctly returns a copy of all the + // addresses in the iterator. + addrList := iter.GetAll() + require.ElementsMatch( + t, addrList, []net.Addr{addr1, addr2, addr3}, + ) - iter.Add(addr2) - a2, err = iter.NextAndLock() - require.NoError(t, err) - require.Equal(t, addr2, a2) - require.True(t, iter.HasLocked()) + // Also check that an iterator constructed via the Copy method, + // also contains all the expected addresses. + newIterAddrs := iter.Copy().GetAll() + require.ElementsMatch( + t, newIterAddrs, []net.Addr{addr1, addr2, addr3}, + ) - // Now release addr2 and asset that HasLock is still true. - iter.ReleaseLock(addr2) - require.True(t, iter.HasLocked()) + // Let's now remove addr3. + err = iter.Remove(addr3) + require.NoError(t, err) - // Releasing one of the locks on addr3 now should still result in - // HasLocked returning true. - iter.ReleaseLock(addr3) - require.True(t, iter.HasLocked()) + // Since addr3 is gone, Peek should return addr1. + a1 = iter.Peek() + require.Equal(t, addr1, a1) + }) - // Releasing it again should now result in should still result in - // HasLocked returning false. - iter.ReleaseLock(addr3) - require.False(t, iter.HasLocked()) + t.Run("address locking", func(t *testing.T) { + t.Parallel() + + // Initialise the iterator with addr1 and addr2. + iter, err := newAddressIterator(addr1, addr2) + require.NoError(t, err) + + // First we test the locking of an address via the PeekAndLock + // function. + a1 := iter.PeekAndLock() + require.Equal(t, addr1, a1) + require.True(t, iter.HasLocked()) + + // Assert that we can't remove addr1 if there is a lock on it. + err = iter.Remove(addr1) + require.ErrorIs(t, err, ErrAddrInUse) + + // Now release the lock on addr1. + iter.ReleaseLock(addr1) + require.False(t, iter.HasLocked()) + + // Since the lock has been released, we should now be able to + // remove addr1. + err = iter.Remove(addr1) + require.NoError(t, err) + + // Now we test the locking of an address via the NextAndLock + // function. To do this, we first re-add addr3. + iter.Add(addr3) + + a2, err := iter.NextAndLock() + require.NoError(t, err) + require.Equal(t, addr2, a2) + require.True(t, iter.HasLocked()) + + // Assert that we can't remove addr2 if there is a lock on it. + err = iter.Remove(addr2) + require.ErrorIs(t, err, ErrAddrInUse) + + // Now release the lock on addr2. + iter.ReleaseLock(addr2) + require.False(t, iter.HasLocked()) + + // Since the lock has been released, we should now be able to + // remove addr1. + err = iter.Remove(addr2) + require.NoError(t, err) + + // Only addr3 should still be left in the iterator. + addrList := iter.GetAll() + require.Len(t, addrList, 1) + require.Contains(t, addrList, addr3) + + // Ensure that HasLocked acts correctly in the case where more + // than one address is being locked and unlock as well as the + // case where the same address is locked more than once. + require.False(t, iter.HasLocked()) + + a3 := iter.PeekAndLock() + require.Equal(t, addr3, a3) + require.True(t, iter.HasLocked()) + + a3 = iter.PeekAndLock() + require.Equal(t, addr3, a3) + require.True(t, iter.HasLocked()) + + iter.Add(addr2) + a2, err = iter.NextAndLock() + require.NoError(t, err) + require.Equal(t, addr2, a2) + require.True(t, iter.HasLocked()) + + // Now release addr2 and assert that HasLock is still true. + iter.ReleaseLock(addr2) + require.True(t, iter.HasLocked()) + + // Releasing one of the locks on addr3 now should still result + // in HasLocked returning true. + iter.ReleaseLock(addr3) + require.True(t, iter.HasLocked()) + + // Releasing it again should now result in should still result + // in HasLocked returning false. + iter.ReleaseLock(addr3) + require.False(t, iter.HasLocked()) + }) } From 104751451579eae998195a658a58381decf464f6 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Thu, 30 Mar 2023 11:45:34 +0200 Subject: [PATCH 2/5] wtclient: demo addr iterator panic This commit adds a test that shows that it is possible to cause the AddressIterator to panic if the `Next` method is ever called twice when the iterator is at the end of its list without Reset first being called. --- watchtower/wtclient/addr_iterator_test.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/watchtower/wtclient/addr_iterator_test.go b/watchtower/wtclient/addr_iterator_test.go index 1e58471dc..436be2a7e 100644 --- a/watchtower/wtclient/addr_iterator_test.go +++ b/watchtower/wtclient/addr_iterator_test.go @@ -216,4 +216,27 @@ func TestAddrIterator(t *testing.T) { iter.ReleaseLock(addr3) require.False(t, iter.HasLocked()) }) + + t.Run("calling Next twice without Reset panics", func(t *testing.T) { + t.Parallel() + + // This defer-function asserts that a panic does occur. + defer func() { + r := recover() + require.NotNilf(t, r, "the code did not panic") + }() + + // Initialise the iterator with addr1. + iter, err := newAddressIterator(addr1) + require.NoError(t, err) + + a1 := iter.Peek() + require.Equal(t, addr1, a1) + + _, err = iter.Next() + require.ErrorIs(t, err, ErrAddressesExhausted) + + _, err = iter.Next() + require.ErrorIs(t, err, ErrAddressesExhausted) + }) } From 9e4c8dd5090666f008361390d19a9c2bc8ecba17 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Thu, 30 Mar 2023 11:49:58 +0200 Subject: [PATCH 3/5] wtclient: make addr iterator panic safe Ensure that calling Next twice in a row without first calling Reset is safe when the iterator is at the end of its list. Also alter the towerListIterator to call Reset after hitting an error on Next. --- watchtower/wtclient/addr_iterator.go | 4 ++++ watchtower/wtclient/addr_iterator_test.go | 8 +------- watchtower/wtclient/candidate_iterator.go | 1 + 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/watchtower/wtclient/addr_iterator.go b/watchtower/wtclient/addr_iterator.go index cb16d335a..8abcb3891 100644 --- a/watchtower/wtclient/addr_iterator.go +++ b/watchtower/wtclient/addr_iterator.go @@ -162,6 +162,10 @@ func (a *addressIterator) next(lock bool) (net.Addr, error) { a.mu.Lock() defer a.mu.Unlock() + if a.currentTopAddr == nil { + return nil, ErrAddressesExhausted + } + // Set the next candidate to the subsequent element. a.currentTopAddr = a.currentTopAddr.Next() diff --git a/watchtower/wtclient/addr_iterator_test.go b/watchtower/wtclient/addr_iterator_test.go index 436be2a7e..a2e49ff90 100644 --- a/watchtower/wtclient/addr_iterator_test.go +++ b/watchtower/wtclient/addr_iterator_test.go @@ -217,15 +217,9 @@ func TestAddrIterator(t *testing.T) { require.False(t, iter.HasLocked()) }) - t.Run("calling Next twice without Reset panics", func(t *testing.T) { + t.Run("calling Next twice without Reset is safe", func(t *testing.T) { t.Parallel() - // This defer-function asserts that a panic does occur. - defer func() { - r := recover() - require.NotNilf(t, r, "the code did not panic") - }() - // Initialise the iterator with addr1. iter, err := newAddressIterator(addr1) require.NoError(t, err) diff --git a/watchtower/wtclient/candidate_iterator.go b/watchtower/wtclient/candidate_iterator.go index 10ef86465..29c4719f0 100644 --- a/watchtower/wtclient/candidate_iterator.go +++ b/watchtower/wtclient/candidate_iterator.go @@ -145,6 +145,7 @@ func (t *towerListIterator) AddCandidate(candidate *Tower) { for { next, err := candidate.Addresses.Next() if err != nil { + candidate.Addresses.Reset() break } From 4f7e871b42b5f359de2bc70163dcd53e708a39f2 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Thu, 30 Mar 2023 12:00:52 +0200 Subject: [PATCH 4/5] wtclient: add clarifying comments to the AddressIterator --- watchtower/wtclient/addr_iterator.go | 39 +++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/watchtower/wtclient/addr_iterator.go b/watchtower/wtclient/addr_iterator.go index 8abcb3891..5ada7d5de 100644 --- a/watchtower/wtclient/addr_iterator.go +++ b/watchtower/wtclient/addr_iterator.go @@ -162,11 +162,15 @@ func (a *addressIterator) next(lock bool) (net.Addr, error) { a.mu.Lock() defer a.mu.Unlock() + // In-case currentTopAddr is nil (meaning that Reset has not yet been + // called), return an error indicating this. if a.currentTopAddr == nil { return nil, ErrAddressesExhausted } - // Set the next candidate to the subsequent element. + // Set the next candidate to the subsequent element. If we are at the + // end of the address list, this could mean setting currentTopAddr to + // nil. a.currentTopAddr = a.currentTopAddr.Next() for a.currentTopAddr != nil { @@ -176,9 +180,26 @@ func (a *addressIterator) next(lock bool) (net.Addr, error) { // Check whether this address is still considered a candidate. // If it's not, we'll proceed to the next. candidate, ok := a.candidates[addrID] + + // If the address cannot be found in the candidate set, then + // this must mean that the Remove method was called for the + // address. The Remove method would have checked that the + // address is not the last one in the iterator and that it has + // no locks on it. It is therefor safe to remove. if !ok { + // Grab the next address candidate. This might be nil + // if the iterator is on the last item in the list. nextCandidate := a.currentTopAddr.Next() + + // Remove the address from the list that is no longer + // in the candidate set. a.addrList.Remove(a.currentTopAddr) + + // Set the current top to the next candidate. This might + // mean setting it to nil if the iterator is on its last + // item in which case the loop will be exited and an + // ErrAddressesExhausted exhausted error will be + // returned. a.currentTopAddr = nextCandidate continue } @@ -234,9 +255,25 @@ func (a *addressIterator) peek(lock bool) net.Addr { addrID := a.currentTopAddr.Value.(string) candidate, ok := a.candidates[addrID] + + // If the address cannot be found in the candidate set, then + // this must mean that the Remove method was called for the + // address. The Remove method would have checked that the + // address is not the last one in the iterator and that it has + // no locks on it. It is therefor safe to remove. if !ok { + // Grab the next address candidate. This might be nil + // if the iterator is on the last item in the list. nextCandidate := a.currentTopAddr.Next() + + // Remove the address from the list that is no longer + // in the candidate set. a.addrList.Remove(a.currentTopAddr) + + // Set the current top to the next candidate. This might + // mean setting it to nil if the iterator is on its last + // item but this will be reset at the top of the for + // loop. a.currentTopAddr = nextCandidate continue } From 82afb6a958fe7f1314da0068f346dd21206fe85c Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Thu, 30 Mar 2023 12:36:53 +0200 Subject: [PATCH 5/5] docs: add release note for 7556 --- docs/release-notes/release-notes-0.16.1.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/release-notes/release-notes-0.16.1.md b/docs/release-notes/release-notes-0.16.1.md index 6b6cd1346..790388eed 100644 --- a/docs/release-notes/release-notes-0.16.1.md +++ b/docs/release-notes/release-notes-0.16.1.md @@ -14,6 +14,8 @@ ## Watchtowers +* [Fix Address iterator + panic](https://github.com/lightningnetwork/lnd/pull/7556) * [Allow caller to filter sessions at the time of reading them from disk](https://github.com/lightningnetwork/lnd/pull/7059) * [Clean up sessions once all channels for which they have updates for are