[PATCH] D119307: [Bitstream] Fix UB in left-shift in ReadVBR

[Jan Korous via Phabricator via llvm-commits]

jkorous updated this revision to Diff 408112.
jkorous added a comment.
Herald added a subscriber: hiraditya.

rebased + improved error messages


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119307/new/

https://reviews.llvm.org/D119307

Files:
  llvm/include/llvm/Bitstream/BitstreamReader.h
  llvm/lib/Bitstream/Reader/BitstreamReader.cpp
  llvm/test/Bitcode/invalid.test


Index: llvm/test/Bitcode/invalid.test
===================================================================
--- llvm/test/Bitcode/invalid.test
+++ llvm/test/Bitcode/invalid.test
@@ -275,7 +275,7 @@
 RUN: not llvm-dis -disable-output %p/Inputs/unterminated-vbr.bc 2>&1 | \
 RUN:   FileCheck --check-prefix=UNTERMINATED-VBR %s
 
-UNTERMINATED-VBR: Unterminated VBR
+UNTERMINATED-VBR: Failed to read size: Invalid NumBits or NextBit value
 
 RUN: not llvm-dis -disable-output %p/Inputs/comdat-name-too-large.bc 2>&1 | \
 RUN:   FileCheck --check-prefix=COMDAT-NAME-TOO-LARGE %s
Index: llvm/lib/Bitstream/Reader/BitstreamReader.cpp
===================================================================
--- llvm/lib/Bitstream/Reader/BitstreamReader.cpp
+++ llvm/lib/Bitstream/Reader/BitstreamReader.cpp
@@ -220,7 +220,9 @@
     uint32_t Code = MaybeCode.get();
     Expected<uint32_t> MaybeNumElts = ReadVBR(6);
     if (!MaybeNumElts)
-      return MaybeNumElts.takeError();
+      return error(("Failed to read size: " +
+                    toString(std::move(MaybeNumElts.takeError())))
+                       .c_str());
     uint32_t NumElts = MaybeNumElts.get();
     if (!isSizePlausible(NumElts))
       return error("Size is not plausible");
@@ -275,7 +277,9 @@
       // Array case.  Read the number of elements as a vbr6.
       Expected<uint32_t> MaybeNumElts = ReadVBR(6);
       if (!MaybeNumElts)
-        return MaybeNumElts.takeError();
+        return error(("Failed to read size: " +
+                      toString(std::move(MaybeNumElts.takeError())))
+                         .c_str());
       uint32_t NumElts = MaybeNumElts.get();
       if (!isSizePlausible(NumElts))
         return error("Size is not plausible");
Index: llvm/include/llvm/Bitstream/BitstreamReader.h
===================================================================
--- llvm/include/llvm/Bitstream/BitstreamReader.h
+++ llvm/include/llvm/Bitstream/BitstreamReader.h
@@ -227,21 +227,33 @@
     return R;
   }
 
-  Expected<uint32_t> ReadVBR(unsigned NumBits) {
+  Expected<uint32_t> ReadVBR(const unsigned NumBits) {
     Expected<unsigned> MaybeRead = Read(NumBits);
     if (!MaybeRead)
       return MaybeRead;
     uint32_t Piece = MaybeRead.get();
 
-    if ((Piece & (1U << (NumBits-1))) == 0)
+    if (NumBits >= 33 || NumBits == 0)
+      return make_error<StringError>("Invalid NumBits value",
+                                     llvm::inconvertibleErrorCode());
+    const uint32_t MaskBitOrder = (NumBits - 1);
+    const uint32_t Mask = 1UL << MaskBitOrder;
+
+    if ((Piece & Mask) == 0)
       return Piece;
 
     uint32_t Result = 0;
     unsigned NextBit = 0;
     while (true) {
-      Result |= (Piece & ((1U << (NumBits-1))-1)) << NextBit;
+      if (Mask > 1) {
+        if (MaskBitOrder - 1 + NextBit >= 32) {
+          return make_error<StringError>("Invalid NumBits or NextBit value",
+                                         llvm::inconvertibleErrorCode());
+        }
+      }
+      Result |= (Piece & (Mask - 1)) << NextBit;
 
-      if ((Piece & (1U << (NumBits-1))) == 0)
+      if ((Piece & Mask) == 0)
         return Result;
 
       NextBit += NumBits-1;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D119307.408112.patch
Type: text/x-patch
Size: 3182 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220212/97ef848d/attachment.bin>