Support build-ids of other sizes than 16 in UUID::SetFromStringRef

[jarin]

Summary

SBTarget::AddModule currently handles the UUID parameter in a very weird way: UUIDs with more than 16 bytes are trimmed to 16 bytes. On the other hand, shorter-than-16-bytes UUIDs are completely ignored. In this patch, we change the parsing code to handle UUIDs of arbitrary size.

To support arbitrary size UUIDs in SBTarget::AddModule, this patch changes UUID::SetFromStringRef to parse UUIDs of arbitrary length. We subtly change the semantics of SetFromStringRef - SetFromStringRef now only succeeds if the entire input is consumed to prevent some prefix-parsing confusion. This is up for discussion, but I believe this is more consistent - we always return false for invalid UUIDs rather than sometimes truncating to a valid prefix. Also, all the call-sites except the API and interpreter seem to expect to consume the entire input.

This also adds tests for adding existing modules 4-, 16-, and 20-byte build-ids. Finally, we took the liberty of testing the minidump scenario we care about - removing placeholder module from minidump and replacing it with the real module.

[jarin]

lldb/source/API/SBTarget.cpp

1593 ↗

(On Diff #267004)

The four lines above should perhaps be a method of UUID. Something like SetFromStringRef, but for any size. What do you think?

[jankratochvil]

lldb/source/Utility/UUID.cpp

104–105

Now the return type could be bool.

[MaskRay]

I haven't been paying close attention to this patch, but allowing 20 bytes makes sense. In GNU ld and gold, --build-id is --build-id=sha1 (20 bytes). The 3 linkers (plus LLD) don't have a way to produce a build ID longer than 20 bytes.

[jarin]

Change SetFromStringRef to return bool.

[labath]

lldb/source/Utility/UUID.cpp

93–94

I don't think we should be restricting the size here in any way. It is possible to produce larger build-ids already (-Wl,--build-id=0xlonghexstring), and the rest of the UUID class does support arbitrary sizes. Some tools (e.g. llvm-readelf) will choke on them, but let's try to not make lldb one of those tools.

Using super-short uuids is most likely a bad idea, and will result in a lot of collisions, but if someone really wants to use a 3-byte "uuid", I don't see a reason to stop him here.

[jarin]

Removed size restrictions on UUIDs.

[jarin]

lldb/source/Utility/UUID.cpp

93–94

Good idea, that makes the code even simpler.

104–105

I was worried about having to touch even more call sites, but perhaps it is not too bad.

[jarin]

Added a test for missing nibble in UUID.

[labath]

Looks good. Let's just wait a while to see if Fred has any comments. If you haven't already, you can use that time to get commit access. :)

lldb/source/Utility/UUID.cpp

67–68

I guess now obsoletes Fred's D80807.

