Issue 1146203003: Fix abbreviation handling in munged bitcode writer.

Issue 1146203003: Fix abbreviation handling in munged bitcode writer. (Closed)

Created:
5 years, 7 months ago by Karl

Modified:
5 years, 7 months ago

Reviewers:
jvoung (off chromium), Derek Schuff

CC:
native-client-reviews_googlegroups.com

Base URL:
https://chromium.googlesource.com/native_client/pnacl-llvm.git@master

Target Ref:
refs/heads/master

Visibility:
Public.

More Reviews

Description

Fix abbreviation handling in munged bitcode writer. The bitstream writer assumes several properties, and will fail (either through assert or core dump) if these properties aren't true. Fixing munged bitcode writer to handle the following assumptions: 1) Don't write out an abbreviation if the abbreviation can't be applied to the record. 2) Don't write out an abbreviation definition if the record doesn't define a valid abbreviation (previous code was checking this, but not correctly for the abbreviation operand). 3) Don't use abbreviations whose abbreviation index is larger than couldn't be written out (case 2). Such indices will be broken because the bitstream reader doesn't correctly match. BUG=https://code.google.com/p/nativeclient/issues/detail?id=4169 R=jvoung@chromium.org Committed: https://chromium.googlesource.com/native_client/pnacl-llvm/+/e439162d4e1a4b422cab9dfc07819f891f633306

Patch Set 1 #

Patch Set 2 : Fix nits. #

Patch Set 3 : Don't allow application of abbreviation if index out of range. #

Total comments: 39

Patch Set 4 : Fix isses raised in patch set 3. #

Total comments: 4

Patch Set 5 : Fix nits. #

Created: 5 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+350 lines, -60 lines)			Patch
M	include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h	View	1 2 3 4	6 chunks	+36 lines, -23 lines	0 comments	Download
M	lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp	View	1 2 3	14 chunks	+128 lines, -23 lines	0 comments	Download
M	unittests/Bitcode/NaClMungeWriteErrorTests.cpp	View	1 2 3 4	4 chunks	+186 lines, -14 lines	0 comments	Download

Messages

