Make IpcRemHandle trivially destructible to prevent secondary segfaults (meta-pytorch#2068)

Summary:

IpcRemHandle::peerId is changed from std::string to char[kMaxPeerIdLen].

When an IB request double-completion causes refCount_ to go negative, commInternalError propagates up through FB_COMMCHECKTHROW_EX in AllReduceRing, and stack unwinding destroys the local vector<unique_ptr<CtranMapperRequest>>. The destruction chain reaches IpcRemHandle::~IpcRemHandle(), where ~std::string() tries to free a corrupted heap pointer and segfaults — masking the real IB bug.

With char[], the destructor is trivial (no-op), so the error path completes cleanly and the actual commInternalError is reported.

Reviewed By: elvinlife

Differential Revision: D97314372

Author: dsjohns2

comms/ctran/mapper/tests/CtranDistMapperUT.cc

@@ -813,7 +813,8 @@ TEST_P(CtranDistMapperBufExportParam, BufExportCtrl) {
     if (remoteAccessKeys.backend == CtranMapperBackend::IB) {
       ASSERT_NE(remoteAccessKeys.ibKey.rkeys[0], 0);
     } else if (remoteAccessKeys.backend == CtranMapperBackend::NVL) {
-      ASSERT_EQ(remoteAccessKeys.nvlKey.peerId, statex->gPid(recvPeer));
+      ASSERT_STREQ(
+          remoteAccessKeys.nvlKey.peerId, statex->gPid(recvPeer).c_str());
       ASSERT_NE(remoteAccessKeys.nvlKey.basePtr, nullptr);
     }
 

comms/ctran/mapper/tests/CtranMapperUT.cc

@@ -1273,7 +1273,8 @@ TEST_F(CtranMapperTest, RemoteAccessKeyToString) {
     rkey1.ibKey.rkeys[i] = 291 + i;
   }
   rkey1.ibKey.nKeys = CTRAN_MAX_IB_DEVICES_PER_RANK;
-  rkey1.nvlKey.peerId = "host1:1234";
+  std::strncpy(
+      rkey1.nvlKey.peerId, "host1:1234", ctran::regcache::kMaxPeerIdLen);
   rkey1.nvlKey.basePtr = (void*)0x4567890;
   EXPECT_EQ(rkey1.toString(), "backend=IB, ibKey=[291, 292]");
 

comms/ctran/regcache/IpcRegCache.cc

@@ -108,7 +108,7 @@ commResult_t ctran::IpcRegCache::importMem(
   // import from baseAddr of a remote segment, return buf at offset from
   // baseAddr
   *buf = reinterpret_cast<char*>(basePtr) + ipcDesc.offset;
-  remKey->peerId = peerId;
+  std::snprintf(remKey->peerId, regcache::kMaxPeerIdLen, "%s", peerId.c_str());
   remKey->basePtr = ipcDesc.desc.base;
   remKey->uid = ipcDesc.uid;
   CLOGF_TRACE(

comms/ctran/regcache/IpcRegCacheBase.h

@@ -129,10 +129,14 @@ struct IpcRemRegElem {
   }
 };
 
+// Maximum length for peer ID string (including null terminator)
+// Format: "hostname:pid" - hostname can be up to 255 chars (DNS limit)
+constexpr size_t kMaxPeerIdLen = 272;
+
 struct IpcRemHandle {
   // use peerId, basePtr and uid on peer to lookup the imported memory handle
-  // in local cache
-  std::string peerId;
+  // in local cache.
+  char peerId[kMaxPeerIdLen]{};
   void* basePtr;
   uint32_t uid;
 
@@ -148,10 +152,6 @@ enum class IpcReqType : uint8_t {
   kRelease = 1, // Release notification
 };
 
-// Maximum length for peer ID string (including null terminator)
-// Format: "hostname:pid" - hostname can be up to 255 chars (DNS limit)
-constexpr size_t kMaxPeerIdLen = 272;
-
 // Unified IPC request structure sent over the network.
 // Used for both memory export (IpcDesc) and release (IpcRelease) requests.
 // The peer checks IpcReqType to determine which callback to invoke.

comms/ctran/regcache/tests/RegCacheUT.cc

@@ -4,6 +4,7 @@
 #include <gtest/gtest.h>
 
 #include <cstdlib>
+#include <cstring>
 #include <memory>
 
 #include "comms/ctran/backends/ib/CtranIbSingleton.h"
@@ -1365,6 +1366,26 @@ TEST_F(RegCacheTest, ImportMemWithExtraSegments) {
   COMMCHECK_TEST(ctran::commMemFreeDisjoint(buf, segSizes));
 }
 
+// Verify IpcRemHandle is trivially destructible and survives heap corruption.
+// Simulates the scenario from the IB double-completion bug: construct a valid
+// IpcRemHandle, corrupt its memory, then destroy it. With std::string peerId,
+// the destructor tries to free a corrupted pointer and segfaults. With a
+// fixed-size char[], the destructor is trivial and this is safe.
+TEST(IpcRemHandleTest, CorruptedDestroyDoesNotCrash) {
+  alignas(ctran::regcache::IpcRemHandle) char
+      buf[sizeof(ctran::regcache::IpcRemHandle)];
+  auto* handle = new (buf) ctran::regcache::IpcRemHandle();
+
+  // Corrupt the entire struct — simulates heap corruption
+  std::memset(buf, 0xAB, sizeof(ctran::regcache::IpcRemHandle));
+  // Prevent the compiler from optimizing away the corruption or destructor
+  asm volatile("" ::: "memory");
+
+  // Destructor must not crash. With std::string this segfaults;
+  // with char[] this is a no-op.
+  handle->~IpcRemHandle();
+}
+
 int main(int argc, char* argv[]) {
   ::testing::InitGoogleTest(&argc, argv);
   return RUN_ALL_TESTS();