[lldb] [Mach-O] don't strip the end of the "kern ver str" LC_NOTE

[jasonmolenda]

The "kern ver str" LC_NOTE gives lldb a kernel version string -- with a UUID and/or a load address (stext) to load it at. The LC_NOTE specifies a size of the identifier string in bytes. In ObjectFileMachO::GetIdentifierString, I copy that number of bytes into a std::string, and in case there were additional nul characters at the end of the sting for padding reasons, I tried to shrink the std::string to not include these extra nul's.

However, I did this resizing without handling the case of an empty identifier string. I don't know why any corefile creator would do that, but of course at least one does. This patch removes the resizing altogether; I was solving something that hasn't ever shown to be a problem. I also added a test case for this, to check that lldb doesn't crash when given one of these corefiles.

rdar://120390199

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jason Molenda (jasonmolenda)

Changes

The "kern ver str" LC_NOTE gives lldb a kernel version string -- with a UUID and/or a load address (stext) to load it at. The LC_NOTE specifies a size of the identifier string in bytes. In ObjectFileMachO::GetIdentifierString, I copy that number of bytes into a std::string, and in case there were additional nul characters at the end of the sting for padding reasons, I tried to shrink the std::string to not include these extra nul's.

However, I did this resizing without handling the case of an empty identifier string. I don't know why any corefile creator would do that, but of course at least one does. This patch removes the resizing altogether; I was solving something that hasn't ever shown to be a problem. I also added a test case for this, to check that lldb doesn't crash when given one of these corefiles.

rdar://120390199

---

Full diff: https://github.com/llvm/llvm-project/pull/77538.diff

3 Files Affected:

• (modified) lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp (-4)
• (modified) lldb/test/API/macosx/lc-note/firmware-corefile/TestFirmwareCorefiles.py (+23-1)
• (modified) lldb/test/API/macosx/lc-note/firmware-corefile/create-empty-corefile.cpp (+17-8)

diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index 182a9f2afaeb2e..d7a2846200fcaf 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -5467,8 +5467,6 @@ std::string ObjectFileMachO::GetIdentifierString() {
           uint32_t strsize = payload_size - sizeof(uint32_t);
           std::string result(strsize, '\0');
           m_data.CopyData(payload_offset, strsize, result.data());
-          while (result.back() == '\0')
-            result.resize(result.size() - 1);
           LLDB_LOGF(log, "LC_NOTE 'kern ver str' found with text '%s'",
                     result.c_str());
           return result;
@@ -5488,8 +5486,6 @@ std::string ObjectFileMachO::GetIdentifierString() {
         std::string result(ident_command.cmdsize, '\0');
         if (m_data.CopyData(offset, ident_command.cmdsize, result.data()) ==
             ident_command.cmdsize) {
-          while (result.back() == '\0')
-            result.resize(result.size() - 1);
           LLDB_LOGF(log, "LC_IDENT found with text '%s'", result.c_str());
           return result;
         }
diff --git a/lldb/test/API/macosx/lc-note/firmware-corefile/TestFirmwareCorefiles.py b/lldb/test/API/macosx/lc-note/firmware-corefile/TestFirmwareCorefiles.py
index 9fd12c3ba49c29..b9d2055e83a56c 100644
--- a/lldb/test/API/macosx/lc-note/firmware-corefile/TestFirmwareCorefiles.py
+++ b/lldb/test/API/macosx/lc-note/firmware-corefile/TestFirmwareCorefiles.py
@@ -24,6 +24,9 @@ def test_lc_note_version_string(self):
         aout_exe_basename = "a.out"
         aout_exe = self.getBuildArtifact(aout_exe_basename)
         verstr_corefile = self.getBuildArtifact("verstr.core")
+        verstr_corefile_invalid_ident = self.getBuildArtifact(
+            "verstr-invalid-ident.core"
+        )
         verstr_corefile_addr = self.getBuildArtifact("verstr-addr.core")
         create_corefile = self.getBuildArtifact("create-empty-corefile")
         slide = 0x70000000000
@@ -36,6 +39,14 @@ def test_lc_note_version_string(self):
             + " 0xffffffffffffffff 0xffffffffffffffff",
             shell=True,
         )
+        call(
+            create_corefile
+            + " version-string "
+            + verstr_corefile_invalid_ident
+            + ' "" '
+            + "0xffffffffffffffff 0xffffffffffffffff",
+            shell=True,
+        )
         call(
             create_corefile
             + " version-string "
@@ -71,7 +82,18 @@ def test_lc_note_version_string(self):
         self.assertEqual(fspec.GetFilename(), aout_exe_basename)
         self.dbg.DeleteTarget(target)
 
-        # Second, try the "kern ver str" corefile where it loads at an address
+        # Second, try the "kern ver str" corefile which has an invalid ident,
+        # make sure we don't crash.
+        target = self.dbg.CreateTarget("")
+        err = lldb.SBError()
+        if self.TraceOn():
+            self.runCmd(
+                "script print('loading corefile %s')" % verstr_corefile_invalid_ident
+            )
+        process = target.LoadCore(verstr_corefile_invalid_ident)
+        self.assertEqual(process.IsValid(), True)
+
+        # Third, try the "kern ver str" corefile where it loads at an address
         target = self.dbg.CreateTarget("")
         err = lldb.SBError()
         if self.TraceOn():
diff --git a/lldb/test/API/macosx/lc-note/firmware-corefile/create-empty-corefile.cpp b/lldb/test/API/macosx/lc-note/firmware-corefile/create-empty-corefile.cpp
index 8bd6aaabecd636..d7c2d422412e42 100644
--- a/lldb/test/API/macosx/lc-note/firmware-corefile/create-empty-corefile.cpp
+++ b/lldb/test/API/macosx/lc-note/firmware-corefile/create-empty-corefile.cpp
@@ -86,14 +86,16 @@ std::vector<uint8_t> lc_thread_load_command(cpu_type_t cputype) {
 void add_lc_note_kern_ver_str_load_command(
     std::vector<std::vector<uint8_t>> &loadcmds, std::vector<uint8_t> &payload,
     int payload_file_offset, std::string uuid, uint64_t address) {
-  std::string ident = "EFI UUID=";
-  ident += uuid;
-
-  if (address != 0xffffffffffffffff) {
-    ident += "; stext=";
-    char buf[24];
-    sprintf(buf, "0x%" PRIx64, address);
-    ident += buf;
+  std::string ident;
+  if (!uuid.empty()) {
+    ident = "EFI UUID=";
+    ident += uuid;
+    if (address != 0xffffffffffffffff) {
+      ident += "; stext=";
+      char buf[24];
+      sprintf(buf, "0x%" PRIx64, address);
+      ident += buf;
+    }
   }
 
   std::vector<uint8_t> loadcmd_data;
@@ -187,6 +189,9 @@ void add_lc_segment(std::vector<std::vector<uint8_t>> &loadcmds,
 
 std::string get_uuid_from_binary(const char *fn, cpu_type_t &cputype,
                                  cpu_subtype_t &cpusubtype) {
+  if (strlen(fn) == 0)
+    return {};
+
   FILE *f = fopen(fn, "r");
   if (f == nullptr) {
     fprintf(stderr, "Unable to open binary '%s' to get uuid\n", fn);
@@ -295,6 +300,10 @@ int main(int argc, char **argv) {
     fprintf(stderr, "an LC_NOTE 'main bin spec' load command without an "
                     "address specified, depending on\n");
     fprintf(stderr, "whether the 1st arg is version-string or main-bin-spec\n");
+    fprintf(stderr, "\nan LC_NOTE 'kern ver str' with no binary provided "
+                    "(empty string filename) to get a UUID\n");
+    fprintf(stderr, "means an empty 'kern ver str' will be written, an invalid "
+                    "LC_NOTE that lldb should handle.\n");
     exit(1);
   }
   if (strcmp(argv[1], "version-string") != 0 &&