tools/cephfs_mirror: Fix crash loading persisted sync stats on restart

PeerReplayer::apply_persisted_dir_sync_stat() took a reference into
a json_spirit::mValue for last_synced_snap, then reused that same
mValue for nested field lookups. The get_json_value() overwrote the
parent value and left a dangling Object reference, which could abort
the daemon when directories were re-acquired after a mirror daemon
restart or a mgr failover.

The fix is to Copy last_synced_snap before reading nested fields,
add typed JSON helpers (moved to Utils) that check types before
get_* calls, and catch std::exception so unexpected parse failures
are logged instead of taking down cephfs-mirror.

Fixes: https://tracker.ceph.com/issues/78932
Signed-off-by: Kotresh HR <khiremat@redhat.com>

diff --git a/src/tools/cephfs_mirror/PeerReplayer.cc b/src/tools/cephfs_mirror/PeerReplayer.cc
index 2527d9f15ab36c84e9c56205719afbd4776fc087..aa31ea26cfd5dc12f7c75653bd6dc34330aa70ae 100644 (file)
--- a/src/tools/cephfs_mirror/PeerReplayer.cc
+++ b/src/tools/cephfs_mirror/PeerReplayer.cc
@@ -1,6 +1,9 @@
 // -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:nil -*-
 // vim: ts=8 sw=2 sts=2 expandtab
 
+#include <cmath>
+#include <cstdint>
+#include <limits>
 #include <stack>
 #include <fcntl.h>
 #include <algorithm>
@@ -111,17 +114,6 @@ std::string peer_config_key(const std::string &fs_name, const std::string &uuid)
   return PEER_CONFIG_KEY_PREFIX + "/" + fs_name + "/" + uuid;
 }
 
