Issue 1157273004: Check for invalid abbreviation operators in munged bitcode.

Issue 1157273004: Check for invalid abbreviation operators in munged bitcode. (Closed)

Created:
5 years, 7 months ago by Karl

Modified:
5 years, 6 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

Check for invalid abbreviation operators in munged bitcode. Discovered (via fuzzing) that errors are generated by the PNaCl bitstream writer if the abbreviation is invalid. It also appplies operator-specific validity checks as each abbreviation operator is built. Updated the munged bitcode writer to check for these conditions and apply recoverable fixes if they occur. BUG=https://code.google.com/p/nativeclient/issues/detail?id=4169 R=jvoung@chromium.org Committed: https://chromium.googlesource.com/native_client/pnacl-llvm/+/2c9a36eceb3d8f0f8f2214f77879b65308e8c89e

Patch Set 1 #

Patch Set 2 : Fix nits and simplify tests. #

Patch Set 3 : Fix nit. Remove copied comment from tests. #

Total comments: 22

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

Patch Set 5 : Fix nits. #

Total comments: 8

Patch Set 6 : Fix nits. #

Created: 5 years, 6 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+515 lines, -376 lines)			Patch
	M	lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp	View	1 2 3 4	3 chunks	+79 lines, -42 lines	0 comments	Download
	M	unittests/Bitcode/NaClMungeWriteErrorTests.cpp	View	1 2 3 4 5	9 chunks	+436 lines, -334 lines	0 comments	Download

Messages

Total messages: 8 (2 generated)

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

jvoung (off chromium)

https://codereview.chromium.org/1157273004/diff/40001/lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp File lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp (right): https://codereview.chromium.org/1157273004/diff/40001/lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp#newcode121 lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:121: // Returns true if abbreviation operand is legal. If ...

5 years, 7 months ago (2015-05-27 20:09:38 UTC) #4

Karl

5 years, 6 months ago (2015-05-28 20:50:32 UTC) #5

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

https://codereview.chromium.org/1157273004/diff/40001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:121: // Returns true if
abbreviation operand is legal. If not, generates
On 2015/05/27 20:09:37, jvoung wrote:
> Maybe "logs" instead of "generates"? It's not returning a new abbrev that
works
> or anything as part of the recovery. The recovery is handled by the caller,
and
> the caller just drops the abbrev instead of building it.

Ok. Updated the comment to better reflect this.

https://codereview.chromium.org/1157273004/diff/40001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:496: size_t
NumAbbreviations = Record.Values[Index++];
On 2015/05/27 20:09:37, jvoung wrote:
> Sorry a few random cleanup suggestions as I'm re-reading this...
> 
> How about "NumAbbrevOps" instead of "NumAbbreviations" ? This is building one
> abbreviation, out of several operands. It's not building multiple abbrevs.

Good point. Changing.

