Fix[bmqio]: return peerUri by value to eliminate data race (#1641)

SrinathhSatuluri · web-flow · commit fb2004a43eb3 · 2026-07-27T10:55:32.000-04:00
Return `peerUri` by value instead of by const reference so the caller is not left holding an unprotected reference after the lock drops. This changes the base `Channel` interface and all four implementations. In `ResolvingChannelFactory_Channel`, the constructor previously took the address of the return value for the `AtomicPointer` pattern, so a stored copy is used instead. Also fixes two latent `StringRef` dangles that only worked because `peerUri` returned a reference to a stable member.

Signed-off-by: Srinath Satuluri &lt;satulurisrinath@gmail.com&gt;
diff --git a/src/groups/bmq/bmqio/bmqio_channel.h b/src/groups/bmq/bmqio/bmqio_channel.h
@@ -219,7 +219,7 @@ class Channel {
 
     /// Return the URI of the "remote" end of this channel.  It is up to the
     /// underlying implementation to define the format of the returned URI.
-    virtual const bsl::string& peerUri() const = 0;
+    virtual bsl::string peerUri() const = 0;
 
     /// Return a reference providing modifiable access to the properties of
     /// this Channel.
diff --git a/src/groups/bmq/bmqio/bmqio_decoratingchannelpartialimp.h b/src/groups/bmq/bmqio/bmqio_decoratingchannelpartialimp.h
@@ -100,7 +100,7 @@ class DecoratingChannelPartialImp : public Channel {
     Channel* base() const;
 
     // Channel
-    const bsl::string& peerUri() const BSLS_KEYWORD_OVERRIDE;
+    bsl::string peerUri() const BSLS_KEYWORD_OVERRIDE;
 
     /// Forward to the underlying base `Channel`.
     const bmqvt::PropertyBag& properties() const BSLS_KEYWORD_OVERRIDE;
@@ -177,7 +177,7 @@ inline Channel* DecoratingChannelPartialImp::base() const
     return d_base.get();
 }
 
-inline const bsl::string& DecoratingChannelPartialImp::peerUri() const
+inline bsl::string DecoratingChannelPartialImp::peerUri() const
 {
     return d_base->peerUri();
 }
diff --git a/src/groups/bmq/bmqio/bmqio_ntcchannel.cpp b/src/groups/bmq/bmqio/bmqio_ntcchannel.cpp
@@ -1342,8 +1342,9 @@ ntsa::Endpoint NtcChannel::sourceEndpoint() const
                              : ntsa::Endpoint();
 }
 
-const bsl::string& NtcChannel::peerUri() const
+bsl::string NtcChannel::peerUri() const
 {
+    bslmt::LockGuard<bslmt::Mutex> lock(&d_mutex);
     return d_peerUri;
 }
 
diff --git a/src/groups/bmq/bmqio/bmqio_ntcchannel.h b/src/groups/bmq/bmqio/bmqio_ntcchannel.h
@@ -423,7 +423,7 @@ class NtcChannel : public bmqio::Channel,
 
     /// Return the URI of the "remote" end of this channel.  It is up to the
     /// underlying implementation to define the format of the returned URI.
-    const bsl::string& peerUri() const BSLS_KEYWORD_OVERRIDE;
+    bsl::string peerUri() const BSLS_KEYWORD_OVERRIDE;
 
     /// Return a reference providing modifiable access to the properties of
     /// this Channel.
diff --git a/src/groups/bmq/bmqio/bmqio_ntcchannel.t.cpp b/src/groups/bmq/bmqio/bmqio_ntcchannel.t.cpp
@@ -88,7 +88,7 @@ void executeOnClosedChannelFunc(bmqio::NtcChannel*              channel,
     BSLA_MAYBE_UNUSED ntsa::Endpoint peerEndpoint = channel->peerEndpoint();
     BSLA_MAYBE_UNUSED ntsa::Endpoint sourceEndpoint =
         channel->sourceEndpoint();
-    BSLA_MAYBE_UNUSED const bsl::string& peerUri     = channel->peerUri();
+    BSLA_MAYBE_UNUSED bsl::string peerUri            = channel->peerUri();
     BSLA_MAYBE_UNUSED bmqvt::PropertyBag& properties = channel->properties();
 
     channel->setChannelId(id);
diff --git a/src/groups/bmq/bmqio/bmqio_ntcchannelfactory.t.cpp b/src/groups/bmq/bmqio/bmqio_ntcchannelfactory.t.cpp
@@ -1051,7 +1051,7 @@ void Tester::checkChannelUri(int                      line,
         return;  // RETURN
     }
 
-    bslstl::StringRef uri = info.d_channel->peerUri();
+    bsl::string uri = info.d_channel->peerUri();
     BMQTST_ASSERT_EQ_D(line,
                        uri.data(),
                        bdlb::StringRefUtil::strstr(uri, prefix).data());
diff --git a/src/groups/bmq/bmqio/bmqio_resolvingchannelfactory.cpp b/src/groups/bmq/bmqio/bmqio_resolvingchannelfactory.cpp
@@ -95,12 +95,13 @@ ResolvingChannelFactory_Channel::ResolvingChannelFactory_Channel(
     bslma::Allocator*               basicAllocator)
 : DecoratingChannelPartialImp(channel, basicAllocator)
 , d_resolvedPeerUri(basicAllocator)
+, d_basePeerUri(channel->peerUri(), basicAllocator)
 , d_peerUri()
 {
     // PRECONDITIONS
     BSLS_ASSERT(channel);
 
-    d_peerUri = &channel->peerUri();
+    d_peerUri = &d_basePeerUri;
 }
 
 // MANIPULATORS
@@ -117,7 +118,7 @@ void ResolvingChannelFactory_Channel::updatePeerUri()
 }
 
 // ACCESSORS
-const bsl::string& ResolvingChannelFactory_Channel::peerUri() const
+bsl::string ResolvingChannelFactory_Channel::peerUri() const
 {
     return *d_peerUri;
 }
@@ -235,7 +236,7 @@ void ResolvingChannelFactoryUtil::defaultResolutionFn(
     const ResolveFn& resolveFn,
     bool             verbose)
 {
-    bslstl::StringRef peerUri = baseChannel.peerUri();
+    bsl::string       peerUri = baseChannel.peerUri();
     bslstl::StringRef colon   = bdlb::StringRefUtil::strstr(peerUri, ":");
     if (colon.length() == 0) {
         if (verbose) {
@@ -248,7 +249,7 @@ void ResolvingChannelFactoryUtil::defaultResolutionFn(
 
     bdlma::LocalSequentialAllocator<128> arena;
 
-    bsl::string     ipAddrStr(peerUri.data(), colon.data(), &arena);
+    bsl::string     ipAddrStr(peerUri.c_str(), colon.data(), &arena);
     ntsa::IpAddress ipAddr;
 
     if (!ipAddr.parse(ipAddrStr)) {
@@ -277,7 +278,7 @@ void ResolvingChannelFactoryUtil::defaultResolutionFn(
     resolvedUri->append(ipAddrStr);
     resolvedUri->append(1, '~');
     resolvedUri->append(resolvedName);
-    resolvedUri->append(colon.data(), peerUri.end());
+    resolvedUri->append(colon.data(), peerUri.c_str() + peerUri.length());
 }
 
 }  // close package namespace
diff --git a/src/groups/bmq/bmqio/bmqio_resolvingchannelfactory.h b/src/groups/bmq/bmqio/bmqio_resolvingchannelfactory.h
@@ -140,6 +140,8 @@ class ResolvingChannelFactory_Channel : public DecoratingChannelPartialImp {
     // PRIVATE DATA
     bsl::string d_resolvedPeerUri;
 
+    bsl::string d_basePeerUri;
+
     bsls::AtomicPointer<const bsl::string> d_peerUri;
 
   private:
@@ -177,7 +179,7 @@ class ResolvingChannelFactory_Channel : public DecoratingChannelPartialImp {
 
     /// Return our base Channel's peerUri until our resolution is done, and
     /// start returning the resolved peerUri after that.
-    const bsl::string& peerUri() const BSLS_KEYWORD_OVERRIDE;
+    bsl::string peerUri() const BSLS_KEYWORD_OVERRIDE;
 };
 
 // =============================
diff --git a/src/groups/bmq/bmqio/bmqio_testchannel.cpp b/src/groups/bmq/bmqio/bmqio_testchannel.cpp
@@ -163,7 +163,7 @@ bmqvt::PropertyBag& TestChannel::properties()
     return d_properties;
 }
 
-const bsl::string& TestChannel::peerUri() const
+bsl::string TestChannel::peerUri() const
 {
     return d_peerUri;
 }
diff --git a/src/groups/bmq/bmqio/bmqio_testchannel.h b/src/groups/bmq/bmqio/bmqio_testchannel.h
@@ -308,7 +308,7 @@ class TestChannel : public Channel {
 
     // ACCESSORS
     // Channel
-    const bsl::string&        peerUri() const BSLS_KEYWORD_OVERRIDE;
+    bsl::string               peerUri() const BSLS_KEYWORD_OVERRIDE;
     const bmqvt::PropertyBag& properties() const BSLS_KEYWORD_OVERRIDE;
 };
 
diff --git a/src/groups/mqb/mqba/mqba_adminsession.t.cpp b/src/groups/mqb/mqba/mqba_adminsession.t.cpp
@@ -116,7 +116,7 @@ struct TestAdminRetranslator {
     TestAdminRetranslator() {}
 
     int enqueueCommand(
-        BSLA_MAYBE_UNUSED const bslstl::StringRef&      source,
+        BSLA_MAYBE_UNUSED const bsl::string&            source,
         const bsl::string&                              cmd,
         const mqbnet::Session::AdminCommandProcessedCb& onProcessedCb)
     {
diff --git a/src/groups/mqb/mqba/mqba_application.cpp b/src/groups/mqb/mqba/mqba_application.cpp
@@ -884,10 +884,10 @@ void Application::executeCommand(const mqbcmd::Command&  command,
     }
 }
 
-int Application::processCommand(const bslstl::StringRef& source,
-                                const bsl::string&       cmd,
-                                bsl::ostream&            os,
-                                bool                     fromReroute)
+int Application::processCommand(const bsl::string& source,
+                                const bsl::string& cmd,
+                                bsl::ostream&      os,
+                                bool               fromReroute)
 {
     enum RcEnum {
         rc_SUCCESS     = 0,
@@ -987,7 +987,7 @@ int Application::processCommand(const bslstl::StringRef& source,
 }
 
 int Application::processCommandCb(
-    const bslstl::StringRef&                            source,
+    const bsl::string&                                  source,
     const bsl::string&                                  cmd,
     const bsl::function<void(int, const bsl::string&)>& onProcessedCb,
     bool                                                fromReroute)
@@ -1008,7 +1008,7 @@ int Application::processCommandCb(
 }
 
 int Application::enqueueCommand(
-    const bslstl::StringRef&                            source,
+    const bsl::string&                                  source,
     const bsl::string&                                  cmd,
     const bsl::function<void(int, const bsl::string&)>& onProcessedCb,
     bool                                                fromReroute)
diff --git a/src/groups/mqb/mqba/mqba_application.h b/src/groups/mqb/mqba/mqba_application.h
@@ -240,18 +240,18 @@ class Application {
     /// Mark `fromReroute` as true if executing the command from a reroute to
     /// ensure proper routing logic. Returns 0 on success, -1 on early exit,
     /// -2 on error, and some non-zero error code on parse failure.
-    int processCommand(const bslstl::StringRef& source,
-                       const bsl::string&       cmd,
-                       bsl::ostream&            os,
-                       bool                     fromReroute = false);
+    int processCommand(const bsl::string& source,
+                       const bsl::string& cmd,
+                       bsl::ostream&      os,
+                       bool               fromReroute = false);
 
     /// Process the command `cmd` coming from the specified `source` node, and
     /// send the result of the command in the given `onProcessedCb`. Mark
     /// `fromReroute` as true if executing command from a reroute to ensure
     /// proper routing logic. Returns the error code of calling
     /// `processCommand` with the given `cmd`, `source`, and `fromReroute`.
     int processCommandCb(
-        const bslstl::StringRef&                            source,
+        const bsl::string&                                  source,
         const bsl::string&                                  cmd,
         const bsl::function<void(int, const bsl::string&)>& onProcessedCb,
         bool fromReroute = false);
@@ -262,7 +262,7 @@ class Application {
     /// as true if executing command from a reroute to ensure proper routing
     /// logic.
     int enqueueCommand(
-        const bslstl::StringRef&                            source,
+        const bsl::string&                                  source,
         const bsl::string&                                  cmd,
         const bsl::function<void(int, const bsl::string&)>& onProcessedCb,
         bool fromReroute = false);
diff --git a/src/groups/mqb/mqbnet/mqbnet_session.h b/src/groups/mqb/mqbnet/mqbnet_session.h
@@ -99,7 +99,7 @@ class Session : public SessionEventProcessor {
     /// executed.  The execution result (structured, non-structured text or
     /// possible error message) is expected to be passed to the specified
     /// `onProcessed` callback.
-    typedef bsl::function<void(const bslstl::StringRef&       source,
+    typedef bsl::function<void(const bsl::string&             source,
                                const bsl::string&             command,
                                const AdminCommandProcessedCb& onProcessed,
                                bool                           fromReroute)>
diff --git a/src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.g.cpp b/src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.g.cpp
@@ -136,7 +136,7 @@ class MockChannel : public bmqio::Channel {
 
     MOCK_METHOD1(setWriteQueueHighWatermark, void(int highWatermark));
 
-    MOCK_CONST_METHOD0(peerUri, const bsl::string&());
+    MOCK_CONST_METHOD0(peerUri, bsl::string());
 
     MOCK_CONST_METHOD0(properties, const bmqvt::PropertyBag&());
 };

Original file line number	Diff line number	Diff line change
`@@ -100,7 +100,7 @@ class DecoratingChannelPartialImp : public Channel {`
`100`	`100`	`Channel* base() const;`
`101`	`101`
`102`	`102`	`// Channel`
`103`		`- const bsl::string& peerUri() const BSLS_KEYWORD_OVERRIDE;`
	`103`	`+ bsl::string peerUri() const BSLS_KEYWORD_OVERRIDE;`
`104`	`104`
`105`	`105`	/// Forward to the underlying base `Channel`.
`106`	`106`	`const bmqvt::PropertyBag& properties() const BSLS_KEYWORD_OVERRIDE;`
`@@ -177,7 +177,7 @@ inline Channel* DecoratingChannelPartialImp::base() const`
`177`	`177`	`return d_base.get();`
`178`	`178`	`}`
`179`	`179`
`180`		`-inline const bsl::string& DecoratingChannelPartialImp::peerUri() const`
	`180`	`+inline bsl::string DecoratingChannelPartialImp::peerUri() const`
`181`	`181`	`{`
`182`	`182`	`return d_base->peerUri();`
`183`	`183`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1342,8 +1342,9 @@ ntsa::Endpoint NtcChannel::sourceEndpoint() const`
`1342`	`1342`	`: ntsa::Endpoint();`
`1343`	`1343`	`}`
`1344`	`1344`
`1345`		`-const bsl::string& NtcChannel::peerUri() const`
	`1345`	`+bsl::string NtcChannel::peerUri() const`
`1346`	`1346`	`{`
	`1347`	`+ bslmt::LockGuard<bslmt::Mutex> lock(&d_mutex);`
`1347`	`1348`	`return d_peerUri;`
`1348`	`1349`	`}`
`1349`	`1350`
Original file line number	Diff line number	Diff line change
`@@ -1051,7 +1051,7 @@ void Tester::checkChannelUri(int line,`
`1051`	`1051`	`return; // RETURN`
`1052`	`1052`	`}`
`1053`	`1053`
`1054`		`- bslstl::StringRef uri = info.d_channel->peerUri();`
	`1054`	`+ bsl::string uri = info.d_channel->peerUri();`
`1055`	`1055`	`BMQTST_ASSERT_EQ_D(line,`
`1056`	`1056`	`uri.data(),`
`1057`	`1057`	`bdlb::StringRefUtil::strstr(uri, prefix).data());`
Original file line number	Diff line number	Diff line change
`@@ -163,7 +163,7 @@ bmqvt::PropertyBag& TestChannel::properties()`
`163`	`163`	`return d_properties;`
`164`	`164`	`}`
`165`	`165`
`166`		`-const bsl::string& TestChannel::peerUri() const`
	`166`	`+bsl::string TestChannel::peerUri() const`
`167`	`167`	`{`
`168`	`168`	`return d_peerUri;`
`169`	`169`	`}`