Harden writer of munged bitcode for fuzzing

Harden writer of munged bitcode for fuzzing

Hardens the writer of munged bitcode. Does this by adding error
recovery that guarantees the bitstream writer will not fail on
assertions.

Also cleans up error recovery in general and adds tests to show what
errors are reported by the writer, as well as what error recovery is
applied.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4169
R=jvoung@chromium.org

Committed: https://chromium.googlesource.com/native_client/pnacl-llvm/+/e5c16c87eb22c2529bec44cf2e77529450956011

[Karl, 2015-05-11 22:59:58]

kschimpf@google.com changed reviewers:
+ dsansome@chromium.org, jvoung@chromium.org

[dsansome, 2015-05-12 01:23:44]

dsansome@chromium.org changed reviewers:
- dsansome@chromium.org

[dsansome, 2015-05-12 01:23:45]

I think you added the wrong reviewer, I've never seen this code before :)

[Karl, 2015-05-12 05:02:38]

kschimpf@google.com changed reviewers:
+ dschuff@chromium.org

[jvoung (off chromium), 2015-05-13 00:58:17]

re: "BUG= https://code.google.com/p/nativeclient/issues/detail?id=4164"

Seems like this is more for bitcode fuzzing than for "Extend PNaCl bitstream
reader/writer to be able to print out records byte aligned.", or am I
misunderstanding?

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

https://codereview.chromium.org/1139673004/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:32: // Defines the maximun
value for abbreviation indices in block.
maximun -> maximum

https://codereview.chromium.org/1139673004/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:106:
ScopeStack.pop_back();
Hmm, I don't see how this prevents popping too much? ScopeStack is a plain
SmallVector?

https://codereview.chromium.org/1139673004/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:117: for (auto Scope :
ScopeStack)
auto &Scope ? Not sure if this is one of those cases where it might have made a
copy instead.

https://codereview.chromium.org/1139673004/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:137: uint64_t MaxAbbrev =
(static_cast<uint64_t>(1) << NumBits) - 1;
maybe assert NumBits < 64 or some smaller limit -- unless it's checked somewhere
else

https://codereview.chromium.org/1139673004/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:161:
NaClMungedBitcode::WriteResults & WriteState::finish(
flush the & in the return type to the right

https://codereview.chromium.org/1139673004/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:219: enterBlock(Writer,
WriteBlockID, NumBits, Record);
At this point, it's still possible for NumValues == 2, so technically you could
have set "NumBits = Record.Values[1];"

https://codereview.chromium.org/1139673004/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:219: enterBlock(Writer,
WriteBlockID, NumBits, Record);
return enterBlock(...);

To avoid falling through and doing another enterBlock ()

