What the developer wrote
Authored by j-berman
p2p: use a bool for send()
Identified by @selsta
Callers can otherwise misinterpret int -1 as truthy success,
e.g. see make_payload_send_txs
What changed, and why it matters
This patch fixes a type-safety bug in Monero's peer-to-peer networking code. The send() function used to return an integer where -1 meant failure, 0 meant 'connection not found', and 1 meant success. Some callers treated any non-zero value as success, so a -1 failure could be misread as success. The patch changes send() to return a proper true/false boolean so failures cannot be misinterpreted. The commit message explicitly points to make_payload_send_txs as an example of such a caller.
Review all remaining int-returning network send/notify functions for similar truthiness bugs, and verify that make_payload_send_txs and related tx propagation paths now correctly handle send failures. Consider adding explicit unit tests for send failure paths.
Security signals we found
Return value type confusion (int vs bool) leading to potential misinterpretation of failure as success
P2P message send failure not propagated correctly to caller
Defensive hardening of network protocol error handling
Commit message explicitly references a real misinterpretation site (make_payload_send_txs)
Evidence from the diff
The levin protocol send() methods in contrib/epee/include/net/levin_protocol_handler_async.h and contrib/epee/include/storages/levin_abstract_invoke2.h returned int: -1 on send_message failure, 0 when the connection was not found, and 1 on success. Callers in levin_abstract_invoke2.h and src/p2p/net_node.inl compared the result with >0 or <=0, but the commit message notes that make_payload_send_txs could misinterpret -1 as truthy. The patch changes the return type to bool, returning false on failure/not-found and true on success, and updates all callers and unit tests accordingly. This is a defensive correctness fix that removes a class of error-handling bugs.
Changed components
contrib/epee/include/net/levin_protocol_handler_async.hcontrib/epee/include/storages/levin_abstract_invoke2.hsrc/p2p/net_node.inltests/unit_tests/levin.cppInspect captured patch +16 / −16
diff --git a/contrib/epee/include/net/levin_protocol_handler_async.h b/contrib/epee/include/net/levin_protocol_handler_async.h
index 4a0e565..4a63ab3 100644
--- a/contrib/epee/include/net/levin_protocol_handler_async.h
+++ b/contrib/epee/include/net/levin_protocol_handler_async.h
@@ -111,7 +111,7 @@ public:
template<class callback_t>
int invoke_async(int command, message_writer in_msg, boost::uuids::uuid connection_id, const callback_t &cb, std::chrono::milliseconds timeout = LEVIN_DEFAULT_TIMEOUT_PRECONFIGURED);
- int send(epee::byte_slice message, const boost::uuids::uuid& connection_id);
+ bool send(epee::byte_slice message, const boost::uuids::uuid& connection_id);
bool close(boost::uuids::uuid connection_id, const bool wait_for_shutdown);
bool request_callback(boost::uuids::uuid connection_id);
template<class callback_t>
@@ -640,15 +640,15 @@ public:
`message_writer::finalize_notify`. See additional instructions for
`make_fragmented_notify`.
- \return 1 on success */
- int send(byte_slice message)
+ \return true on success */
+ bool send(byte_slice message)
{
if (!send_message(std::move(message)))
{
LOG_ERROR_CC(m_connection_context, "Failed to send message, dropping it");
- return -1;
+ return false;
}
- return 1;
+ return true;
}
//------------------------------------------------------------------------------------------
boost::uuids::uuid get_connection_id() {return m_connection_context.m_connection_id;}
@@ -799,10 +799,10 @@ void async_protocol_handler_config<t_connection_context>::set_handler(levin_comm
}
//------------------------------------------------------------------------------------------
template<class t_connection_context>
-int async_protocol_handler_config<t_connection_context>::send(byte_slice message, const boost::uuids::uuid& connection_id)
+bool async_protocol_handler_config<t_connection_context>::send(byte_slice message, const boost::uuids::uuid& connection_id)
{
const std::shared_ptr<levin_endpoint> aph = find_and_lock_connection(connection_id);
- return aph ? aph->m_protocol_handler.send(std::move(message)) : 0;
+ return aph ? aph->m_protocol_handler.send(std::move(message)) : false;
}
//------------------------------------------------------------------------------------------
template<class t_connection_context>
diff --git a/contrib/epee/include/storages/levin_abstract_invoke2.h b/contrib/epee/include/storages/levin_abstract_invoke2.h
index 7fd52ea..1a291f6 100644
--- a/contrib/epee/include/storages/levin_abstract_invoke2.h
+++ b/contrib/epee/include/storages/levin_abstract_invoke2.h
@@ -110,10 +110,10 @@ namespace epee
levin::message_writer to_send;
stg.store_to_binary(to_send.buffer);
- int res = transport.send(to_send.finalize_notify(command), conn_id);
- if(res <=0 )
+ bool res = transport.send(to_send.finalize_notify(command), conn_id);
+ if(!res)
{
- MERROR("Failed to notify command " << command << " return code " << res);
+ MERROR("Failed to notify command " << command);
return false;
}
return true;
diff --git a/src/p2p/net_node.inl b/src/p2p/net_node.inl
index f989ef7..11f3185 100644
--- a/src/p2p/net_node.inl
+++ b/src/p2p/net_node.inl
@@ -2503,8 +2503,8 @@ namespace nodetool
return false;
network_zone& zone = m_network_zones.at(context.m_remote_address.get_zone());
- int res = zone.m_net_server.get_config_object().send(message.finalize_notify(command), context.m_connection_id);
- return res > 0;
+ bool res = zone.m_net_server.get_config_object().send(message.finalize_notify(command), context.m_connection_id);
+ return res;
}
//-----------------------------------------------------------------------------------
template<class t_payload_net_handler>
diff --git a/tests/unit_tests/levin.cpp b/tests/unit_tests/levin.cpp
index 99c4aa9..4d890ef 100644
--- a/tests/unit_tests/levin.cpp
+++ b/tests/unit_tests/levin.cpp
@@ -2470,8 +2470,8 @@ TEST_F(levin_notify, command_max_bytes)
bytes = dest.finalize_notify(ping_command);
}
- EXPECT_EQ(1, get_connections().send(bytes.clone(), contexts_.front()->get_id()));
- EXPECT_EQ(1u, contexts_.front()->process_send_queue(true));
+ EXPECT_TRUE(get_connections().send(bytes.clone(), contexts_.front()->get_id()));
+ EXPECT_TRUE(contexts_.front()->process_send_queue(true));
EXPECT_EQ(1u, receiver_.notified_size());
const received_message msg = receiver_.get_raw_notification();
@@ -2486,7 +2486,7 @@ TEST_F(levin_notify, command_max_bytes)
bytes = dest.finalize_notify(ping_command);
}
- EXPECT_EQ(1, get_connections().send(std::move(bytes), contexts_.front()->get_id()));
- EXPECT_EQ(1u, contexts_.front()->process_send_queue(false));
+ EXPECT_TRUE(get_connections().send(std::move(bytes), contexts_.front()->get_id()));
+ EXPECT_TRUE(contexts_.front()->process_send_queue(false));
EXPECT_EQ(0u, receiver_.notified_size());
}
Why this scored 48/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.