[Support] Fix error handling in DataExtractor::get[US]LEB128

labath · labath · commit bb6d0b8e7b0d · 2019-06-24T09:11:24.000Z
Summary: These functions are documented as not modifying the offset argument if the extraction fails (just like other DataExtractor functions). However, while reviewing D63591 we discovered that this is not the case -- if the function reaches the end of the data buffer, it will just return the value parsed until that point and set offset to point to the end of the buffer. This fixes the functions to act as advertised, and adds a regression test. Reviewers: dblaikie, probinson, bkramer Subscribers: kristina, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D63645 llvm-svn: 364169
diff --git a/llvm/lib/Support/DataExtractor.cpp b/llvm/lib/Support/DataExtractor.cpp
@@ -157,12 +157,12 @@ uint64_t DataExtractor::getULEB128(uint32_t *offset_ptr) const {
     byte = Data[offset++];
     result |= uint64_t(byte & 0x7f) << shift;
     shift += 7;
-    if ((byte & 0x80) == 0)
-      break;
+    if ((byte & 0x80) == 0) {
+      *offset_ptr = offset;
+      return result;
+    }
   }
-
-  *offset_ptr = offset;
-  return result;
+  return 0;
 }
 
 int64_t DataExtractor::getSLEB128(uint32_t *offset_ptr) const {
@@ -178,14 +178,14 @@ int64_t DataExtractor::getSLEB128(uint32_t *offset_ptr) const {
     byte = Data[offset++];
     result |= uint64_t(byte & 0x7f) << shift;
     shift += 7;
-    if ((byte & 0x80) == 0)
-      break;
+    if ((byte & 0x80) == 0) {
+      // Sign bit of byte is 2nd high order bit (0x40)
+      if (shift < 64 && (byte & 0x40))
+        result |= -(1ULL << shift);
+
+      *offset_ptr = offset;
+      return result;
+    }
   }
-
-  // Sign bit of byte is 2nd high order bit (0x40)
-  if (shift < 64 && (byte & 0x40))
-    result |= -(1ULL << shift);
-
-  *offset_ptr = offset;
-  return result;
+  return 0;
 }
diff --git a/llvm/unittests/Support/DataExtractorTest.cpp b/llvm/unittests/Support/DataExtractorTest.cpp
@@ -116,4 +116,14 @@ TEST(DataExtractorTest, LEB128) {
   EXPECT_EQ(8U, offset);
 }
 
+TEST(DataExtractorTest, LEB128_error) {
+  DataExtractor DE(StringRef("\x81"), false, 8);
+  uint32_t Offset = 0;
+  EXPECT_EQ(0U, DE.getULEB128(&Offset));
+  EXPECT_EQ(0U, Offset);
+
+  Offset = 0;
+  EXPECT_EQ(0U, DE.getSLEB128(&Offset));
+  EXPECT_EQ(0U, Offset);
+}
 }

Original file line number	Diff line number	Diff line change
`@@ -116,4 +116,14 @@ TEST(DataExtractorTest, LEB128) {`
`116`	`116`	`EXPECT_EQ(8U, offset);`
`117`	`117`	`}`
`118`	`118`
	`119`	`+TEST(DataExtractorTest, LEB128_error) {`
	`120`	`+ DataExtractor DE(StringRef("\x81"), false, 8);`
	`121`	`+ uint32_t Offset = 0;`
	`122`	`+ EXPECT_EQ(0U, DE.getULEB128(&Offset));`
	`123`	`+ EXPECT_EQ(0U, Offset);`
	`124`	`+`
	`125`	`+ Offset = 0;`
	`126`	`+ EXPECT_EQ(0U, DE.getSLEB128(&Offset));`
	`127`	`+ EXPECT_EQ(0U, Offset);`
	`128`	`+}`
`119`	`129`	`}`