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

[Raphael Isemann via lldb-commits]

Author: teemperor
Date: Thu Oct 10 04:15:38 2019
New Revision: 374311

URL: http://llvm.org/viewvc/llvm-project?rev=374311&view=rev
Log:
[lldb] Fix out of bounds read in DataExtractor::GetCStr and add unit test that function.

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.

Reviewers: labath

Reviewed By: labath

Subscribers: abidh, JDevlieghere, lldb-commits

Tags: #lldb

Differential Revision: https://reviews.llvm.org/D68773

Modified:
    lldb/trunk/source/Utility/DataExtractor.cpp
    lldb/trunk/unittests/Utility/DataExtractorTest.cpp

Modified: lldb/trunk/source/Utility/DataExtractor.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Utility/DataExtractor.cpp?rev=374311&r1=374310&r2=374311&view=diff
==============================================================================
--- lldb/trunk/source/Utility/DataExtractor.cpp (original)
+++ lldb/trunk/source/Utility/DataExtractor.cpp Thu Oct 10 04:15:38 2019
@@ -816,26 +816,25 @@ DataExtractor::CopyByteOrderedData(offse
 // 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;
-    }
+  const char *end = reinterpret_cast<const char *>(m_end);
 
-    // 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...
-  }
-  return nullptr;
+  // Check all bytes for a null terminator that terminates a C string.
+  const char *terminator_or_end = std::find(start, end, '\0');
+
+  // 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

Modified: lldb/trunk/unittests/Utility/DataExtractorTest.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Utility/DataExtractorTest.cpp?rev=374311&r1=374310&r2=374311&view=diff
==============================================================================
--- lldb/trunk/unittests/Utility/DataExtractorTest.cpp (original)
+++ lldb/trunk/unittests/Utility/DataExtractorTest.cpp Thu Oct 10 04:15:38 2019
@@ -49,6 +49,51 @@ TEST(DataExtractorTest, PeekData) {
   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,