-bool get_json_value(const json_spirit::mObject& obj,
-                    const std::string& key,
-                    json_spirit::mValue *val) {
-  auto it = obj.find(key);
-  if (it != obj.end()) {
-    *val = it->second;
-    return true;
-  }
-  return false;
-}
-
 struct C_PersistSyncStatAio : Context {
   std::string dir_root;
 
@@ -797,40 +789,55 @@ std::string PeerReplayer::peer_sync_stat_omap_key(std::string_view dir_root) con
 
 void PeerReplayer::apply_persisted_dir_sync_stat(SnapSyncStat &sync_stat,
                                                  const bufferlist &bl) {
-  json_spirit::mValue root;
-  if (!json_spirit::read(bl.to_str(), root) || root.type() != json_spirit::obj_type) {
-    return;
-  }
+  try {
+    json_spirit::mValue root;
+    if (!json_spirit::read(bl.to_str(), root) || root.type() != json_spirit::obj_type) {
+      return;
+    }
 
-  auto &obj = root.get_obj();
-  json_spirit::mValue v;
+    auto &obj = root.get_obj();
+    json_spirit::mValue v;
 
-  if (get_json_value(obj, "last_synced_snap", &v) && v.type() == json_spirit::obj_type) {
-    auto &last_synced_snap = v.get_obj();
-    if (get_json_value(last_synced_snap, "id", &v)) {
-      uint64_t snap_id = v.get_uint64();
-      if (get_json_value(last_synced_snap, "name", &v)) {
-        sync_stat.last_synced_snap = std::make_pair(snap_id, v.get_str());
+    // Copy the object out of v before reading nested fields. get_json_value()
+    // would otherwise overwrite v and leave a dangling reference into its Object.
+    if (get_json_value(obj, "last_synced_snap", &v) && v.type() == json_spirit::obj_type) {
+      const json_spirit::mObject last_synced_snap = v.get_obj();
+      uint64_t snap_id;
+      std::string snap_name;
+      if (get_json_uint64(last_synced_snap, "id", &snap_id) &&
+          get_json_string(last_synced_snap, "name", &snap_name)) {
+        sync_stat.last_synced_snap = std::make_pair(snap_id, snap_name);
+      }
+      double value;
+      if (get_json_real(last_synced_snap, "crawl_duration", &value)) {
+        sync_stat.last_sync_crawl_duration = value;
+      }
+      if (get_json_real(last_synced_snap, "datasync_queue_wait_duration", &value)) {
+        sync_stat.last_sync_datasync_queue_wait_duration = value;
+      }
+      if (get_json_real(last_synced_snap, "sync_duration", &value)) {
+        sync_stat.last_sync_duration = value;
+      }
+      if (get_json_real(last_synced_snap, "sync_time_stamp", &value)) {
+        // set_from_double() casts into __u32; reject non-finite / out-of-range.
+        if (std::isfinite(value) &&
+            value >= 0.0 &&
+            value <= static_cast<double>(std::numeric_limits<uint32_t>::max())) {
+          sync_stat.last_synced.set_from_double(value);
+        } else {
+          derr << ": persisted sync_time_stamp out of range; ignoring" << dendl;
+        }
+      }
+      uint64_t uval;
+      if (get_json_uint64(last_synced_snap, "sync_bytes", &uval)) {
+        sync_stat.last_sync_bytes = uval;
+      }
+      if (get_json_uint64(last_synced_snap, "sync_files", &uval)) {
+        sync_stat.last_sync_files = uval;
       }
     }
-    if (get_json_value(last_synced_snap, "crawl_duration", &v)) {
-      sync_stat.last_sync_crawl_duration = v.get_real();
-    }
-    if (get_json_value(last_synced_snap, "datasync_queue_wait_duration", &v)) {
-      sync_stat.last_sync_datasync_queue_wait_duration = v.get_real();
-    }
-    if (get_json_value(last_synced_snap, "sync_duration", &v)) {
-      sync_stat.last_sync_duration = v.get_real();
-    }
-    if (get_json_value(last_synced_snap, "sync_time_stamp", &v)) {
-      sync_stat.last_synced.set_from_double(v.get_real());
-    }
-    if (get_json_value(last_synced_snap, "sync_bytes", &v)) {
-      sync_stat.last_sync_bytes = v.get_uint64();
-    }
-    if (get_json_value(last_synced_snap, "sync_files", &v)) {
-      sync_stat.last_sync_files = v.get_uint64();
-    }
+  } catch (const std::exception &e) {
+    derr << ": failed to apply persisted sync stat: " << e.what() << dendl;
   }
 }
 

diff --git a/src/tools/cephfs_mirror/Utils.cc b/src/tools/cephfs_mirror/Utils.cc
index e4692b333ff2cc539cd38bfec3b8060e307ac597..eabf79d7cab3a1987bdf9917a488d58e73390743 100644 (file)
--- a/src/tools/cephfs_mirror/Utils.cc
+++ b/src/tools/cephfs_mirror/Utils.cc
@@ -175,5 +175,75 @@ std::string snapshot_path(CephContext *cct, const std::string &dir_root,
   return snapshot_dir_path(cct, dir_root) + "/" + snap_name;
 }
 
+bool get_json_value(const json_spirit::mObject& obj,
+                    const std::string& key,
+                    json_spirit::mValue *val) {
+  auto it = obj.find(key);
+  if (it != obj.end()) {
+    *val = it->second;
+    return true;
+  }
+  return false;
+}
+
+bool get_json_string(const json_spirit::mObject& obj,
+                     const std::string& key,
+                     std::string *val) {
+  json_spirit::mValue v;
+  if (!get_json_value(obj, key, &v)) {
+    return false;
+  }
+  if (v.type() != json_spirit::str_type) {
+    derr << ": persisted sync stat key '" << key
+         << "' has type " << v.type()
+         << " (expected string); ignoring" << dendl;
+    return false;
+  }
+  *val = v.get_str();
+  return true;
+}
+
+bool get_json_uint64(const json_spirit::mObject& obj,
+                     const std::string& key,
+                     uint64_t *val) {
+  json_spirit::mValue v;
+  if (!get_json_value(obj, key, &v)) {
+    return false;
+  }
+  if (v.type() != json_spirit::int_type) {
+    derr << ": persisted sync stat key '" << key
+         << "' has type " << v.type()
+         << " (expected int); ignoring" << dendl;
+    return false;
+  }
+  // json_spirit reports signed and unsigned as int_type; get_uint64()
+  // would cast a negative value to a huge uint64_t.
+  if (!v.is_uint64() && v.get_int64() < 0) {
+    derr << ": persisted sync stat key '" << key
+         << "' has negative value; ignoring" << dendl;
+    return false;
+  }
+  *val = v.get_uint64();
+  return true;
+}
+
+bool get_json_real(const json_spirit::mObject& obj,
+                   const std::string& key,
+                   double *val) {
+  json_spirit::mValue v;
+  if (!get_json_value(obj, key, &v)) {
+    return false;
+  }
+  if (v.type() != json_spirit::int_type &&
+      v.type() != json_spirit::real_type) {
+    derr << ": persisted sync stat key '" << key
+         << "' has type " << v.type()
+         << " (expected int or real); ignoring" << dendl;
+    return false;
+  }
+  *val = v.get_real();
+  return true;
+}
+
 } // namespace mirror
 } // namespace cephfs

diff --git a/src/tools/cephfs_mirror/Utils.h b/src/tools/cephfs_mirror/Utils.h
index 28dc08fd0e94f17a43065d4fdc08cb19909d0aa5..f2dd547f4fe5c250ffd82ee7fc221e3c8e4a116f 100644 (file)
--- a/src/tools/cephfs_mirror/Utils.h
+++ b/src/tools/cephfs_mirror/Utils.h
@@ -7,6 +7,7 @@
 #include <string>
 
 #include "Types.h"
+#include "json_spirit/json_spirit.h"
 
 namespace cephfs {
 namespace mirror {
@@ -23,6 +24,21 @@ int connect(std::string_view client_name, std::string_view cluster_name,
 int mount(RadosRef cluster, const Filesystem &filesystem, bool cross_check_fscid,
           MountRef *mount);
 
+// Typed JSON field getters. Use a local mValue so callers can keep a live
+// copy/reference of a parent object without get_json_value() overwriting it.
+bool get_json_value(const json_spirit::mObject& obj,
+                    const std::string& key,
+                    json_spirit::mValue *val);
+bool get_json_string(const json_spirit::mObject& obj,
+                     const std::string& key,
+                     std::string *val);
+bool get_json_uint64(const json_spirit::mObject& obj,
+                     const std::string& key,
+                     uint64_t *val);
+bool get_json_real(const json_spirit::mObject& obj,
+                   const std::string& key,
+                   double *val);
+
 } // namespace mirror
 } // namespace cephfs