(Btw, I actually liked how Fred's solution rejects strings which end in a trailing dash.)

[friss]

lldb/source/Utility/UUID.cpp

67–68

Yeah, I didn't have a strong opinion before, but given we want to reject a buffer that isn't parsed completely, I think it's better to reject a buffer ending with a -. As the code would test p.size() anyway, we might as well use p.size() >= 2 as the loop condition.

Otherwise this LGTM. Thanks!

[jarin]

Exclude UUID strings ending with "-".

lldb/include/lldb/Utility/UUID.h

public:
 
   llvm::ArrayRef<uint8_t> GetBytes() const { return m_bytes; }
 
   explicit operator bool() const { return IsValid(); }
   bool IsValid() const { return !m_bytes.empty(); }
 
   std::string GetAsString(llvm::StringRef separator = "-") const;
 
-  size_t SetFromStringRef(llvm::StringRef str, uint32_t num_uuid_bytes = 16);
+  bool SetFromStringRef(llvm::StringRef str);
 
   // Same as SetFromStringRef, but if the resultant UUID is all 0 bytes, set the
   // UUID to invalid.
-  size_t SetFromOptionalStringRef(llvm::StringRef str,
-                                  uint32_t num_uuid_bytes = 16);
+  bool SetFromOptionalStringRef(llvm::StringRef str);
 
   // Decode as many UUID bytes (up to 16) as possible from the C string "cstr"
   // This is used for auto completion where a partial UUID might have been
   // typed in. It
   /// Decode as many UUID bytes (up to 16) as possible from the C
   /// string \a cstr.
   ///
   /// \param[in] str

lldb/source/Interpreter/OptionValueUUID.cpp

Status OptionValueUUID::SetValueFromString(llvm::StringRef value,
   switch (op) {
   case eVarSetOperationClear:
     Clear();
     NotifyValueChanged();
     break;
 
   case eVarSetOperationReplace:
   case eVarSetOperationAssign: {
-    if (m_uuid.SetFromStringRef(value) == 0)
+    if (!m_uuid.SetFromStringRef(value))
       error.SetErrorStringWithFormat("invalid uuid string value '%s'",
                                      value.str().c_str());
     else {
       m_value_was_set = true;
       NotifyValueChanged();
     }
   } break;
 

lldb/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp

if (consume<Token>(Line) != Token::CodeID)
     return llvm::None;
 
   llvm::StringRef Str;
   std::tie(Str, Line) = getToken(Line);
   // If we don't have any text following the code ID (e.g. on linux), we should
   // use this as the UUID. Otherwise, we should revert back to the module ID.
   UUID ID;
   if (Line.trim().empty()) {
-    if (Str.empty() || ID.SetFromStringRef(Str, Str.size() / 2) != Str.size())
+    if (Str.empty() || !ID.SetFromStringRef(Str))
       return llvm::None;
   }
   return InfoRecord(std::move(ID));
 }
 
 llvm::raw_ostream &breakpad::operator<<(llvm::raw_ostream &OS,
                                         const InfoRecord &R) {
   return OS << "INFO CODE_ID " << R.ID.GetAsString();

lldb/source/Plugins/Process/MacOSX-Kernel/CommunicationKDP.cpp

lldb_private::UUID CommunicationKDP::GetUUID() {
   if (m_kernel_version.find("UUID=") == std::string::npos)
     return uuid;
 
   size_t p = m_kernel_version.find("UUID=") + strlen("UUID=");
   std::string uuid_str = m_kernel_version.substr(p, 36);
   if (uuid_str.size() < 32)
     return uuid;
 
-  if (uuid.SetFromStringRef(uuid_str) == 0) {
+  if (!uuid.SetFromStringRef(uuid_str)) {
     UUID invalid_uuid;
     return invalid_uuid;
   }
 
   return uuid;
 }
 
 bool CommunicationKDP::RemoteIsEFI() {

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

bool GDBRemoteCommunicationClient::GetModuleInfo(
   module_spec.Clear();
   module_spec.GetFileSpec() = module_file_spec;
 
   while (response.GetNameColonValue(name, value)) {
     if (name == "uuid" || name == "md5") {
       StringExtractor extractor(value);
       std::string uuid;
       extractor.GetHexByteString(uuid);
-      module_spec.GetUUID().SetFromStringRef(uuid, uuid.size() / 2);
+      module_spec.GetUUID().SetFromStringRef(uuid);
     } else if (name == "triple") {
       StringExtractor extractor(value);
       std::string triple;
       extractor.GetHexByteString(triple);
       module_spec.GetArchitecture().SetTriple(triple.c_str());
     } else if (name == "file_offset") {
       uint64_t ival = 0;
       if (!value.getAsInteger(16, ival))
ParseModuleSpec(StructuredData::Dictionary *dict) {
   if (!dict)
     return llvm::None;
 
   llvm::StringRef string;
   uint64_t integer;
 
   if (!dict->GetValueForKeyAsString("uuid", string))
     return llvm::None;
-  if (result.GetUUID().SetFromStringRef(string, string.size() / 2) !=
-      string.size())
+  if (!result.GetUUID().SetFromStringRef(string))
     return llvm::None;
 
   if (!dict->GetValueForKeyAsInteger("file_offset", integer))
     return llvm::None;
   result.SetObjectOffset(integer);
 
   if (!dict->GetValueForKeyAsInteger("file_size", integer))
     return llvm::None;

lldb/source/Utility/UUID.cpp

static inline int xdigit_to_int(char ch) {
   return ch - '0';
 }
 
 llvm::StringRef
 UUID::DecodeUUIDBytesFromString(llvm::StringRef p,
                                 llvm::SmallVectorImpl<uint8_t> &uuid_bytes,
                                 uint32_t num_uuid_bytes) {
   uuid_bytes.clear();
   while (!p.empty()) {
     if (isxdigit(p[0]) && isxdigit(p[1])) {
       int hi_nibble = xdigit_to_int(p[0]);
       int lo_nibble = xdigit_to_int(p[1]);
       // Translate the two hex nibble characters into a byte
       uuid_bytes.push_back((hi_nibble << 4) + lo_nibble);
 
       // Skip both hex digits
       p = p.drop_front(2);
 
       // Increment the byte that we are decoding within the UUID value and
       // break out if we are done
       if (uuid_bytes.size() == num_uuid_bytes)
         break;
     } else if (p.front() == '-') {
       // Skip dashes
       p = p.drop_front();
     } else {
       // UUID values can only consist of hex characters and '-' chars
       break;
     }
   }
   return p;
 }
 
-size_t UUID::SetFromStringRef(llvm::StringRef str, uint32_t num_uuid_bytes) {
+bool UUID::SetFromStringRef(llvm::StringRef str) {
+  const size_t max_uuid_size = 20;
+  const size_t min_uuid_size = 4;
+
   llvm::StringRef p = str;
 
   // Skip leading whitespace characters
   p = p.ltrim();
 
-  llvm::SmallVector<uint8_t, 20> bytes;
+  llvm::SmallVector<uint8_t, max_uuid_size> bytes;
   llvm::StringRef rest =
-      UUID::DecodeUUIDBytesFromString(p, bytes, num_uuid_bytes);
+      UUID::DecodeUUIDBytesFromString(p, bytes, max_uuid_size);
 
-  // If we successfully decoded a UUID, return the amount of characters that
-  // were consumed
-  if (bytes.size() == num_uuid_bytes) {
-    *this = fromData(bytes);
-    return str.size() - rest.size();
-  }
+  // Return false if we could not consume the entire string or if the parsed
+  // UUID is too short.
+  if (!rest.empty() || bytes.size() < min_uuid_size)
+    return false;
 
-  // Else return zero to indicate we were not able to parse a UUID value
-  return 0;
+  *this = fromData(bytes);
+  return true;
 }
 
-size_t UUID::SetFromOptionalStringRef(llvm::StringRef str, 
-                                      uint32_t num_uuid_bytes) {
-  size_t num_chars_consumed = SetFromStringRef(str, num_uuid_bytes);
-  if (num_chars_consumed) {
+bool UUID::SetFromOptionalStringRef(llvm::StringRef str) {
+  bool result = SetFromStringRef(str);
+  if (result) {
     if (llvm::all_of(m_bytes, [](uint8_t b) { return b == 0; }))
         Clear();
   }
-  
-  return num_chars_consumed;
+  return result;
 }
-

lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py

def test_relative_module_name(self):
         self.addTearDownHook(lambda: os.chdir(old_cwd))
         os.chdir(self.getBuildDir())
         name = "file-with-a-name-unlikely-to-exist-in-the-current-directory.so"
         open(name, "a").close()
         modules = self.get_minidump_modules(
                 self.getSourcePath("relative_module_name.yaml"))
         self.assertEqual(1, len(modules))
         self.verify_module(modules[0], name, None)
+
+    def test_add_module_build_id_16(self):
+        """
+            Test that adding module with 16 byte UUID returns the existing
+            module or fails.
+        """
+        modules = self.get_minidump_modules("linux-arm-uuids-elf-build-id-16.yaml")
+        self.assertEqual(2, len(modules))
+
+        # Add the existing modules.
+        self.assertEqual(modules[0], self.target.AddModule(
+            "/some/local/a", "", "01020304-0506-0708-090A-0B0C0D0E0F10"))
+        self.assertEqual(modules[1], self.target.AddModule(
+            "/some/local/b", "", "0A141E28-323C-4650-5A64-6E78828C96A0"))
+
+        # Adding modules with non-existing UUID should fail.
+        self.assertFalse(
+            self.target.AddModule(
+                "a", "", "12345678-1234-1234-1234-123456789ABC").IsValid())
+        self.assertFalse(
+            self.target.AddModule(
+                "a", "", "01020304-0506-0708-090A-0B0C0D0E0F10-12345678").IsValid())
+
+    def test_add_module_build_id_20(self):
+        """
+            Test that adding module with 20 byte UUID returns the existing
+            module or fails.
+        """
+        modules = self.get_minidump_modules("linux-arm-uuids-elf-build-id-20.yaml")
+
+        # Add the existing modules.
+        self.assertEqual(modules[0], self.target.AddModule(
+            "/some/local/a", "", "01020304-0506-0708-090A-0B0C0D0E0F10-11121314"))
+        self.assertEqual(modules[1], self.target.AddModule(
+            "/some/local/b", "", "0A141E28-323C-4650-5A64-6E78828C96A0-AAB4BEC8"))
+
+        # Adding modules with non-existing UUID should fail.
+        self.assertFalse(
+            self.target.AddModule(
+                "a", "", "01020304-0506-0708-090A-0B0C0D0E0F10").IsValid())
+        self.assertFalse(
+            self.target.AddModule(
+                "a", "", "01020304-0506-0708-090A-0B0C0D0E0F10-12345678").IsValid())
+
+    def test_add_module_build_id_4(self):
+        """
+            Test that adding module with 4 byte UUID returns the existing
+            module or fails.
+        """
+        modules = self.get_minidump_modules("linux-arm-uuids-elf-build-id-4.yaml")
+
+        # Add the existing modules.
+        self.assertEqual(modules[0], self.target.AddModule(
+            "/some/local/a.so", "", "01020304"))
+        self.assertEqual(modules[1], self.target.AddModule(
+            "/some/local/b.so", "", "0A141E28"))
+
+        # Adding modules with non-existing UUID should fail.
+        self.assertFalse(
+            self.target.AddModule(
+                "a", "", "01020304-0506-0708-090A-0B0C0D0E0F10").IsValid())
+        self.assertFalse(self.target.AddModule("a", "", "01020305").IsValid())
+
+    def test_remove_placeholder_add_real_module(self):
+        """
+            Test that removing a placeholder module and adding back the real
+            module succeeds.
+        """
+        so_path = self.getBuildArtifact("libuuidmatch.so")
+        self.yaml2obj("libuuidmatch.yaml", so_path)
+        modules = self.get_minidump_modules("linux-arm-uuids-match.yaml")
+
+        uuid = "7295E17C-6668-9E05-CBB5-DEE5003865D5-5267C116";
+        self.assertEqual(1, len(modules))
+        self.verify_module(modules[0], "/target/path/libuuidmatch.so",uuid)
+
+        self.target.RemoveModule(modules[0])
+        new_module = self.target.AddModule(so_path, "", uuid)
+
+        self.verify_module(new_module, so_path, uuid)
+        self.assertEqual(new_module, self.target.modules[0])
+        self.assertEqual(1, len(self.target.modules))

lldb/test/API/functionalities/postmortem/minidump-new/linux-arm-uuids-elf-build-id-4.yaml

+--- !minidump
+Streams:         
+  - Type:            SystemInfo
+    Processor Arch:  ARM
+    Platform ID:     Linux
+    CSD Version:     '15E216'
+    CPU:             
+      CPUID:           0x00000000
+  - Type:            ModuleList
+    Modules:         
+      - Base of Image:   0x0000000000001000
+        Size of Image:   0x00001000
+        Module Name:     '/tmp/a.so'
+        CodeView Record: 4C45704201020304
+      - Base of Image:   0x0000000000001000
+        Size of Image:   0x00001000
+        Module Name:     '/tmp/b.so'
+        CodeView Record: 4C4570420A141E28
+...

lldb/test/API/functionalities/postmortem/minidump-new/linux-arm-uuids-match.yaml

+--- !minidump
+Streams:         
+  - Type:            SystemInfo
+    Processor Arch:  ARM
+    Platform ID:     Linux
+    CSD Version:     '15E216'
+    CPU:             
+      CPUID:           0x00000000
+  - Type:            ModuleList
+    Modules:         
+      - Base of Image:   0x0000000000001000
+        Size of Image:   0x00001000
+        Module Name:     '/target/path/libuuidmatch.so'
+        CodeView Record: 525344537295E17C66689E05CBB5DEE5003865D55267C116
+...

lldb/unittests/ObjectFile/ELF/TestObjectFileELF.cpp

 */
 TEST_F(ObjectFileELFTest, GetModuleSpecifications_EarlySectionHeaders) {
   std::string SO = GetInputFilePath("early-section-headers.so");
   ModuleSpecList Specs;
   ASSERT_EQ(1u, ObjectFile::GetModuleSpecifications(FileSpec(SO), 0, 0, Specs));
   ModuleSpec Spec;
   ASSERT_TRUE(Specs.GetModuleSpecAtIndex(0, Spec)) ;
   UUID Uuid;
-  Uuid.SetFromStringRef("1b8a73ac238390e32a7ff4ac8ebe4d6a41ecf5c9", 20);
+  Uuid.SetFromStringRef("1b8a73ac238390e32a7ff4ac8ebe4d6a41ecf5c9");
   EXPECT_EQ(Spec.GetUUID(), Uuid);
 }
 
 TEST_F(ObjectFileELFTest, GetSymtab_NoSymEntryPointArmThumbAddressClass) {
   /*
   // nosym-entrypoint-arm-thumb.s
   .thumb_func
   _start:
)");
 
   ModuleSpec spec{FileSpec(ExpectedFile->name())};
   spec.GetSymbolFileSpec().SetFile(ExpectedFile->name(),
                                    FileSpec::Style::native);
   auto module_sp = std::make_shared<Module>(spec);
 
   auto entry_point_addr = module_sp->GetObjectFile()->GetEntryPointAddress();
   ASSERT_EQ(entry_point_addr.GetAddressClass(), AddressClass::eCode);
-}
+}
 

lldb/unittests/Target/ModuleCacheTest.cpp

 };
 }
 
 static const char dummy_hostname[] = "dummy_hostname";
 static const char dummy_remote_dir[] = "bin";
 static const char module_name[] = "TestModule.so";
 static const char module_uuid[] =
     "F4E7E991-9B61-6AD4-0073-561AC3D9FA10-C043A476";
-static const uint32_t uuid_bytes = 20;
 static const size_t module_size = 5602;
 
 static FileSpec GetDummyRemotePath() {
   FileSpec fs("/", FileSpec::Style::posix);
   fs.AppendPathComponent(dummy_remote_dir);
   fs.AppendPathComponent(module_name);
   return fs;
 }
static void VerifyDiskState(const FileSpec &cache_dir, const char *hostname) {
   EXPECT_EQ(module_size, FileSystem::Instance().GetByteSize(sysroot_view));
 }
 
 void ModuleCacheTest::TryGetAndPut(const FileSpec &cache_dir,
                                    const char *hostname, bool expect_download) {
   ModuleCache mc;
   ModuleSpec module_spec;
   module_spec.GetFileSpec() = GetDummyRemotePath();
-  module_spec.GetUUID().SetFromStringRef(module_uuid, uuid_bytes);
+  module_spec.GetUUID().SetFromStringRef(module_uuid);
   module_spec.SetObjectSize(module_size);
   ModuleSP module_sp;
   bool did_create;
   bool download_called = false;
 
   Status error = mc.GetAndPut(
       cache_dir, hostname, module_spec,
       [&download_called, this](const ModuleSpec &module_spec,

lldb/unittests/Utility/UUIDTest.cpp

TEST(UUIDTest, Validity) {
   UUID a16 = UUID::fromData(zeroes.data(), 16);
   UUID a20 = UUID::fromData(zeroes.data(), 20);
   UUID a16_0 = UUID::fromOptionalData(zeroes.data(), 16);
   UUID a20_0 = UUID::fromOptionalData(zeroes.data(), 20);
   UUID from_str;
   from_str.SetFromStringRef("00000000-0000-0000-0000-000000000000");
   UUID opt_from_str;
   opt_from_str.SetFromOptionalStringRef("00000000-0000-0000-0000-000000000000");
-  
   EXPECT_FALSE(empty);
   EXPECT_TRUE(a16);
   EXPECT_TRUE(a20);
   EXPECT_TRUE(from_str);
   EXPECT_FALSE(a16_0);
   EXPECT_FALSE(a20_0);
   EXPECT_FALSE(opt_from_str);
 }
 
 TEST(UUIDTest, SetFromStringRef) {
   UUID u;
-  EXPECT_EQ(32u, u.SetFromStringRef("404142434445464748494a4b4c4d4e4f"));
+  EXPECT_TRUE(u.SetFromStringRef("404142434445464748494a4b4c4d4e4f"));
   EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNO", 16), u);
 
-  EXPECT_EQ(36u, u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f"));
+  EXPECT_TRUE(u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f"));
   EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNO", 16), u);
 
-  EXPECT_EQ(45u, u.SetFromStringRef(
-                     "40-41-42-43-4445464748494a4b4c4d4e4f-50515253", 20));
+  EXPECT_TRUE(
+      u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f-50515253"));
   EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNOPQRS", 20), u);
 
-  EXPECT_EQ(0u, u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f", 20));
-  EXPECT_EQ(0u, u.SetFromStringRef("40xxxxx"));
-  EXPECT_EQ(0u, u.SetFromStringRef(""));
-  EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNOPQRS", 20), u)
+  EXPECT_TRUE(u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f"));
+
+  EXPECT_FALSE(u.SetFromStringRef("40xxxxx"));
+  EXPECT_FALSE(u.SetFromStringRef(""));
+  EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNO", 16), u)
       << "uuid was changed by failed parse calls";
 
-  EXPECT_EQ(
-      32u, u.SetFromStringRef("404142434445464748494a4b4c4d4e4f-50515253", 16));
-  EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNO", 16), u);
+  EXPECT_TRUE(u.SetFromStringRef("404142434445464748494a4b4c4d4e4f-50515253"));
+  EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNOPQRS", 20), u);
+
+  EXPECT_TRUE(u.SetFromStringRef("40414243"));
+  EXPECT_EQ(UUID::fromData("@ABCD", 4), u);
 }
 
 TEST(UUIDTest, StringConverion) {
   EXPECT_EQ("40414243", UUID::fromData("@ABC", 4).GetAsString());
   EXPECT_EQ("40414243-4445-4647", UUID::fromData("@ABCDEFG", 8).GetAsString());
   EXPECT_EQ("40414243-4445-4647-4849-4A4B",
             UUID::fromData("@ABCDEFGHIJK", 12).GetAsString());
   EXPECT_EQ("40414243-4445-4647-4849-4A4B4C4D4E4F",
             UUID::fromData("@ABCDEFGHIJKLMNO", 16).GetAsString());
   EXPECT_EQ("40414243-4445-4647-4849-4A4B4C4D4E4F-50515253",
             UUID::fromData("@ABCDEFGHIJKLMNOPQRS", 20).GetAsString());
 }