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

[Jan Korous via Phabricator via llvm-commits]

jkorous created this revision.
jkorous added reviewers: browneee, nikic, vitalybuka.
jkorous requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

This patch aims to address: https://reviews.llvm.org/D119182

This commit from today improves the situation but doesn't fix the UB.
https://github.com/llvm/llvm-project/commit/67348c8acfc205785996f2aea21b442f4b76f2c2

I am trying to prevent the UB in a mechanical fashion. Possibly there's a better solution and the error messages can very likely be worded better. I am not familiar with the code - open to suggestions!

When running the test against ubsanitized build (`-fsanitize=integer`) it still triggers couple issues but not the one that lead to the test getting disabled in https://reviews.llvm.org/D119182

  > bin/llvm-lit -v ../repo.git/llvm/test/Bitcode/invalid-no-ubsan.test
  /tmp/llvm-project/include/llvm/Support/DJB.h:23:12: runtime error: left shift of 4042302735 by 5 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
  /tmp/llvm-project/include/llvm/Support/DJB.h:23:18: runtime error: unsigned integer overflow: 504668640 + 4042302735 cannot be represented in type 'unsigned int'
  /tmp/llvm-project/include/llvm/Option/ArgList.h:135:42: runtime error: negation of 1 cannot be represented in type 'unsigned int'
  /tmp/llvm-project/include/llvm/ADT/DenseMap.h:807:86: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'unsigned int'
  /tmp/llvm-project/lib/Option/ArgList.cpp:68:18: runtime error: negation of 1 cannot be represented in type 'unsigned int'
  /tmp/llvm-project/lib/Support/StringRef.cpp:299:45: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'unsigned long'
  /tmp/llvm-project/include/llvm/ADT/StringRef.h:864:74: runtime error: unsigned integer overflow: 18446744073709551615 + 1 cannot be represented in type 'unsigned long'


https://reviews.llvm.org/D119307

Files:
  llvm/include/llvm/Bitstream/BitstreamReader.h
  llvm/test/Bitcode/invalid-no-ubsan.test


Index: llvm/test/Bitcode/invalid-no-ubsan.test
===================================================================
--- llvm/test/Bitcode/invalid-no-ubsan.test
+++ llvm/test/Bitcode/invalid-no-ubsan.test
@@ -6,14 +6,12 @@
 # TODO: This code should be fixed to not exhibit UB, and these tests should be
 # incorporated back into invalid.test and run under UBSan again.
 
-UNSUPPORTED: ubsan
-
 RUN: not llvm-dis -disable-output %p/Inputs/size-not-plausible.bc 2>&1 | \
 RUN:   FileCheck --check-prefix=SIZE-NOT-PLAUSIBLE %s
 
-SIZE-NOT-PLAUSIBLE: Size is not plausible
+SIZE-NOT-PLAUSIBLE: Invalid NumBits or NextBit value
 
 RUN: not llvm-dis -disable-output %p/Inputs/invalid-value-symbol-table-2.bc 2>&1 | \
 RUN:   FileCheck --check-prefix=INVALID-VALUE-SYMBOL-TABLE-2 %s
 
-INVALID-VALUE-SYMBOL-TABLE-2: Expected value symbol table subbloc
+INVALID-VALUE-SYMBOL-TABLE-2: Invalid NumBits or NextBit value
Index: llvm/include/llvm/Bitstream/BitstreamReader.h
===================================================================
--- llvm/include/llvm/Bitstream/BitstreamReader.h
+++ llvm/include/llvm/Bitstream/BitstreamReader.h
@@ -229,21 +229,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.407021.patch
Type: text/x-patch
Size: 2355 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220209/60947768/attachment.bin>