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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri accepted this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

I would still like to see a test with `CodeLen`=32, and even a single call to `EmitRecordWithAbbrevImpl()`
(ubsan would then have caught the ub in the initial approach), but otherwise this seems reasonable to me.
I have no expertise in this code, so *please* wait for someone else to review this, too.



================
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();
----------------
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)


================
Comment at: unittests/Bitcode/BitstreamWriterTest.cpp:75-182
+TEST(BitstreamWriterTest, trapOnSmallAbbrev_CodeLength2) {
+  SmallString<64> Buffer;
+  BitstreamWriter W(Buffer);
+  W.EnterSubblock(bitc::FIRST_APPLICATION_BLOCKID, /*CodeLen*/2);
+  auto Abbrev = std::make_shared<BitCodeAbbrev>();
+  Abbrev->Add(BitCodeAbbrevOp(42U));
+  unsigned AbbrevCode = W.EmitAbbrev(std::move(Abbrev));
----------------
jordan_rose wrote:
> lebedev.ri wrote:
> > This looks rather monstrous, with a lot of duplication.
> > I was suggesting to just do:
> > ```
> > TEST(BitstreamWriterTest, trapOnSmallAbbrevUsingBlockInfo_CodeLength3_Ok) {
> >   SmallString<64> Buffer;
> >   BitstreamWriter W(Buffer);
> >   W.EnterBlockInfoBlock();
> >   auto Abbrev = std::make_shared<BitCodeAbbrev>();
> >   Abbrev->Add(BitCodeAbbrevOp(2U);
> >   (void)W.EmitBlockInfoAbbrev(bitc::FIRST_APPLICATION_BLOCKID,
> >                               std::move(Abbrev));
> >   W.ExitBlock();
> >   W.EnterSubblock(bitc::FIRST_APPLICATION_BLOCKID, /*CodeLen*/3);
> >   W.EmitRecordWithAbbrev(Abbrev, makeArrayRef(2U));
> >   W.ExitBlock();
> > }
> > 
> > TEST(BitstreamWriterTest, trapOnSmallAbbrevUsingBlockInfo_CodeLength3_LastOk) {
> >   SmallString<64> Buffer;
> >   BitstreamWriter W(Buffer);
> >   W.EnterBlockInfoBlock();
> >   auto Abbrev = std::make_shared<BitCodeAbbrev>();
> >   Abbrev->Add(BitCodeAbbrevOp(3U);
> >   (void)W.EmitBlockInfoAbbrev(bitc::FIRST_APPLICATION_BLOCKID,
> >                               std::move(Abbrev));
> >   W.ExitBlock();
> >   W.EnterSubblock(bitc::FIRST_APPLICATION_BLOCKID, /*CodeLen*/3);
> >   W.EmitRecordWithAbbrev(Abbrev, makeArrayRef(3U));
> >   W.ExitBlock();
> > }
> > 
> > TEST(BitstreamWriterTest, trapOnSmallAbbrevUsingBlockInfo_CodeLength3_NotOk) {
> >   SmallString<64> Buffer;
> >   BitstreamWriter W(Buffer);
> >   W.EnterBlockInfoBlock();
> >   auto Abbrev = std::make_shared<BitCodeAbbrev>();
> >   Abbrev->Add(BitCodeAbbrevOp(4U);
> >   (void)W.EmitBlockInfoAbbrev(bitc::FIRST_APPLICATION_BLOCKID,
> >                               std::move(Abbrev));
> >   W.ExitBlock();
> >   W.EnterSubblock(bitc::FIRST_APPLICATION_BLOCKID, /*CodeLen*/3);
> >   W.EmitRecordWithAbbrev(Abbrev, makeArrayRef(4U));
> >   W.ExitBlock();
> > 
> > TEST(BitstreamWriterTest, trapOnSmallAbbrevUsingBlockInfo_CodeLength3_NoOverFlow) {
> >   SmallString<64> Buffer;
> >   BitstreamWriter W(Buffer);
> >   W.EnterBlockInfoBlock();
> >   auto Abbrev = std::make_shared<BitCodeAbbrev>();
> >   Abbrev->Add(BitCodeAbbrevOp(4294967295U); // i32 -1 
> >   (void)W.EmitBlockInfoAbbrev(bitc::FIRST_APPLICATION_BLOCKID,
> >                               std::move(Abbrev));
> >   W.ExitBlock();
> >   W.EnterSubblock(bitc::FIRST_APPLICATION_BLOCKID, /*CodeLen*/32);
> >   W.EmitRecordWithAbbrev(Abbrev, makeArrayRef(4294967295U));
> >   W.ExitBlock();
> > }
> > ```
> The values in the abbreviation have nothing to do with the abbreviation codes themselves; in order to get to code 8 you have to actually register 5 abbreviations. (0-3 are reserved.) I could probably pull out some of this into helper functions but I didn't feel like it was going to be super important.
Oh duh, i knew that, but kinda forgot :)


Repository:
  rL LLVM

https://reviews.llvm.org/D38833