[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:
> jordan_rose wrote:
> > lebedev.ri wrote:
> > > jordan_rose wrote:
> > > > 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.
> > > I think you want to move this assert to before the first use of `Abbrev` (before line 301)
> > It shouldn't matter in this case. The first use (and the above assertion) is for indexing into a write-time data structure; the second is for the actual representation in the bitstream.
> Point being, if you don't move it, you won't be able to add a test for 32-bit `CurCodeSize` overflow.
I have to admit I'm not really interested in that. It's worth writing code that won't silently invoke undefined behavior, but in practice no one is actually going to have billions of abbreviations, and it's not interesting to me which assertion fires in that case.


Repository:
  rL LLVM

https://reviews.llvm.org/D38833