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

[Andrew via Phabricator via llvm-commits]

browneee added a comment.

Thanks for following up on D119182 <https://reviews.llvm.org/D119182>!



================
Comment at: llvm/include/llvm/Bitstream/BitstreamReader.h:232
 
-  Expected<uint32_t> ReadVBR(unsigned NumBits) {
+  Expected<uint32_t> ReadVBR(const unsigned NumBits) {
     Expected<unsigned> MaybeRead = Read(NumBits);
----------------
nit: Consider improving the error message for this test by adding some context (i.e, it is the size which has invalid numbits) to the error, at the caller - [here](https://github.com/llvm/llvm-project/blob/5e71bbfb6cdcd7c1d04361a0a34812991479dc69/llvm/lib/Bitstream/Reader/BitstreamReader.cpp#L223).


================
Comment at: llvm/include/llvm/Bitstream/BitstreamReader.h:239
+    if (NumBits >= 33 || NumBits == 0)
+      return make_error<StringError>("Invalid NumBits value",
+                                     llvm::inconvertibleErrorCode());
----------------
nit: Provide the bad value of NumBits in the error message?


================
Comment at: llvm/include/llvm/Bitstream/BitstreamReader.h:250-255
+      if (Mask > 1) {
+        if (MaskBitOrder - 1 + NextBit >= 32) {
+          return make_error<StringError>("Invalid NumBits or NextBit value",
+                                         llvm::inconvertibleErrorCode());
+        }
+      }
----------------
Why not simpler condition?

```
if (NextBit >= 32) { ... }
```

And move later in the loop for a little less overhead (the first iteration will always be NextBit==0).


================
Comment at: llvm/test/Bitcode/invalid-no-ubsan.test:7
 # 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.
 
----------------
With these tests fixed, there is no reason for a separate file (separate file is so `UNSUPPORTED: ubsan` only applies to these, and not the rest).

These tests should be moved back to `invalid.test`.


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

https://reviews.llvm.org/D119307