https://codereview.chromium.org/1139673004/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:224: if (NumBits > 32 ||
MinNumBits < 2) {
"MinNumBits < 2" MinNumBits is pretty much a constant, so I'm not sure that
makes sense to check...

Should that be "NumBits < MinNumBits" ?

https://codereview.chromium.org/1139673004/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:270: &&
Writer.getMaxCurAbbrevIndex() >=  getCurAbbrevIndexLimit()) {
">=  "
">= "

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

https://codereview.chromium.org/1139673004/diff/20001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:115:
TEST(NaClMungeWriteErrorTests, CantDumpTooManyLocalAbbreviations) {
Maybe "define" instead of "dump"? I think of the object dumps / ExpectedDump,
etc.., instead with "dump".

https://codereview.chromium.org/1139673004/diff/20001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:163:
TEST(NaClMungeWriteErrorTests, TooFewExitBlocks) {
re: TooFewExitBlocks, in this case there are too few enter blocks? Or too many
exit blocks.

https://codereview.chromium.org/1139673004/diff/20001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:172:
EXPECT_FALSE(Munger.runTest("TooFewExitBlocks"));
"TooFewExitBlocks" -> ?

https://codereview.chromium.org/1139673004/diff/20001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:199: // abbreviation index.
So to be clear -- the error recovery is in the writer, which writes out
parseable bitcode as the recover step. The reader doesn't recover and doesn't
run into an error.

https://codereview.chromium.org/1139673004/diff/20001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:271: // than exit blocks.
Similar comment to TooFewExitBlocks

[Karl, 2015-05-13 21:39:25]

I fixed the description and corresponding bug to reflect that this is for
fuzzing.

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

https://codereview.chromium.org/1139673004/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:32: // Defines the maximun
value for abbreviation indices in block.
On 2015/05/13 00:58:16, jvoung wrote:
> maximun -> maximum

Done.

https://codereview.chromium.org/1139673004/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:106:
ScopeStack.pop_back();
On 2015/05/13 00:58:17, jvoung wrote:
> Hmm, I don't see how this prevents popping too much? ScopeStack is a plain
> SmallVector?

Hmm, I'm confused too. Earlier versions had the check.

Looking more closely, I see that the callers guarantee that this won't happen.
However, a better solution is to put it here, and generate an error if there is
a mismatch.

https://codereview.chromium.org/1139673004/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:117: for (auto Scope :
ScopeStack)
On 2015/05/13 00:58:17, jvoung wrote:
> auto &Scope ? Not sure if this is one of those cases where it might have made
a
> copy instead.

Done.

https://codereview.chromium.org/1139673004/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:137: uint64_t MaxAbbrev =
(static_cast<uint64_t>(1) << NumBits) - 1;
On 2015/05/13 00:58:17, jvoung wrote:
> maybe assert NumBits < 64 or some smaller limit -- unless it's checked
somewhere
> else

Good point. The code reader/writer assumes 32. Adding test.

https://codereview.chromium.org/1139673004/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:161:
NaClMungedBitcode::WriteResults & WriteState::finish(
On 2015/05/13 00:58:17, jvoung wrote:
> flush the & in the return type to the right

Done.

https://codereview.chromium.org/1139673004/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:219: enterBlock(Writer,
WriteBlockID, NumBits, Record);
On 2015/05/13 00:58:17, jvoung wrote:
> return enterBlock(...);
> 
> To avoid falling through and doing another enterBlock ()

Removing call to writeBlock(). Rather, let the code fall through to get NumBits

https://codereview.chromium.org/1139673004/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:224: if (NumBits > 32 ||
MinNumBits < 2) {
On 2015/05/13 00:58:17, jvoung wrote:
> "MinNumBits < 2" MinNumBits is pretty much a constant, so I'm not sure that
> makes sense to check...
> 
> Should that be "NumBits < MinNumBits" ?
> 

Good catch. Using MinNumBits.

https://codereview.chromium.org/1139673004/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:270: &&
Writer.getMaxCurAbbrevIndex() >=  getCurAbbrevIndexLimit()) {
On 2015/05/13 00:58:17, jvoung wrote:
> ">=  "
> ">= " 

Done.

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

https://codereview.chromium.org/1139673004/diff/20001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:115:
TEST(NaClMungeWriteErrorTests, CantDumpTooManyLocalAbbreviations) {
On 2015/05/13 00:58:17, jvoung wrote:
> Maybe "define" instead of "dump"? I think of the object dumps / ExpectedDump,
> etc.., instead with "dump".

Changing CantWriteTooManyLocallAbbreviations.

https://codereview.chromium.org/1139673004/diff/20001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:163:
TEST(NaClMungeWriteErrorTests, TooFewExitBlocks) {
On 2015/05/13 00:58:17, jvoung wrote:
> re: TooFewExitBlocks, in this case there are too few enter blocks? Or too many
> exit blocks.

Fixing to TooMayExitBlocks.

https://codereview.chromium.org/1139673004/diff/20001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:172:
EXPECT_FALSE(Munger.runTest("TooFewExitBlocks"));
On 2015/05/13 00:58:17, jvoung wrote:
> "TooFewExitBlocks" -> ?

Done.

https://codereview.chromium.org/1139673004/diff/20001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:199: // abbreviation index.
On 2015/05/13 00:58:17, jvoung wrote:
> So to be clear -- the error recovery is in the writer, which writes out
> parseable bitcode as the recover step. The reader doesn't recover and doesn't
> run into an error.

Yes.

Rewriting error recovery comments to be clearer about this.

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

https://codereview.chromium.org/1139673004/diff/60001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:196:
TEST(NaClMungerWriteErrorTests, CantWriteBlockWithMaxLimit) {
Added this test.

https://codereview.chromium.org/1139673004/diff/60001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:223:
TEST(NaClMungerWriteErrorTests, CantWriteBlockWithBadBitLimit) {
Added this test.

https://codereview.chromium.org/1139673004/diff/60001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:241:
TEST(NaClMungerWriteErrorTests, CantWriteBlockWithLargeBlockID) {
Added this test.

https://codereview.chromium.org/1139673004/diff/60001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:260:
TEST(MyNaClMungerWriteErrorTests, DieOnWriteBadAbbreviationIndex) {
Added this test.

[jvoung (off chromium), 2015-05-13 22:24:06]

Otherwise LGTM

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

https://codereview.chromium.org/1139673004/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:259: exitBlock(Writer);
btw: I was about to suggest adding LLVM_ATTRIBUTE_UNUSED_RESULT to the new
methods with bool results, to get a warning if the return value is mistakenly
ignored to help catch some of these + the previous case where you had
enterBlock() without the return and just a fallthrough.

Not sure number of false positives vs the benefit, though, so up to you.

https://codereview.chromium.org/1139673004/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:270: &&
Writer.getMaxCurAbbrevIndex() >=  getCurAbbrevIndexLimit()) {
On 2015/05/13 21:39:23, Karl wrote:
> On 2015/05/13 00:58:17, jvoung wrote:
> > ">=  "
> > ">= " 
> 
> Done.

Ping =) >=

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

https://codereview.chromium.org/1139673004/diff/60001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:57: // The miniumum number
of bits allowed to be specified in a block.
miniumum -> minimum

https://codereview.chromium.org/1139673004/diff/60001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:110: if
(ScopeStack.empty())
how about

if (atOutermostScope())
  return false;

Then it matches the comment, and also leaves the sentinel node there. Otherwise
if you pop that, the assert(!ScopeStack.empty()) that you have in a bunch of
places can fail.

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

https://codereview.chromium.org/1139673004/diff/60001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:149:
EXPECT_FALSE(Munger.runTest("TooManyEnterBlocks"));
CantWriteTooManyEnterBlocks -- I wasn't too sure these strings were too helpful?
gtest should tell you which test is running based on the X and Y in "TEST(X, Y)
{ ... }", and the EXPECT_* should tell you which line failed if there are
multiple EXPECT_*? Anyway, not asking for any action, just saying you might be
able to save some work syncing stuff.

https://codereview.chromium.org/1139673004/diff/60001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:196:
TEST(NaClMungerWriteErrorTests, CantWriteBlockWithMaxLimit) {
Seems like Cant should be Can, since this doesn't print any errors and it
returns true.

https://codereview.chromium.org/1139673004/diff/60001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:251: "Error (Block unknown):
Block id must be less than << 4294967295: 1:"
"less than << 4294967295" -- why the "<<" ?

https://codereview.chromium.org/1139673004/diff/60001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:257: // Show that parsing
successfully recovers (i.e. not crash) if the
"parsing successfully recovers" ? I thought it was writing that recovers, and
not in a way that parsing can succeed? There is EXPECT_DEATH, so it does crash.

https://codereview.chromium.org/1139673004/diff/60001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:260:
TEST(MyNaClMungerWriteErrorTests, DieOnWriteBadAbbreviationIndex) {
My... -> NaClMungerWriteErrorTests

[Karl, 2015-05-14 17:07:43]

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

[Karl, 2015-05-14 17:08:12]

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

https://codereview.chromium.org/1139673004/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:259: exitBlock(Writer);
On 2015/05/13 22:24:05, jvoung wrote:
> btw: I was about to suggest adding LLVM_ATTRIBUTE_UNUSED_RESULT to the new
> methods with bool results, to get a warning if the return value is mistakenly
> ignored to help catch some of these + the previous case where you had
> enterBlock() without the return and just a fallthrough.
> 
> Not sure number of false positives vs the benefit, though, so up to you.

Added and fixed code (3 cases). In all three, they shouldn't happen, but forced
a non-recoverable error should they fail for some reason. This makes the code
safer, in case code gets modified and the conditions are no longer assured.

https://codereview.chromium.org/1139673004/diff/20001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:270: &&
Writer.getMaxCurAbbrevIndex() >=  getCurAbbrevIndexLimit()) {
On 2015/05/13 22:24:06, jvoung wrote:
> On 2015/05/13 21:39:23, Karl wrote:
> > On 2015/05/13 00:58:17, jvoung wrote:
> > > ">=  "
> > > ">= " 
> > 
> > Done.
> 
> Ping =) >=

Done.

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

https://codereview.chromium.org/1139673004/diff/60001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:57: // The miniumum number
of bits allowed to be specified in a block.
On 2015/05/13 22:24:06, jvoung wrote:
> miniumum -> minimum

Done.

https://codereview.chromium.org/1139673004/diff/60001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:110: if
(ScopeStack.empty())
On 2015/05/13 22:24:06, jvoung wrote:
> how about
> 
> if (atOutermostScope())
>   return false;
> 
> Then it matches the comment, and also leaves the sentinel node there.
Otherwise
> if you pop that, the assert(!ScopeStack.empty()) that you have in a bunch of
> places can fail.
> 
> 

Done.

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

https://codereview.chromium.org/1139673004/diff/60001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:149:
EXPECT_FALSE(Munger.runTest("TooManyEnterBlocks"));
On 2015/05/13 22:24:06, jvoung wrote:
> CantWriteTooManyEnterBlocks -- I wasn't too sure these strings were too
helpful?
> gtest should tell you which test is running based on the X and Y in "TEST(X,
Y)
> { ... }", and the EXPECT_* should tell you which line failed if there are
> multiple EXPECT_*? Anyway, not asking for any action, just saying you might be
> able to save some work syncing stuff.

I agree that these names are no longer useful. Changing API to allow this
parameter to be skipped, and then remove them from runTest. Will fix other test
files in a separate CL.

https://codereview.chromium.org/1139673004/diff/60001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:196:
TEST(NaClMungerWriteErrorTests, CantWriteBlockWithMaxLimit) {
On 2015/05/13 22:24:06, jvoung wrote:
> Seems like Cant should be Can, since this doesn't print any errors and it
> returns true.

Good point. Fixing name.

https://codereview.chromium.org/1139673004/diff/60001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:251: "Error (Block unknown):
Block id must be less than << 4294967295: 1:"
On 2015/05/13 22:24:06, jvoung wrote:
> "less than << 4294967295" -- why the "<<" ?

Done.

https://codereview.chromium.org/1139673004/diff/60001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:257: // Show that parsing
successfully recovers (i.e. not crash) if the
On 2015/05/13 22:24:06, jvoung wrote:
> "parsing successfully recovers" ? I thought it was writing that recovers, and
> not in a way that parsing can succeed? There is EXPECT_DEATH, so it does
crash.

Good point. Rewrote to better fit test.