Merge branch 'fix/SDK-5874_Crash-at-advanceMacComputation' into 'release/v10.4.0'

SDK-5874. MAC computation thread safety [release v10.4.0]

See merge request sdk/sdk!7145

Author: vmgaGH

include/mega/localpath.h

@@ -182,7 +182,7 @@ class LocalPath
     LocalPath(LocalPath&&) noexcept;
     LocalPath& operator=(LocalPath&&) noexcept;
     LocalPath(const LocalPath& p);
-    LocalPath operator=(const LocalPath& p);
+    LocalPath& operator=(const LocalPath& p);
 
     virtual ~LocalPath() {}
 

include/mega/syncinternals/mac_computation_state.h

@@ -98,6 +98,11 @@ struct MacComputationState
     std::atomic<bool> completed{false}; // True when local MAC computed
     std::atomic<bool> failed{false}; // True if read/compute error
 
+    // True when initialization is complete (first advanceMacComputation has returned).
+    // Used to prevent checkPendingCloneMac from racing with initCloneCandidateMacComputation.
+    // The sync thread (checkPendingCloneMac) should not proceed until this is true.
+    std::atomic<bool> initializationComplete{false};
+
     // Throttle tracking - true if we've acquired a slot from MacComputationThrottle
     bool throttleSlotAcquired{false};
 
@@ -175,6 +180,28 @@ struct MacComputationState
         return failed.load(std::memory_order_acquire);
     }
 
+    /**
+     * @brief Thread-safe: check if initialization is complete.
+     *
+     * Returns true after the initializing thread has finished setting up the computation
+     * and the first advanceMacComputation call has returned. This prevents race conditions
+     * where checkPendingCloneMac runs before initCloneCandidateMacComputation completes.
+     */
+    bool isInitializationComplete() const
+    {
+        return initializationComplete.load(std::memory_order_acquire);
+    }
+
+    /**
+     * @brief Thread-safe: mark initialization as complete.
+     *
+     * Called after advanceMacComputation returns in the initialization function.
+     */
+    void setInitializationComplete()
+    {
+        initializationComplete.store(true, std::memory_order_release);
+    }
+
     /**
      * @brief Check if stored context matches current state (CSF case only).
      */

src/localpath.cpp

@@ -353,7 +353,7 @@ LocalPath::LocalPath(const LocalPath& p)
     assert(invariant());
 }
 
