wallet2: validate account tags before mutation
What the developer wrote
Authored by Samy
wallet2: validate account tags before mutation
What changed, and why it matters
This commit fixes a bug in the Monero wallet where it would partially change account tags even when some requested account numbers did not exist. Previously, the wallet checked each account number one at a time and applied the tag immediately, so an invalid account number appearing after a valid one could leave tags changed. Now all account numbers are checked first, and only if they are all valid are any tags changed. The included test confirms that an out-of-bounds account index now causes an error and leaves existing tags untouched.
Review related wallet mutation paths for similar validate-and-mutate-in-one-loop patterns, especially where std::set or ordered iteration could allow partial updates before an exception. Consider whether untag_accounts and other tag operations share the same code path and are covered by the same fix.
Security signals we found
Atomicity violation in state mutation
Partial state update on error path
Input validation moved before mutation
Functional test added for out-of-bounds handling
Evidence from the diff
In wallet2::set_account_tag(), the original loop both validated account_index and mutated m_account_tags.second[account_index] in the same pass. Because std::set iteration is ordered, a valid low index would be processed before an invalid high index was detected, leaving the tag mutation persisted. The patch splits this into two loops: the first validates every account index against get_num_subaddress_accounts(), and the second performs mutations only after validation succeeds. A functional test is added to assert that tagging/untagging with an out-of-bounds index [0, 3] throws ‘Account index out of bound’ and does not alter the existing tag state.
Changed components
src/wallet/wallet2.cppwallet2::set_account_tag()tests/functional_tests/wallet.pyInspect captured patch +24 / −0
diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp
index 86eba93..ba759e9 100644
--- a/src/wallet/wallet2.cpp
+++ b/src/wallet/wallet2.cpp
@@ -12873,6 +12873,10 @@ void wallet2::set_account_tag(const std::set<uint32_t> &account_indices, const s
for (uint32_t account_index : account_indices)
{
THROW_WALLET_EXCEPTION_IF(account_index >= get_num_subaddress_accounts(), error::wallet_internal_error, "Account index out of bound");
+ }
+
+ for (uint32_t account_index : account_indices)
+ {
if (m_account_tags.second[account_index] == tag)
MDEBUG("This tag is already assigned to this account");
else
diff --git a/tests/functional_tests/wallet.py b/tests/functional_tests/wallet.py
index e38cf70..c2e5cd1 100755
--- a/tests/functional_tests/wallet.py
+++ b/tests/functional_tests/wallet.py
@@ -236,6 +236,26 @@ class WalletTest():
assert res.account_tags[0].tag == 'tagB'
assert res.account_tags[0].label == ''
assert res.account_tags[0].accounts == [0, 1]
+ ok = False
+ try: wallet.tag_accounts('tagC', [0, 3])
+ except Exception as e:
+ assert 'Account index out of bound' in str(e)
+ ok = True
+ assert ok
+ res = wallet.get_account_tags()
+ assert len(res.account_tags) == 1
+ assert res.account_tags[0].tag == 'tagB'
+ assert res.account_tags[0].accounts == [0, 1]
+ ok = False
+ try: wallet.untag_accounts([0, 3])
+ except Exception as e:
+ assert 'Account index out of bound' in str(e)
+ ok = True
+ assert ok
+ res = wallet.get_account_tags()
+ assert len(res.account_tags) == 1
+ assert res.account_tags[0].tag == 'tagB'
+ assert res.account_tags[0].accounts == [0, 1]
wallet.set_account_tag_description('tagB', 'tag B')
res = wallet.get_account_tags()
assert len(res.account_tags) == 1
Why this scored 34/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.