Merge PR ceph#60726 into main

* refs/pull/60726/head:
	qa: add test checking 'journal import' from empty dump file
	cephfs-journal-tool: fix segfault during 'journal import' from invalid dump file

Reviewed-by: Dhairya Parmar <[email protected]>
Reviewed-by: Venky Shankar <[email protected]>

Author: vshankar

qa/tasks/cephfs/test_journal_repair.py

@@ -7,6 +7,7 @@
 import logging
 from textwrap import dedent
 import time
+import tempfile
 
 from teuthology.exceptions import CommandFailedError, ConnectionLostError
 from tasks.cephfs.filesystem import ObjectNotFound, ROOT_INO
@@ -403,3 +404,49 @@ def test_journal_smoke(self):
             "timeout": "1h"
         })
 
+    def test_journal_import_from_empty_dump_file(self):
+        """
+        That the 'journal import' recognizes empty file read and errors out.
+        """
+        fname = tempfile.NamedTemporaryFile(delete=False).name
+        self.mount_a.run_shell(["sudo", "touch", fname], omit_sudo=False)
+        self.fs.fail()
+        import_out = None
+        try:
+            import_out = self.fs.journal_tool(["journal", "import", fname], 0)
+        except CommandFailedError as e:
+            self.mount_a.run_shell(["sudo", "rm", fname], omit_sudo=False)
+            raise RuntimeError(f"Unexpected journal import error: {str(e)}")
+        self.fs.set_joinable()
+        self.fs.wait_for_daemons()
+        try:
+            if import_out.endswith("done."):
+                assert False
+        except AssertionError:
+            raise RuntimeError(f"Unexpected journal-tool result: '{import_out}'")
+        finally:
+            self.mount_a.run_shell(["sudo", "rm", fname], omit_sudo=False)
+
+    def test_journal_import_from_invalid_dump_file(self):
+        """
+        That the 'journal import' recognizes invalid dump file and errors out.
+        """
+        # Create an invalid dump file with partial header
+        fname = tempfile.NamedTemporaryFile(delete=False).name
+        self.mount_a.run_shell(["sudo", "sh", "-c", f'printf "Ceph mds0 journal dump\n\
+        start offset 4194304 (0x400000)\n\
+        length 940 (0x3ac)\nwrite_pos 4194304 (0x400000)\n" > {fname}'], omit_sudo=False)
+        self.fs.fail()
+        try:
+            self.fs.journal_tool(["journal", "import", fname], 0)
+        except CommandFailedError as e:
+            self.fs.set_joinable()
+            self.fs.wait_for_daemons()
+            if e.exitstatus != 234:
+                raise RuntimeError(f"Unexpected journal import error: {str(e)}")
+        else:
+            self.fs.set_joinable()
+            self.fs.wait_for_daemons()
+            raise RuntimeError("Expected journal import to fail")
+        finally:
+            self.mount_a.run_shell(["sudo", "rm", fname], omit_sudo=False)

src/tools/cephfs/Dumper.cc

@@ -224,23 +224,54 @@ int Dumper::undump(const char *dump_file, bool force)
   char buf[HEADER_LEN];
   r = safe_read(fd, buf, sizeof(buf));
   if (r < 0) {
+    derr << "Error reading " << dump_file << dendl;
+    VOID_TEMP_FAILURE_RETRY(::close(fd));
+    return r;
+  } else if (r == 0) {
+    //Empty file read
+    derr << "No-op since empty journal dump file: " << dump_file << dendl;
     VOID_TEMP_FAILURE_RETRY(::close(fd));
     return r;
   }
 
   long long unsigned start, len, write_pos, format, trimmed_pos;
   long unsigned stripe_unit, stripe_count, object_size;
-  sscanf(strstr(buf, "start offset"), "start offset %llu", &start);
-  sscanf(strstr(buf, "length"), "length %llu", &len);
-  sscanf(strstr(buf, "write_pos"), "write_pos %llu", &write_pos);
-  sscanf(strstr(buf, "format"), "format %llu", &format);
+  char *phdr = strstr(buf, "start offset");
+  if (phdr == NULL) {
+      derr  << "Invalid header, no 'start offset' embedded" << dendl;
+      ::close(fd);
+      return -EINVAL;
+  }
+  sscanf(phdr, "start offset %llu", &start);
+  phdr = strstr(buf, "length");
+  if (phdr == NULL) {
+      derr  << "Invalid header, no 'length' embedded" << dendl;
+      ::close(fd);
+      return -EINVAL;
+  }
+  sscanf(phdr, "length %llu", &len);
+  phdr = strstr(buf, "write_pos");
+  if (phdr == NULL) {
+      derr  << "Invalid header, no 'write_pos' embedded" << dendl;
+      ::close(fd);
+      return -EINVAL;
+  }
+  sscanf(phdr, "write_pos %llu", &write_pos);
+  phdr = strstr(buf, "format");
+  if (phdr == NULL) {
+      derr  << "Invalid header, no 'format' embedded" << dendl;
+      ::close(fd);
+      return -EINVAL;
+  }
+  sscanf(phdr, "format %llu", &format);
 
   if (!force) {
     // need to check if fsid match onlien cluster fsid
-    if (strstr(buf, "fsid")) {
+    phdr = strstr(buf, "fsid");
+    if (phdr) {
       uuid_d fsid;
       char fsid_str[40];
-      sscanf(strstr(buf, "fsid"), "fsid %39s", fsid_str);
+      sscanf(phdr, "fsid %39s", fsid_str);
       r = fsid.parse(fsid_str);
       if (!r) {
 	derr  << "Invalid fsid" << dendl;