https://codereview.chromium.org/1157273004/diff/40001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:509: switch
(Record.Values[Index++]) {
On 2015/05/27 20:09:37, jvoung wrote:
> Maybe stash Record.Values[Index++] in a named variable "IsLiteral" or
something
> to make it clear what the switch is for (just matching 1/0/other is less clear
I
> think).

Done.

https://codereview.chromium.org/1157273004/diff/40001/lib/Bitcode/NaCl/TestUt...
lib/Bitcode/NaCl/TestUtils/NaClBitcodeMungeWriter.cpp:555: if (Index >=
NumValues) {
On 2015/05/27 20:09:37, jvoung wrote:
> fwiw, in the other cases that have this check against Index, the code is about
> to pop another value out of Record.Values. In this case it's not about to do
> that, so seems closer to what happens with Char6. After the loop there's
already
> a check of Index vs NumValues, so this may be redundant?

I agree. Fixing.

Also, since getting values and checking size of Record.Values is so common,
factored that out into a method.

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

https://codereview.chromium.org/1157273004/diff/40001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:63: // Same as above, but just
the records as generated by NaClWriteMunger.
On 2015/05/27 20:09:37, jvoung wrote:
> I don't see a class named NaClWriteMunger.

I was trying to clarify what tool generated the output. The dump above is from
class NaClObjDumpMunger while the following is from NaClWriteMunger.

However, after thinking about it a bit more, I realized I could have just used
NaClObjDumpMunger as well. Fixing the code to do that.

Also realized that the check pattern use for abbreviations could be applied
elsewhere, removing code. So I'm also changing that.

https://codereview.chromium.org/1157273004/diff/40001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:583: // abbreviation definitions
via Edit.  ErrorMessage is the expected
On 2015/05/27 20:09:37, jvoung wrote:
> via Edit -> via Edits ?

Done.

https://codereview.chromium.org/1157273004/diff/40001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:584: // error message to be
generated by the write munger. Assumes the the
On 2015/05/27 20:09:37, jvoung wrote:
> the the -> that the

Done.

https://codereview.chromium.org/1157273004/diff/40001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:585: // error recovery is to
delete the bad abbreviation definitions.
On 2015/05/27 20:09:37, jvoung wrote:
> error recover -> error recover method
> 
> or something like that?

No longer applicable. Comments were rewritten.

https://codereview.chromium.org/1157273004/diff/40001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:603: // Insert bad abbreviation
Fixed(36) into type block.
On 2015/05/27 20:09:37, jvoung wrote:
> how about static_assert(36 > naclbitc::MaxAbbrevWidth, "...");
> 
> To show why 36 is chosen.

Done.

https://codereview.chromium.org/1157273004/diff/40001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:607: 0,
NaClBitCodeAbbrevOp::Fixed, 36, Terminator
On 2015/05/27 20:09:38, jvoung wrote:
> can line up the 0, ... with the other array elems from the earlier rows

Done.

https://codereview.chromium.org/1157273004/diff/40001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:675: // Show that the code checks
if specifies too few operands for an
On 2015/05/27 20:09:37, jvoung wrote:
> nit: "Show that the code checks if specifies too few operands" sounds a bit
odd.

Rewritten.

jvoung (off chromium)

lgtm https://codereview.chromium.org/1157273004/diff/70001/unittests/Bitcode/NaClMungeWriteErrorTests.cpp File unittests/Bitcode/NaClMungeWriteErrorTests.cpp (right): https://codereview.chromium.org/1157273004/diff/70001/unittests/Bitcode/NaClMungeWriteErrorTests.cpp#newcode96 unittests/Bitcode/NaClMungeWriteErrorTests.cpp:96: // written bitcode records. DumpedBitcode is the expected ...

5 years, 6 months ago (2015-05-29 16:43:25 UTC) #6

Karl

Committed patchset #6 (id:90001) manually as 2c9a36eceb3d8f0f8f2214f77879b65308e8c89e (presubmit successful).

5 years, 6 months ago (2015-05-29 20:01:22 UTC) #7

Karl

5 years, 6 months ago (2015-05-29 20:02:17 UTC) #8

Message was sent while issue was closed.

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

https://codereview.chromium.org/1157273004/diff/70001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:96: //  written bitcode records. 
DumpedBitcode is the expected dumped
On 2015/05/29 16:43:25, jvoung wrote:
> extra space (line up comments)

Done.

https://codereview.chromium.org/1157273004/diff/70001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:120: // simplier write munger.
On 2015/05/29 16:43:25, jvoung wrote:
> simplier -> simpler

Done.

https://codereview.chromium.org/1157273004/diff/70001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:135: // Same as CheckParseEdits,
but run the simplier write munger instead
On 2015/05/29 16:43:25, jvoung wrote:
> simplier -> simpler

Done.

https://codereview.chromium.org/1157273004/diff/70001/unittests/Bitcode/NaClM...
unittests/Bitcode/NaClMungeWriteErrorTests.cpp:136: // of the bitcode parser.
Records is the the records dumped by the
On 2015/05/29 16:43:25, jvoung wrote:
> the the -> the

Done.

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