[Lldb-commits] [PATCH] D73913: [lldb/DataExtractor] Fix UB shift in GetMaxS64Bitfield

[Vedant Kumar via Phabricator via lldb-commits]

vsk created this revision.
vsk added reviewers: labath, JDevlieghere, teemperor.

DataExtractor::GetMaxS64Bitfield performs a shift with UB in order to
construct a bitmask when bitfield_bit_size is 64. The current
implementation actually does “work” in this case, because the assumption
that the shift result is 0 holds, and 0 minus 1 gives the all-ones value
(the correct mask). However, the more readable/maintainable approach
might be to use an off-the-shelf UB-free helper.

Fixes a UBSan issue:

  "col" : 37,
  "description" : "invalid-shift-exponent",
  "filename" : "/Users/vsk/src/llvm-project-master/lldb/source/Utility/DataExtractor.cpp",
  "instrumentation_class" : "UndefinedBehaviorSanitizer",
  "line" : 615,
  "memory_address" : 0,
  "summary" : "Shift exponent 64 is too large for 64-bit type 'uint64_t' (aka 'unsigned long long')",

rdar://59117758


https://reviews.llvm.org/D73913

Files:
  lldb/include/lldb/Utility/DataExtractor.h
  lldb/source/Utility/DataExtractor.cpp
  lldb/unittests/Utility/DataExtractorTest.cpp


Index: lldb/unittests/Utility/DataExtractorTest.cpp
===================================================================
--- lldb/unittests/Utility/DataExtractorTest.cpp
+++ lldb/unittests/Utility/DataExtractorTest.cpp
@@ -25,6 +25,9 @@
   offset = 0;
   ASSERT_EQ(buffer[1], BE.GetMaxU64Bitfield(&offset, sizeof(buffer), 8, 8));
   offset = 0;
+  ASSERT_EQ(static_cast<uint64_t>(0xEFCDAB8967452301),
+            LE.GetMaxU64Bitfield(&offset, sizeof(buffer), 64, 0));
+  offset = 0;
   ASSERT_EQ(static_cast<uint64_t>(0x0123456789ABCDEF),
             BE.GetMaxU64Bitfield(&offset, sizeof(buffer), 64, 0));
   offset = 0;
@@ -40,6 +43,12 @@
   offset = 0;
   ASSERT_EQ(int8_t(buffer[1]),
             BE.GetMaxS64Bitfield(&offset, sizeof(buffer), 8, 8));
+  offset = 0;
+  ASSERT_EQ(static_cast<int64_t>(0xEFCDAB8967452301),
+            LE.GetMaxS64Bitfield(&offset, sizeof(buffer), 64, 0));
+  offset = 0;
+  ASSERT_EQ(static_cast<int64_t>(0x0123456789ABCDEF),
+            BE.GetMaxS64Bitfield(&offset, sizeof(buffer), 64, 0));
 }
 
 TEST(DataExtractorTest, PeekData) {
Index: lldb/source/Utility/DataExtractor.cpp
===================================================================
--- lldb/source/Utility/DataExtractor.cpp
+++ lldb/source/Utility/DataExtractor.cpp
@@ -604,6 +604,8 @@
 int64_t DataExtractor::GetMaxS64Bitfield(offset_t *offset_ptr, size_t size,
                                          uint32_t bitfield_bit_size,
                                          uint32_t bitfield_bit_offset) const {
+  assert(size >= 1 && "GetMaxS64Bitfield size must be >= 1");
+  assert(size <= 8 && "GetMaxS64Bitfield size must be <= 8");
   int64_t sval64 = GetMaxS64(offset_ptr, size);
   if (bitfield_bit_size > 0) {
     int32_t lsbcount = bitfield_bit_offset;
@@ -612,7 +614,7 @@
     if (lsbcount > 0)
       sval64 >>= lsbcount;
     uint64_t bitfield_mask =
-        ((static_cast<uint64_t>(1)) << bitfield_bit_size) - 1;
+        llvm::maskTrailingOnes<uint64_t>(bitfield_bit_size);
     sval64 &= bitfield_mask;
     // sign extend if needed
     if (sval64 & ((static_cast<uint64_t>(1)) << (bitfield_bit_size - 1)))
Index: lldb/include/lldb/Utility/DataExtractor.h
===================================================================
--- lldb/include/lldb/Utility/DataExtractor.h
+++ lldb/include/lldb/Utility/DataExtractor.h
@@ -535,13 +535,13 @@
                              uint32_t bitfield_bit_size,
                              uint32_t bitfield_bit_offset) const;
 
-  /// Extract an signed integer of size \a byte_size from \a *offset_ptr, then
-  /// extract and signe extend the bitfield from this value if \a
+  /// Extract an signed integer of size \a size from \a *offset_ptr, then
+  /// extract and sign-extend the bitfield from this value if \a
   /// bitfield_bit_size is non-zero.
   ///
-  /// Extract a single signed integer value (sign extending if required) and
+  /// Extract a single signed integer value (sign-extending if required) and
   /// update the offset pointed to by \a offset_ptr. The size of the extracted
-  /// integer is specified by the \a byte_size argument. \a byte_size must
+  /// integer is specified by the \a size argument. \a size must
   /// have a value greater than or equal to one and less than or equal to
   /// eight since the return value is 64 bits wide.
   ///


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D73913.242140.patch
Type: text/x-patch
Size: 3335 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20200203/25a57efd/attachment.bin>