Total messages: 9 (1 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Karl

https://codereview.chromium.org/1146203003/diff/40001/lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp File lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp (right): https://codereview.chromium.org/1146203003/diff/40001/lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp#newcode239 lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:239: NaClBitstreamWriter &Writer, const NaClBitcodeAbbrevRecord &Record) { Added following two ...

5 years, 7 months ago (2015-05-20 20:32:04 UTC) #3

jvoung (off chromium)

https://codereview.chromium.org/1146203003/diff/40001/include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h File include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h (right): https://codereview.chromium.org/1146203003/diff/40001/include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h#newcode425 include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:425: // the abbreviation index. Returns nullptr if no such ...

5 years, 7 months ago (2015-05-20 22:40:10 UTC) #4

Karl

5 years, 7 months ago (2015-05-21 18:22:08 UTC) #5

https://codereview.chromium.org/1146203003/diff/40001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h (right):

https://codereview.chromium.org/1146203003/diff/40001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:425: // the abbreviation index.
Returns nullptr if no such abbreviation
On 2015/05/20 22:40:09, jvoung wrote:
> slash consistency /// vs //
> 
> period

Done.

https://codereview.chromium.org/1146203003/diff/40001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:435: size_t
getCurNumAbbreviations() const {
On 2015/05/20 22:40:09, jvoung wrote:
> unused method ?

Removed.

https://codereview.chromium.org/1146203003/diff/40001/lib/Bitcode/NaCl/TestUt...
File lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp (right):

https://codereview.chromium.org/1146203003/diff/40001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:30: //
NaClBitstreamWriter::Emit().
On 2015/05/20 22:40:09, jvoung wrote:
> Seems like this should be part of NaClBitstreamWriter and used there too.

I agree. I originally put it there, but I didn't like how it read. Will put it
back in.

https://codereview.chromium.org/1146203003/diff/40001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:47: << AbbrevIndexLimit <<
"OmittedAbbreviations = "
On 2015/05/20 22:40:10, jvoung wrote:
> = spacing consistency
> 
> previous ones just did X=Y

Done.

https://codereview.chromium.org/1146203003/diff/40001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:64:
SmallVector<BlockScope, 3> ScopeStack;
On 2015/05/20 22:40:10, jvoung wrote:
> btw, I don't remember if this 3 was "optimized" for the most common case, but
in
> some recent refactoring you added an sentinel extra layer, so the 3 may no
> longer be "optimized".

Good point. It was optimized as you suggested. Increasing to 4.

https://codereview.chromium.org/1146203003/diff/40001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:73: std::set<unsigned>
BlocksWithOmittedAbbrevs;
On 2015/05/20 22:40:10, jvoung wrote:
> Would DenseSet be better?
> 
> I suppose there are only so many block types, so this doesn't grow much.

I did not expect the set to get much larger than 8, and hence, I think a set is
a better choice in this case.

https://codereview.chromium.org/1146203003/diff/40001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:123: return;
On 2015/05/20 22:40:10, jvoung wrote:
> The return doesn't seem to do much so may be better to remove.
> 
> It's indented even though it's not part of the then-statement (no curly), so
> that's a bit confusing =)

Hmm. I guess this is a leftover of an earlier version. Removing the return.

https://codereview.chromium.org/1146203003/diff/40001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:165: // Returns true if
the abbreviation index defines an anbbreviation
On 2015/05/20 22:40:09, jvoung wrote:
> anbbreviation -> ?

Fixed.

https://codereview.chromium.org/1146203003/diff/40001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:239: NaClBitstreamWriter
&Writer, const NaClBitcodeAbbrevRecord &Record) {
On 2015/05/20 22:40:09, jvoung wrote:
> On 2015/05/20 20:32:04, Karl wrote:
> > Added following two lines to make sure that the block defined enough bits to
> > apply abbreviation.
> 
> Does the Writer.getAbbreviation(...); if (Abbrev == nullptr) cover this, or
does
> this first check help?

This check helps. I didn't see the case until the fuzzer generated it. Because
global abbreviations are generated without knowledge of the number of bits
allowed for an individual block, and the abbreviations are copied in blindly,
this can lead to an error.

The bitstream writer also doesn't check that there is a mismatch between the
number of defined abbreviations, and the number of bits defined for the block.
Hence I added this test.

I decided to move this logic back into the bitstream writer, since it should
really check for this complex condition and fail. Otherwise, subtle bugs could
be added to pnacl bitcode files that won't manifest themselves until the file is
read.

https://codereview.chromium.org/1146203003/diff/40001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:270: if (Value >=
(static_cast<uint64_t>(1) < MaxEmitFixedBits)
On 2015/05/20 22:40:10, jvoung wrote:
> I assume this should actually be a shift and not (1 < MaxEmitFixedBits)?

Yes. Fixing.

https://codereview.chromium.org/1146203003/diff/40001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:279: // This should not
happen
On 2015/05/20 22:40:10, jvoung wrote:
> llvm_unreachable(...);

Done.

https://codereview.chromium.org/1146203003/diff/40001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:381: if
(!Flags.getTryToRecover())
On 2015/05/20 22:40:10, jvoung wrote:
> Might be simpler to just return now like the other cases, since it will just
> fall through and hit the "if (curBlockHasOmmitted...) { ... return ...; }".
> 
> Then you can have the "if (curBlock...)" check as the first thing, which seems
> more straightforward.

I agree that the check for omitted abbreviations should be the first case, and
moved it up.

For the case of the wrong abbreviation index, I did the wrong thing. The simple
recovery is to just ignore the abbreviation index and generate it.

Further, we can remove the limit on bitsize check, since method
Writer.getAbbreviation() now checks this when it is used.

https://codereview.chromium.org/1146203003/diff/40001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:432: RecoverableError() <<
"Abbreviation doesn't apply to record: "
On 2015/05/20 22:40:09, jvoung wrote:
> So here the abbreviation exists, but doesn't apply.

Yes.

https://codereview.chromium.org/1146203003/diff/40001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:435: WriteRecord(Writer,
Record, UsesDefaultAbbrev);
On 2015/05/20 22:40:09, jvoung wrote:
> This and the one below might be the first case where WriteRecord() happens
> before checking Flags.getTryToRecover(). So, is that the intention to allow
> writing some additional recovery-based-amount when the flag isn't set?

Done.

https://codereview.chromium.org/1146203003/diff/40001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:438: if
(Flags.getWriteBadAbbrevIndex()) {
On 2015/05/20 22:40:10, jvoung wrote:
> Maybe add a comment here. This CL adds the "figure out why" comment, but this
> case doesn't apply to the "figure out why". I.e., it's not clear how much of
the
> following code is for the "figure out why".
> 
> Alternatively, make the "figure out why" more precise and have it only apply
to
> the first if-block.

Removed the first comment, and added a comment to this case to explain.

https://codereview.chromium.org/1146203003/diff/40001/unittests/Bitcode/NaClM...
File unittests/Bitcode/NaClMungeWriteErrorTests.cpp (right):

https://codereview.chromium.org/1146203003/diff/40001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:264: // the problem by applying
the default abbreviation instead.o
On 2015/05/20 22:40:10, jvoung wrote:
> instead.o --> instead.

Done.

https://codereview.chromium.org/1146203003/diff/40001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:275: RetVoidIndex,
NaClMungedBitcode::AddBefore,
On 2015/05/20 22:40:10, jvoung wrote:
> I guess it is kind of cool/weird that you can define an abbrev in the middle
of
> a block.

It is.

https://codereview.chromium.org/1146203003/diff/40001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:300:
EXPECT_TRUE(Munger.runTest());
On 2015/05/20 22:40:10, jvoung wrote:
> Add a run without recovery too?

Done.

https://codereview.chromium.org/1146203003/diff/40001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:331: 2,
naclbitc::BLK_CODE_DEFINE_ABBREV, 1, // 1,
On 2015/05/20 22:40:10, jvoung wrote:
> How about expand on why the "// 1,"? E.g.,
> 
> // 1,
> 
> // 1, intentionally leave out "literal" operand

Done.

jvoung (off chromium)

LGTM -- please remember to update deps when ready =) https://codereview.chromium.org/1146203003/diff/60001/include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h File include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h (right): https://codereview.chromium.org/1146203003/diff/60001/include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h#newcode164 ...

5 years, 7 months ago (2015-05-21 20:58:00 UTC) #6

Karl

Committed patchset #5 (id:80001) manually as e439162d4e1a4b422cab9dfc07819f891f633306 (presubmit successful).

5 years, 7 months ago (2015-05-22 15:03:45 UTC) #7

Karl

https://codereview.chromium.org/1146203003/diff/60001/include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h File include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h (right): https://codereview.chromium.org/1146203003/diff/60001/include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h#newcode164 include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:164: // Nax Number of bits that can be written ...

5 years, 7 months ago (2015-05-22 15:30:43 UTC) #8

Karl

5 years, 7 months ago (2015-05-22 15:30:43 UTC) #9

Message was sent while issue was closed.

https://codereview.chromium.org/1146203003/diff/60001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h (right):

https://codereview.chromium.org/1146203003/diff/60001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h:164: // Nax Number of bits that
can be written using Emit.
On 2015/05/21 20:57:59, jvoung wrote:
> Nax Number -> Max number

Done.

https://codereview.chromium.org/1146203003/diff/60001/unittests/Bitcode/NaClM...
File unittests/Bitcode/NaClMungeWriteErrorTests.cpp (right):

https://codereview.chromium.org/1146203003/diff/60001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:464: // Block only spefies 2 bits
for abbreviations (i.e. limit = 3).
On 2015/05/21 20:57:59, jvoung wrote:
> spefies -> specifies

Done.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages