[PATCH] D38833: BitstreamWriter: Better assertion when a block's abbrev code size is too small

[Jordan Rose via Phabricator via llvm-commits]

jordan_rose added inline comments.


================
Comment at: include/llvm/Bitcode/BitstreamWriter.h:303
     assert(AbbrevNo < CurAbbrevs.size() && "Invalid abbrev #!");
+    assert(Abbrev < (1U << CurCodeSize) && "Block code length is too small");
     const BitCodeAbbrev *Abbv = CurAbbrevs[AbbrevNo].get();
----------------
lebedev.ri wrote:
> What is the expected range of `CurCodeSize` values?
> This has UB if `CurCodeSize` == `32`.
> It would be much safer to just use the variant with subtraction:
> ```
> assert(Abbrev <= (~(~0U >> (32-CurCodeSize))) && "Block code length is too small");
> ```
Good point. I don't think anyone's trying to use 32-bit record codes, but the [spec](http://llvm.org/docs/BitCodeFormat.html) doesn't seem to impose a limit. I'll switch it.


================
Comment at: unittests/Bitcode/BitstreamWriterTest.cpp:65
+  auto Abbrev = std::make_shared<BitCodeAbbrev>();
+  Abbrev->Add(BitCodeAbbrevOp(42U));
+  unsigned AbbrevCode = W.EmitAbbrev(std::move(Abbrev));
----------------
lebedev.ri wrote:
> I'd expect to see three tests.
> One last valid one, the invalid one, and the next after the first invalid one.
> So `3U`, `4U`, `5U`.
This is just an arbitrary data value. What makes this too big is the CodeLen of 2 above. But I can certainly test overflowing a CodeLen of 3 as well.


Repository:
  rL LLVM

https://reviews.llvm.org/D38833