In commit https://github.com/bitcoin/bitcoin/pull/35084/changes/33d37f3c35efaac136863253b91799bf2711fd46 "ipc, refactor: Drop connect/listen/serve exe_name parameters
Why do we store this as a const char* instead of an std::string? I could not find any documentation describing the lifetime requirement here.
Right now, CapnpProtocol does not own the text. This is safe for the current callers, but it assumes that the caller keeps the underlying memory alive for the entire lifetime of the protocol. This could leave m_exe_name dangling if, for example, the original string is modified or destroyed after being passed to MakeCapnpProtocol().
Could we store it as an std::string instead? This would make CapnpProtocol own the value and ensure that its lifetime matches the lifetime of the protocol.
index e7eaf64301..1e4bdf533a 100644
--- a/src/ipc/capnp/protocol.cpp
+++ b/src/ipc/capnp/protocol.cpp
@@ -71,7 +71,7 @@ void IpcLogFn(mp::LogMessage message)
class CapnpProtocol : public Protocol
{
public:
- CapnpProtocol(const char* exe_name) : m_exe_name{exe_name} {}
+ CapnpProtocol(std::string exe_name) : m_exe_name{std::move(exe_name)} {}
~CapnpProtocol() noexcept(true)
{
m_loop_ref.reset();
@@ -94,12 +94,12 @@ public:
void serve(interfaces::Init& init, const std::function<mp::Stream()>& make_stream) override
{
assert(!m_loop);
- mp::CurrentThread().thread_name = mp::ThreadName(m_exe_name);
+ mp::CurrentThread().thread_name = mp::ThreadName(m_exe_name.c_str());
mp::LogOptions opts = {
.log_fn = IpcLogFn,
.log_level = GetRequestedIPCLogLevel()
};
- m_loop.emplace(m_exe_name, std::move(opts), &m_context);
+ m_loop.emplace(m_exe_name.c_str(), std::move(opts), &m_context);
mp::ServeStream<messages::Init>(*m_loop, make_stream(), init);
m_parent_connection = &m_loop->m_incoming_connections.back();
m_loop->loop();
@@ -135,7 +135,7 @@ public:
.log_fn = IpcLogFn,
.log_level = GetRequestedIPCLogLevel()
};
- m_loop.emplace(m_exe_name, std::move(opts), &m_context);
+ m_loop.emplace(m_exe_name.c_str(), std::move(opts), &m_context);
m_loop_ref.emplace(*m_loop);
promise.set_value();
m_loop->loop();
@@ -143,7 +143,7 @@ public:
});
promise.get_future().wait();
}
- const char* m_exe_name;
+ std::string m_exe_name;
Context m_context;
//! EventLoop object which manages I/O events for all connections.
std::optional<mp::EventLoop> m_loop;
This is mostly a nice to have and might be out of scope as well and better fit for a followup. Regardless, I think a bit more documentation here might be helpful