-LocalPath LocalPath::operator=(const LocalPath& p)
+LocalPath& LocalPath::operator=(const LocalPath& p)
 {
     if (this != &p)
     {

src/syncinternals/syncinternals.cpp

@@ -793,11 +793,13 @@ enum class MacAdvanceResult
  *
  * @param mc MegaClient for file access and async queue.
  * @param state The MAC computation state (must have cipher params initialized).
+ *              Passed by value to ensure the state stays alive during the function,
+ *              even if another thread resets the original shared_ptr.
  * @param logPrefix Prefix for log messages.
  * @return MacAdvanceResult indicating current state.
  */
 MacAdvanceResult advanceMacComputation(MegaClient& mc,
-                                       std::shared_ptr<MacComputationState>& state,
+                                       std::shared_ptr<MacComputationState> state,
                                        const std::string& logPrefix)
 {
     if (!state)
@@ -987,7 +989,9 @@ FsCloudComparisonResult asyncMacComputation(MegaClient& mc,
     // Check for existing computation
     if (syncNode.hasRare() && syncNode.rareRO().macComputation)
     {
-        auto& macComp = syncNode.rare().macComputation;
+        // Take a copy for consistency with clone candidate pattern (not strictly required
+        // since sync core is single-threaded, but defensive and documents intent)
+        auto macComp = syncNode.rare().macComputation;
 
         // Validate context is still current (handles moves, deletes, content changes)
         if (!macComp->contextMatches(fs.fsid, cn.handle, fs.fingerprint, cn.fingerprint))
@@ -1089,6 +1093,10 @@ FsCloudComparisonResult asyncMacComputation(MegaClient& mc,
         return {NODE_COMP_EREAD, INVALID_META_MAC, INVALID_META_MAC, FingerprintMismatch::Other};
     }
 
+    // Mark initialization as complete (for consistency with clone candidate case,
+    // even though the sync core case is single-threaded)
+    macComp->setInitializationComplete();
+
     return {NODE_COMP_PENDING, INVALID_META_MAC, INVALID_META_MAC, FingerprintMismatch::Other};
 }
 
@@ -1186,34 +1194,36 @@ CloneMacStatus initCloneCandidateMacComputation(MegaClient& mc, SyncUpload_inCli
 
     // Create computation state (no context needed - upload lifetime handles validity)
     const m_off_t fileSize = upload.size;
-    upload.macComputation = std::make_shared<MacComputationState>(fileSize,
-                                                                  upload.getLocalname(),
-                                                                  mc.syncs.mMacComputationThrottle);
+    auto macComp = std::make_shared<MacComputationState>(fileSize,
+                                                         upload.getLocalname(),
+                                                         mc.syncs.mMacComputationThrottle);
+    upload.macComputation = macComp; // Store in upload, but keep local copy for safety
 
     // Initialize cipher params from candidate node
+    // Using local macComp pointer ensures the object stays alive even if another thread
+    // resets upload.macComputation during this function.
     const auto& nodeKey = candidateNode->nodekey();
-    auto& macComp = *upload.macComputation;
 
-    macComp.cloneCandidateHandle = candidateNode->nodeHandle();
-    macComp.cloneCandidateNodeKey = nodeKey;
+    macComp->cloneCandidateHandle = candidateNode->nodeHandle();
+    macComp->cloneCandidateNodeKey = nodeKey;
 
-    memcpy(macComp.transferkey.data(), nodeKey.data(), SymmCipher::KEYLENGTH);
+    memcpy(macComp->transferkey.data(), nodeKey.data(), SymmCipher::KEYLENGTH);
     SymmCipher::xorblock((const byte*)nodeKey.data() + SymmCipher::KEYLENGTH,
-                         macComp.transferkey.data());
+                         macComp->transferkey.data());
 
     const char* iva = nodeKey.data() + SymmCipher::KEYLENGTH;
-    macComp.ctriv = MemAccess::get<int64_t>(iva);
+    macComp->ctriv = MemAccess::get<int64_t>(iva);
 
     // Try to acquire throttle now; if not, leave computation set up and return Pending
     if (mc.syncs.macComputationThrottle().tryAcquireFile())
     {
-        macComp.throttleSlotAcquired = true;
+        macComp->throttleSlotAcquired = true;
 
         LOG_debug << logPre << "Initiating: " << upload.getLocalname() << " [size=" << fileSize
                   << ", files=" << mc.syncs.macComputationThrottle().currentFiles() << "]";
 
-        // Start first chunk
-        auto result = advanceMacComputation(mc, upload.macComputation, logPre);
+        // Start first chunk - pass local macComp to ensure object stays alive
+        auto result = advanceMacComputation(mc, macComp, logPre);
         if (result == MacAdvanceResult::Failed)
         {
             LOG_debug << logPre << "Failed to start computation of MAC for "
@@ -1228,19 +1238,33 @@ CloneMacStatus initCloneCandidateMacComputation(MegaClient& mc, SyncUpload_inCli
                     << " [files=" << mc.syncs.macComputationThrottle().currentFiles() << "]";
     }
 
+    // Mark initialization as complete - checkPendingCloneMac can now safely proceed
+    macComp->setInitializationComplete();
+
     return CloneMacStatus::Pending; // Computation created; may be waiting for throttle or running
 }
 
 CloneMacStatus checkPendingCloneMac(MegaClient& mc, SyncUpload_inClient& upload)
 {
     static const std::string logPre{"checkPendingCloneMac: "};
 
-    if (!upload.macComputation)
+    // Take a copy of the shared_ptr to ensure the MacComputationState stays alive
+    // during this function, even if another thread resets upload.macComputation.
+    // This also makes the null check and subsequent use atomic with respect to
+    // other threads that might reset the shared_ptr.
+    auto macComp = upload.macComputation;
+    if (!macComp)
     {
         return CloneMacStatus::NoCandidates;
     }
 
-    auto& macComp = upload.macComputation;
+    // Wait until initCloneCandidateMacComputation has finished setting up the computation.
+    // This prevents races where we try to process/advance a computation that's still
+    // being initialized on the client thread.
+    if (!macComp->isInitializationComplete())
+    {
+        return CloneMacStatus::Pending;
+    }
 
     if (upload.mMetaMac.has_value())
     {

tests/unit/localpath_test.cpp

@@ -346,6 +346,113 @@ TEST(LocalPathTest, LocalPathToLocalPathStr)
               rootDrive + pathSep + "folder1" + pathSep + "Jos\xC3\xA9.txt");
 }
 
+/**
+ * @brief Tests that the copy assignment operator works correctly.
+ *
+ * Regression test for a bug where operator= returned LocalPath (by value)
+ * instead of LocalPath& (by reference), which could leave the assigned-to
+ * object with a null mImplementation after the temporary return value was destroyed.
+ */
+TEST(LocalPathTest, CopyAssignment)
+{
+    // Create source path
+    LocalPath source =
+        LocalPath::fromAbsolutePath(rootDrive + pathSep + "folder" + pathSep + "file.txt");
+    ASSERT_FALSE(source.empty());
+
+    // Test simple copy assignment
+    LocalPath dest;
+    dest = source;
+
+    // Both source and dest should be valid after assignment
+    EXPECT_FALSE(source.empty()) << "Source should remain valid after copy assignment";
+    EXPECT_FALSE(dest.empty()) << "Destination should be valid after copy assignment";
+    EXPECT_EQ(source, dest) << "Source and destination should be equal";
+
+    // Verify dest can still be used (this would crash with null mImplementation)
+    EXPECT_EQ(dest.toPath(false), source.toPath(false));
+    EXPECT_EQ(dest.leafName(), source.leafName());
+}
+
+/**
+ * @brief Tests that chained copy assignment works correctly.
+ *
+ * This specifically tests the return type of operator= - it must return
+ * LocalPath& for chained assignments to work.
+ */
+TEST(LocalPathTest, ChainedCopyAssignment)
+{
+    LocalPath a = LocalPath::fromAbsolutePath(rootDrive + pathSep + "original.txt");
+    LocalPath b;
+    LocalPath c;
+
+    // Chained assignment: c = b = a
+    // This requires operator= to return LocalPath&
+    c = b = a;
+
+    // All three should be valid and equal
+    EXPECT_FALSE(a.empty());
+    EXPECT_FALSE(b.empty());
+    EXPECT_FALSE(c.empty());
+    EXPECT_EQ(a, b);
+    EXPECT_EQ(b, c);
+
+    // Verify all can still be used
+    std::string expectedPath = a.toPath(false);
+    EXPECT_EQ(b.toPath(false), expectedPath);
+    EXPECT_EQ(c.toPath(false), expectedPath);
+}
+
+/**
+ * @brief Tests that move assignment works correctly.
+ */
+TEST(LocalPathTest, MoveAssignment)
+{
+    std::string originalPath = rootDrive + pathSep + "folder" + pathSep + "file.txt";
+    LocalPath source = LocalPath::fromAbsolutePath(originalPath);
+
+    LocalPath dest;
+    dest = std::move(source);
+
+    // dest should have the original value
+    EXPECT_FALSE(dest.empty());
+    EXPECT_EQ(dest.toPath(false), originalPath);
+}
+
+/**
+ * @brief Tests that copy constructor works correctly.
+ */
+TEST(LocalPathTest, CopyConstructor)
+{
+    LocalPath source =
+        LocalPath::fromAbsolutePath(rootDrive + pathSep + "folder" + pathSep + "file.txt");
+
+    // Copy construct
+    LocalPath dest(source);
+
+    // Both should be valid and equal
+    EXPECT_FALSE(source.empty());
+    EXPECT_FALSE(dest.empty());
+    EXPECT_EQ(source, dest);
+    EXPECT_EQ(source.toPath(false), dest.toPath(false));
+}
+
+/**
+ * @brief Tests that move constructor works correctly.
+ */
+TEST(LocalPathTest, MoveConstructor)
+{
+    std::string originalPath = rootDrive + pathSep + "folder" + pathSep + "file.txt";
+    LocalPath source = LocalPath::fromAbsolutePath(originalPath);
+
+    // Move construct
+    LocalPath dest(std::move(source));
+
+    // dest should have the original value
+    EXPECT_FALSE(dest.empty());
+    EXPECT_EQ(dest.toPath(false), originalPath);
+}
+
 TEST(LocalPathTest, leafOrParentName)
 {
     LOG_debug << "#### Test1: get leafOrParentName for different example LocalPaths ####";