Fix crash inside OverlayImpl loops over ids_

[Bronek]

High Level Overview of Change

Functions OverlayImpl::getActivePeers and OverlayImpl::findPeerByPublicKey may cause a crash when iterating over ids_

Context of Change

Functions OverlayImpl::getActivePeers and OverlayImpl::findPeerByPublicKey use the following buggy pattern to iterate over ids_ :

for (auto& [id, w] : ids_)
{
    if (auto p = w.lock())
    {
        // ...
    } // p destroyed here
}

The problem is that, given specific thread scheduling, when p is destroyed at a marked location, it might also cause the destruction of the PeerImp that it owned since the start of if (auto p = w.lock()) block. This destruction in turn will call OverlayImpl::onPeerDeactivate which will remove the current element from ids_ collection, hence invalidating the iterator to the removed element. This is all happening while p is being destroyed, that is before the for at the start of the loop tries to increment the iterator (that just got invalidated) to reach to the next element. Incrementing the invalidated operator is an invalid operation, in practice it will yield bad data, causing a crash.

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Refactor (non-breaking change that only restructures code)
• Performance (increase or change in throughput and/or latency)
• Tests (you added tests for code that already exists, or your new feature included in this PR)
• Documentation update
• Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
• Release

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.3%. Comparing base (e5aa605) to head (c3deeff).
Report is 22 commits behind head on develop.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5071     +/-   ##
=========================================
- Coverage     71.4%   71.3%   -0.0%     
=========================================
  Files          796     796             
  Lines        67073   67075      +2     
  Branches     10866   10881     +15     
=========================================
- Hits         47881   47854     -27     
- Misses       19192   19221     +29     

Files with missing lines	Coverage Δ
src/xrpld/overlay/detail/OverlayImpl.cpp	29.8% <100.0%> (+1.1%)	⬆️

... and 8 files with indirect coverage changes

[Bronek]

Here's a smaller reproduction of the bug https://godbolt.org/z/9rGsx9eEP and of the proposed fix https://godbolt.org/z/qPbc9oYM9

[ximinez]

This looks good.

I did a little bit of a search, and came across a few more places that at a glance could have a non-zero chance of having the same problem, and could probably be quickly and easily fixed with this same pattern. Would they be worth changing in the PR, or another one?

• https://github.com/ximinez/rippled/blob/1e3ab9f76615cf9bb74b7bfeb6d2bd1cd257fe33/include/xrpl/server/detail/io_list.h#L246
• https://github.com/ximinez/rippled/blob/1e3ab9f76615cf9bb74b7bfeb6d2bd1cd257fe33/src/xrpld/app/ledger/detail/InboundLedger.cpp#L1311
• https://github.com/ximinez/rippled/blob/1e3ab9f76615cf9bb74b7bfeb6d2bd1cd257fe33/src/xrpld/app/ledger/detail/LedgerReplayer.cpp#L274 (because the lambda is used in a call to std::for_each)

[thejohnfreeman]

@ximinez those first two are fine because the collection being iterated is held in a local variable, and the destructor of the objects in the collection cannot remove that object from the collection. I don't think the third is in danger, either, but I haven't looked very deeply.

[Bronek]

@ximinez those first two are fine because the collection being iterated is held in a local variable, and the destructor of the objects in the collection cannot remove that object from the collection. I don't think the third is in danger, either, but I haven't looked very deeply.

Yes, the first two operate on a stack-based collection within the scope of the function, so these are not a problem. The last one is not a problem either, because both collections deltas_ and skipLists_ operate on objects which do not remove themselves from a collection inside their respective destructors :
https://github.com/ximinez/rippled/blob/1e3ab9f76615cf9bb74b7bfeb6d2bd1cd257fe33/src/xrpld/app/ledger/detail/SkipListAcquire.cpp#L48
https://github.com/ximinez/rippled/blob/1e3ab9f76615cf9bb74b7bfeb6d2bd1cd257fe33/src/xrpld/app/ledger/detail/LedgerDeltaAcquire.cpp#L52

Their base class TimeoutCounter doesn't do anything like that either

https://github.com/ximinez/rippled/blob/1e3ab9f76615cf9bb74b7bfeb6d2bd1cd257fe33/src/xrpld/app/ledger/detail/TimeoutCounter.cpp#L118
https://github.com/ximinez/rippled/blob/1e3ab9f76615cf9bb74b7bfeb6d2bd1cd257fe33/src/xrpld/app/ledger/detail/TimeoutCounter.h#L97

[ximinez]

The last one is not a problem either, because both collections deltas_ and skipLists_ operate on objects which do not remove themselves from a collection inside their respective destructors

I am thinking a little bit about future-proofing. This seems like a good pattern to use in general just in case the behavior of these objects changes in the future. However, that is definitely beyond the scope of this PR.