foster/bitcoin-bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-08 10:31:50 -05:00
Code Issues Activity

Merge bitcoin/bitcoin#30617: net: Clarify that m_addr_local is only set once

Browse source 
fa6fe43207 net: Clarify that m_addr_local is only set once (MarcoFalke)

Pull request description:

  The function is supposed to be only called once when the version msg arrives (a single time). Calling it twice would be an internal logic bug. However, the `LogError` in this function has many issues:

  * If the error happens in tests, as is the case for the buggy fuzz test, it will go unnoticed
  * It is dead code, unless a bug is introduced to execute it

  Fix all issues by using `Assume(!m_addr_local.IsValid())` instead. Idea taken from https://github.com/bitcoin/bitcoin/pull/30364#discussion_r1680530382

ACKs for top commit:
  achow101:
    ACK fa6fe43207
  mzumsande:
    utACK fa6fe43207
  glozow:
    ACK fa6fe43207

Tree-SHA512: 8c1e8c524768f4f36cc50110ae54ee423e057a963ff78f736f3bf92df1ce5af28e3e0149153780897944e1d5c22ddbca9dac9865d9f4d44afffa152bc8559405
This commit is contained in:
glozow 2024-08-13 10:07:31 +01:00
commit 7583eac43c
No known key found for this signature in database
GPG key ID: BA03F4DBE0C63FB4
3 changed files with 9 additions and 13 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
src/net.cpp
View file

@ -576,16 +576,14 @@ CService CNode::GetAddrLocal() const
{
AssertLockNotHeld(m_addr_local_mutex);
LOCK(m_addr_local_mutex);
return addrLocal;
return m_addr_local;
}
void CNode::SetAddrLocal(const CService& addrLocalIn) {
AssertLockNotHeld(m_addr_local_mutex);
LOCK(m_addr_local_mutex);
if (addrLocal.IsValid()) {
LogError("Addr local already set for node: %i. Refusing to change from %s to %s\n", id, addrLocal.ToStringAddrPort(), addrLocalIn.ToStringAddrPort());
} else {
addrLocal = addrLocalIn;
if (Assume(!m_addr_local.IsValid())) { // Addr local can only be set once during version msg processing
m_addr_local = addrLocalIn;
}
}

2
src/net.h
View file

@ -963,7 +963,7 @@ private:
size_t m_msg_process_queue_size GUARDED_BY(m_msg_process_queue_mutex){0};
// Our address, as reported by the peer
CService addrLocal GUARDED_BY(m_addr_local_mutex);
CService m_addr_local GUARDED_BY(m_addr_local_mutex);
mutable Mutex m_addr_local_mutex;
mapMsgTypeSize mapSendBytesPerMsgType GUARDED_BY(cs_vSend);

12
src/test/fuzz/net.cpp
View file

@ -33,6 +33,11 @@ FUZZ_TARGET(net, .init = initialize_net)
SetMockTime(ConsumeTime(fuzzed_data_provider));
CNode node{ConsumeNode(fuzzed_data_provider)};
node.SetCommonVersion(fuzzed_data_provider.ConsumeIntegral<int>());
if (const auto service_opt =
ConsumeDeserializable<CService>(fuzzed_data_provider, ConsumeDeserializationParams<CNetAddr::SerParams>(fuzzed_data_provider)))
{
node.SetAddrLocal(*service_opt);
}
LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) {
CallOneOf(
fuzzed_data_provider,
@ -52,13 +57,6 @@ FUZZ_TARGET(net, .init = initialize_net)
node.Release();
}
},
[&] {
const std::optional<CService> service_opt = ConsumeDeserializable<CService>(fuzzed_data_provider, ConsumeDeserializationParams<CNetAddr::SerParams>(fuzzed_data_provider));
if (!service_opt) {
return;
}
node.SetAddrLocal(*service_opt);
},
[&] {
const std::vector<uint8_t> b = ConsumeRandomLengthByteVector(fuzzed_data_provider);
bool complete;