[lldb] Fix out of bounds read in DataExtractor::GetCStr and add unit test that function.

[teemperor]

Summary

The if (*cstr_end == '\0') in the previous code checked if the previous loop terminated because it
found a null terminator or because it reached the end of the data. However, in the case that we hit
the end of the data before finding a null terminator, cstr_end points behind the last byte in our
data and *cstr_end reads the memory behind the array (which may be uninitialised)

This patch just rewrites that function use std::find and adds the relevant unit tests.

lldb/source/Utility/DataExtractor.cpp

 // Extracts a variable length NULL terminated C string from the data at the
 // offset pointed to by "offset_ptr".  The "offset_ptr" will be updated with
 // the offset of the byte that follows the NULL terminator byte.
 //
 // If the offset pointed to by "offset_ptr" is out of bounds, or if "length" is
 // non-zero and there aren't enough available bytes, nullptr will be returned
 // and "offset_ptr" will not be updated.
 const char *DataExtractor::GetCStr(offset_t *offset_ptr) const {
-  const char *cstr = reinterpret_cast<const char *>(PeekData(*offset_ptr, 1));
-  if (cstr) {
-    const char *cstr_end = cstr;
-    const char *end = reinterpret_cast<const char *>(m_end);
-    while (cstr_end < end && *cstr_end)
-      ++cstr_end;
+  const char *start = reinterpret_cast<const char *>(PeekData(*offset_ptr, 1));
+  // Already at the end of the data.
+  if (!start)
+    return nullptr;
+
 
-    // Now we are either at the end of the data or we point to the
-    // NULL C string terminator with cstr_end...
-    if (*cstr_end == '\0') {
-      // Advance the offset with one extra byte for the NULL terminator
-      *offset_ptr += (cstr_end - cstr + 1);
-      return cstr;
-    }
+  // Check all bytes for a null terminator that terminates a C string.
+  const char *terminator_or_end = std::find(start, end, '\0');
 
-    // We reached the end of the data without finding a NULL C string
-    // terminator. Fall through and return nullptr otherwise anyone that would
-    // have used the result as a C string can wander into unknown memory...
-  }
+  // We didn't find a null terminator, so return nullptr to indicate that there
+  // is no valid C string at that offset.
+  if (terminator_or_end == end)
     return nullptr;
+
+  // Update offset_ptr for the caller to point to the data behind the
+  // terminator (which is 1 byte long).
+  *offset_ptr += (terminator_or_end - start + 1UL);
+  return start;
 }
 
 // Extracts a NULL terminated C string from the fixed length field of length
 // "len" at the offset pointed to by "offset_ptr". The "offset_ptr" will be
 // updated with the offset of the byte that follows the fixed length field.
 //
 // If the offset pointed to by "offset_ptr" is out of bounds, or if the offset
 // plus the length of the field is out of bounds, or if the field does not

lldb/unittests/Utility/DataExtractorTest.cpp

TEST(DataExtractorTest, PeekData) {
   EXPECT_EQ(buffer + 2, E.PeekData(2, 0));
   EXPECT_EQ(buffer + 2, E.PeekData(2, 2));
   EXPECT_EQ(nullptr, E.PeekData(2, 3));
 
   EXPECT_EQ(buffer + 4, E.PeekData(4, 0));
   EXPECT_EQ(nullptr, E.PeekData(4, 1));
 }
 
+TEST(DataExtractorTest, GetCStr) {
+  uint8_t buffer[] = {'X', 'f', 'o', 'o', '\0'};
+  DataExtractor E(buffer, sizeof buffer, lldb::eByteOrderLittle, 4);
+
+  lldb::offset_t offset = 1;
+  EXPECT_STREQ("foo", E.GetCStr(&offset));
+  EXPECT_EQ(5U, offset);
+}
+
+TEST(DataExtractorTest, GetCStrEmpty) {
+  uint8_t buffer[] = {'X', '\0'};
+  DataExtractor E(buffer, sizeof buffer, lldb::eByteOrderLittle, 4);
+
+  lldb::offset_t offset = 1;
+  EXPECT_STREQ("", E.GetCStr(&offset));
+  EXPECT_EQ(2U, offset);
+}
+
+TEST(DataExtractorTest, GetCStrUnterminated) {
+  uint8_t buffer[] = {'X', 'f', 'o', 'o'};
+  DataExtractor E(buffer, sizeof buffer, lldb::eByteOrderLittle, 4);
+
+  lldb::offset_t offset = 1;
+  EXPECT_EQ(nullptr, E.GetCStr(&offset));
+  EXPECT_EQ(1U, offset);
+}
+
+TEST(DataExtractorTest, GetCStrAtEnd) {
+  uint8_t buffer[] = {'X'};
+  DataExtractor E(buffer, sizeof buffer, lldb::eByteOrderLittle, 4);
+
+  lldb::offset_t offset = 1;
+  EXPECT_EQ(nullptr, E.GetCStr(&offset));
+  EXPECT_EQ(1U, offset);
+}
+
+TEST(DataExtractorTest, GetCStrAtNullOffset) {
+  uint8_t buffer[] = {'f', 'o', 'o', '\0'};
+  DataExtractor E(buffer, sizeof buffer, lldb::eByteOrderLittle, 4);
+
+  lldb::offset_t offset = 0;
+  EXPECT_STREQ("foo", E.GetCStr(&offset));
+  EXPECT_EQ(4U, offset);
+}
+
 TEST(DataExtractorTest, GetMaxU64) {
   uint8_t buffer[] = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08};
   DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
                    sizeof(void *));
   DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig, sizeof(void *));
 
   lldb::offset_t offset;