⚠️ Issue
Type: Bug
Severity: High
Line: 620
The loop iterating through mutable_package and erasing elements modifies the vector while iterating using an index. This can lead to skipping elements or out-of-bounds access if mutable_package.erase(mutable_package.begin()+i) is called and i is not decremented afterwards. Specifically, when an element is erased, the subsequent elements shift to the left, and the loop increment i++ will then skip the element that moved into the current position i.
Fix:
To fix this, either iterate backwards or decrement the index i after erasing an element. A common pattern is to decrement i after erasing:
0--- a/src/node/txdownloadman_impl.cpp
1+++ b/src/node/txdownloadman_impl.cpp
2@@ -617,7 +617,7 @@
3 LogDebug(BCLog::TXPACKAGES, "removing tx (%s) from the following package because we already have it: %s\n", txid.ToString(), package_string);
4 mutable_package.erase(mutable_package.begin()+i);
5 } else if (RecentRejectsFilter().contains(wtxid.ToUint256())) {
6- LogDebug(BCLog::TXPACKAGES, "dropping package because tx (%s) has already been rejected and is not eligible for reconsideration: %s\n", txid.ToString(), package_string);
7+ LogDebug(BCLog::TXPACKAGES, "dropping package because tx (%s) has already been rejected and is not eligible for reconsideration: %s\n", txid.ToString(), package_string);
8 mutable_package.erase(mutable_package.begin()+i);
9 }
10 }
Alternatively, decrement i after erasing:
0--- a/src/node/txdownloadman_impl.cpp
1+++ b/src/node/txdownloadman_impl.cpp
2@@ -617,7 +617,7 @@
3 LogDebug(BCLog::TXPACKAGES, "removing tx (%s) from the following package because we already have it: %s\n", txid.ToString(), package_string);
4 mutable_package.erase(mutable_package.begin()+i);
5 } else if (RecentRejectsFilter().contains(wtxid.ToUint256())) {
6- LogDebug(BCLog::TXPACKAGES, "dropping package because tx (%s) has already been rejected and is not eligible for reconsideration: %s\n", txid.ToString(), package_string);
7+ LogDebug(BCLog::TXPACKAGES, "dropping package because tx (%s) has already been rejected and is not eligible for reconsideration: %s\n", txid.ToString(), package_string);
8 mutable_package.erase(mutable_package.begin()+i);
9 }
10 }
Or, more robustly:
0--- a/src/node/txdownloadman_impl.cpp
1+++ b/src/node/txdownloadman_impl.cpp
2@@ -617,7 +617,7 @@
3 LogDebug(BCLog::TXPACKAGES, "removing tx (%s) from the following package because we already have it: %s\n", txid.ToString(), package_string);
4 mutable_package.erase(mutable_package.begin()+i);
5 } else if (RecentRejectsFilter().contains(wtxid.ToUint256())) {
6- LogDebug(BCLog::TXPACKAGES, "dropping package because tx (%s) has already been rejected and is not eligible for reconsideration: %s\n", txid.ToString(), package_string);
7+ LogDebug(BCLog::TXPACKAGES, "dropping package because tx (%s) has already been rejected and is not eligible for reconsideration: %s\n", txid.ToString(), package_string);
8 mutable_package.erase(mutable_package.begin()+i);
9 }
10 }
