Added libdisasm to the repository. This library is no longer under developmen...

Added libdisasm to the repository. This library is no longer under development
so there
is no reason not to keep it locally. Implemented a basic disassembler which can
be used
to scan bytecode for interesting conditions. This should be pretty easy to add
to for
things other than exploitability if there is a desire. This also adds several
tests to
the windows exploitability ranking code to take advantage of the disassembler
for x86
code.

BUG=None
TEST=DisassemblerX86Test.*


Committed: http://code.google.com/p/google-breakpad/source/detail?r=705

[Cris Neckar]

This one is kind of a beast but the vast majority is just incorporating
libdisasm as we discussed. Thanks for all the help.

[mochalatte]

http://breakpad.appspot.com/203001/diff/1/3
File Makefile.in (right):

http://breakpad.appspot.com/203001/diff/1/3#newcode196
Line 196: $(CXXFLAGS) $(src_libbreakpad_la_LDFLAGS) $(LDFLAGS) -o $@
just to be up front: i'm far from an automake ninja, but what is special about
this command line that automake wouldn't put it together automatically, rather
than having to provide the link command manually?

http://breakpad.appspot.com/203001/diff/1/6
File src/processor/disassembler_x86.cc (right):

http://breakpad.appspot.com/203001/diff/1/6#newcode30
Line 30: #include <string.h>
nit: alphabetical order for includes

http://breakpad.appspot.com/203001/diff/1/7
File src/processor/disassembler_x86.h (right):

http://breakpad.appspot.com/203001/diff/1/7#newcode58
Line 58: u_int32_t NextInstruction();
please add comments to the method declarations

http://breakpad.appspot.com/203001/diff/1/7#newcode70
Line 70: unsigned char *_bytecode;
where did the convention of a preceding _ come from?

http://breakpad.appspot.com/203001/diff/1/8
File src/processor/disassembler_x86_unittest.cc (right):

http://breakpad.appspot.com/203001/diff/1/8#newcode1
Line 1: // copyright notice, this list of conditions and the following
disclaimer
fix copyright notice

http://breakpad.appspot.com/203001/diff/1/8#newcode224
Line 224: }
nit: i think you can scrap this main and use the one provided by google test

http://breakpad.appspot.com/203001/diff/1/9
File src/processor/exploitability_win.cc (right):

http://breakpad.appspot.com/203001/diff/1/9#newcode68
Line 68: static const size_t kMemoryDumpBytes   = 2048;
since this is related to the disassembler & PC in particular could it be named
something more specific?

http://breakpad.appspot.com/203001/diff/1/9#newcode210
Line 210: if (memory_size) {
just a thought - since the disassembler wrapper is written for the Breakpad
tree, you might want to have the wrapper take a MinidumpMemory region directly
and have it be able to iterate over the memory region, rather than copying the
instruction memory to an array and then passing it to the disassembler - this
way, the disassembler can get wider use without having to deal with the copying
everywhere it is used.

[Cris Neckar]

http://breakpad.appspot.com/203001/diff/1/3
File Makefile.in (right):

http://breakpad.appspot.com/203001/diff/1/3#newcode196
Line 196: $(CXXFLAGS) $(src_libbreakpad_la_LDFLAGS) $(LDFLAGS) -o $@
On 2010/09/29 16:08:08, mochalatte wrote:
> just to be up front: i'm far from an automake ninja, but what is special about
> this command line that automake wouldn't put it together automatically, rather
> than having to provide the link command manually?


So this file is generated by automake from Makefile.am. The substantiative
changes will be in that file. I included the new version of Makefile.in in the
CL since it was already in the repository. My only real change to Makefile.am
aside from adding libdisasm was to include a switch in the libbreakpad build to
statically link in libdisasm.

http://breakpad.appspot.com/203001/diff/1/6
File src/processor/disassembler_x86.cc (right):

http://breakpad.appspot.com/203001/diff/1/6#newcode30
Line 30: #include <string.h>
On 2010/09/29 16:08:08, mochalatte wrote:
> nit: alphabetical order for includes

Done.

http://breakpad.appspot.com/203001/diff/1/7
File src/processor/disassembler_x86.h (right):

http://breakpad.appspot.com/203001/diff/1/7#newcode58
Line 58: u_int32_t NextInstruction();
On 2010/09/29 16:08:08, mochalatte wrote:
> please add comments to the method declarations

Done.

http://breakpad.appspot.com/203001/diff/1/7#newcode70
Line 70: unsigned char *_bytecode;
On 2010/09/29 16:08:08, mochalatte wrote:
> where did the convention of a preceding _ come from?

not sure why I started doing this :/ I changed them.

http://breakpad.appspot.com/203001/diff/1/8
File src/processor/disassembler_x86_unittest.cc (right):

http://breakpad.appspot.com/203001/diff/1/8#newcode1
Line 1: // copyright notice, this list of conditions and the following
disclaimer
On 2010/09/29 16:08:08, mochalatte wrote:
> fix copyright notice

Done.

http://breakpad.appspot.com/203001/diff/1/8#newcode224
Line 224: }
On 2010/09/29 16:08:08, mochalatte wrote:
> nit: i think you can scrap this main and use the one provided by google test

Done.

http://breakpad.appspot.com/203001/diff/1/9
File src/processor/exploitability_win.cc (right):

http://breakpad.appspot.com/203001/diff/1/9#newcode68
Line 68: static const size_t kMemoryDumpBytes   = 2048;
On 2010/09/29 16:08:08, mochalatte wrote:
> since this is related to the disassembler & PC in particular could it be named
> something more specific?

Done.

http://breakpad.appspot.com/203001/diff/1/9#newcode210
Line 210: if (memory_size) {
On 2010/09/29 16:08:08, mochalatte wrote:
> just a thought - since the disassembler wrapper is written for the Breakpad
> tree, you might want to have the wrapper take a MinidumpMemory region directly
> and have it be able to iterate over the memory region, rather than copying the
> instruction memory to an array and then passing it to the disassembler - this
> way, the disassembler can get wider use without having to deal with the
copying
> everywhere it is used.

I agree that this would be nice and I may implement this when I create a bunch
of dumps to test on. For the moment it would be difficult to do testing as I
don't have dumps to test against. For now I will add a TODO.

[Ted Mielczarek]

http://breakpad.appspot.com/203001/diff/9001/10001
File Makefile.am (right):

http://breakpad.appspot.com/203001/diff/9001/10001#newcode334
Line 334: -I$(top_srcdir)/src/testing/include \
Stick a -I$(top-srcdir)/src here please (I had to fix that for other tests so
they'd compile if you configure outside the srcdir).

http://breakpad.appspot.com/203001/diff/9001/10005
File src/processor/disassembler_x86.cc (right):

http://breakpad.appspot.com/203001/diff/9001/10005#newcode35
Line 35: u_int32_t virtual_address) :
Can you just list all the method params each on their own line? I think that'd
be easier to scan here.

http://breakpad.appspot.com/203001/diff/9001/10005#newcode145
Line 145: // When we have a register marked as tainted check if we are pushing
it.
First-person pronouns.

http://breakpad.appspot.com/203001/diff/9001/10005#newcode164
Line 164: // information to determine this.
First-person pronouns.

http://breakpad.appspot.com/203001/diff/9001/10006
File src/processor/disassembler_x86.h (right):

http://breakpad.appspot.com/203001/diff/9001/10006#newcode59
Line 59: DisassemblerX86(unsigned char *bytecode, u_int32_t, u_int32_t);
I'd probably use u_int8_t* here for consistency.

http://breakpad.appspot.com/203001/diff/9001/10008
File src/processor/exploitability_win.cc (right):

http://breakpad.appspot.com/203001/diff/9001/10008#newcode68
Line 68: static const size_t kDisassembleBytesBeyondPC = 2048;
I'd probably say "maximum number of bytes" here, since by default you only get
128 bytes on either side.

http://breakpad.appspot.com/203001/diff/9001/10008#newcode198
Line 198: u_int8_t *raw_memory = new u_int8_t[kDisassembleBytesBeyondPC];
You should use a vector<u_int8_t> here instead to avoid the manual memory
management. You can use &v[0] to pass it around as a pointer (as long as you
check that the length is > 0).

http://breakpad.appspot.com/203001/diff/9001/10008#newcode207
Line 207: memory_size++;
Why don't you just use MinidumpMemoryRegion::GetBase() / ::GetSize() here to
calculate the number of bytes you have available?

http://breakpad.appspot.com/203001/diff/9001/10008#newcode212
Line 212: new DisassemblerX86(raw_memory, memory_size, instruction_ptr);
Is there a reason you're heap allocating this instead of just using it on the
stack? (Also you don't ever free it.)

http://breakpad.appspot.com/203001/diff/9001/10008#newcode219
Line 219: // We check if the faulting instruction falls into one of
Mark sez that comments are supposed to avoid first-person pronouns.

http://breakpad.appspot.com/203001/diff/9001/10008#newcode222
Line 222: case insn_controlflow:
The namespace pollution from all these enums is kind of gross (as is the
all-lowercase naming). Do you think maybe wrapping the whole thing in a
"namespace libdis" would be feasible? (Even adding an extra header with the
namespace declaration that pulls in the libdis headers seems like it'd be a step
up.)

http://breakpad.appspot.com/203001/diff/9001/10008#newcode231
Line 231: // near future. Multiple flags may be set so we want to
First-person pronouns here too.

http://breakpad.appspot.com/203001/diff/9001/10010
File src/third_party/libdisasm/.deps/ia32_implicit.Plo (right):

http://breakpad.appspot.com/203001/diff/9001/10010#newcode1
Line 1: src/third_party/libdisasm/ia32_implicit.lo \
Looks like you accidentally added the .deps dir here, make sure you don't commit
that.

[cdn]

http://breakpad.appspot.com/203001/diff/9001/10001
File Makefile.am (right):

http://breakpad.appspot.com/203001/diff/9001/10001#newcode334
Line 334: -I$(top_srcdir)/src/testing/include \
On 2010/09/30 14:15:06, Ted Mielczarek wrote:
> Stick a -I$(top-srcdir)/src here please (I had to fix that for other tests so
> they'd compile if you configure outside the srcdir).

Done.

http://breakpad.appspot.com/203001/diff/9001/10005
File src/processor/disassembler_x86.cc (right):

http://breakpad.appspot.com/203001/diff/9001/10005#newcode35
Line 35: u_int32_t virtual_address) :
On 2010/09/30 14:15:06, Ted Mielczarek wrote:
> Can you just list all the method params each on their own line? I think that'd
> be easier to scan here.

Done.

http://breakpad.appspot.com/203001/diff/9001/10005#newcode145
Line 145: // When we have a register marked as tainted check if we are pushing
it.
On 2010/09/30 14:15:06, Ted Mielczarek wrote:
> First-person pronouns.

Done.

http://breakpad.appspot.com/203001/diff/9001/10005#newcode164
Line 164: // information to determine this.
On 2010/09/30 14:15:06, Ted Mielczarek wrote:
> First-person pronouns.

Done.

http://breakpad.appspot.com/203001/diff/9001/10006
File src/processor/disassembler_x86.h (right):

http://breakpad.appspot.com/203001/diff/9001/10006#newcode59
Line 59: DisassemblerX86(unsigned char *bytecode, u_int32_t, u_int32_t);
On 2010/09/30 14:15:06, Ted Mielczarek wrote:
> I'd probably use u_int8_t* here for consistency.

Done.

http://breakpad.appspot.com/203001/diff/9001/10008
File src/processor/exploitability_win.cc (right):

http://breakpad.appspot.com/203001/diff/9001/10008#newcode68
Line 68: static const size_t kDisassembleBytesBeyondPC = 2048;
On 2010/09/30 14:15:06, Ted Mielczarek wrote:
> I'd probably say "maximum number of bytes" here, since by default you only get
> 128 bytes on either side.

Done.

http://breakpad.appspot.com/203001/diff/9001/10008#newcode198
Line 198: u_int8_t *raw_memory = new u_int8_t[kDisassembleBytesBeyondPC];
On 2010/09/30 14:15:06, Ted Mielczarek wrote:
> You should use a vector<u_int8_t> here instead to avoid the manual memory
> management. You can use &v[0] to pass it around as a pointer (as long as you
> check that the length is > 0).

Done.

http://breakpad.appspot.com/203001/diff/9001/10008#newcode207
Line 207: memory_size++;
On 2010/09/30 14:15:06, Ted Mielczarek wrote:
> Why don't you just use MinidumpMemoryRegion::GetBase() / ::GetSize() here to
> calculate the number of bytes you have available?

Not sure I understand what you are saying here. We need to call
GetMemoryAtAddress() anyway so is it not reasonable to just use the failure of
this as the bounds for the loop?

http://breakpad.appspot.com/203001/diff/9001/10008#newcode212
Line 212: new DisassemblerX86(raw_memory, memory_size, instruction_ptr);
On 2010/09/30 14:15:06, Ted Mielczarek wrote:
> Is there a reason you're heap allocating this instead of just using it on the
> stack? (Also you don't ever free it.)

Done.

http://breakpad.appspot.com/203001/diff/9001/10008#newcode219
Line 219: // We check if the faulting instruction falls into one of
On 2010/09/30 14:15:06, Ted Mielczarek wrote:
> Mark sez that comments are supposed to avoid first-person pronouns.

Done.

http://breakpad.appspot.com/203001/diff/9001/10008#newcode222
Line 222: case insn_controlflow:
On 2010/09/30 14:15:06, Ted Mielczarek wrote:
> The namespace pollution from all these enums is kind of gross (as is the
> all-lowercase naming). Do you think maybe wrapping the whole thing in a
> "namespace libdis" would be feasible? (Even adding an extra header with the
> namespace declaration that pulls in the libdis headers seems like it'd be a
step
> up.)

Done.

http://breakpad.appspot.com/203001/diff/9001/10008#newcode231
Line 231: // near future. Multiple flags may be set so we want to
On 2010/09/30 14:15:06, Ted Mielczarek wrote:
> First-person pronouns here too.

Done.

[Ted Mielczarek]

http://breakpad.appspot.com/203001/diff/9001/10008
File src/processor/exploitability_win.cc (right):

http://breakpad.appspot.com/203001/diff/9001/10008#newcode207
Line 207: memory_size++;
On 2010/09/30 21:33:33, cdn wrote:
> On 2010/09/30 14:15:06, Ted Mielczarek wrote:
> > Why don't you just use MinidumpMemoryRegion::GetBase() / ::GetSize() here to
> > calculate the number of bytes you have available?
> 
> Not sure I understand what you are saying here. We need to call
> GetMemoryAtAddress() anyway so is it not reasonable to just use the failure of
> this as the bounds for the loop? 

You can just use MinidumpMemoryRegion::GetMemory() and memcpy from it, no need
to call GetMemoryAtAddress(). (You can calculate the offset you want from
instruction_ptr - instruction_region->GetBase().)

[Cris Neckar]

Thanks Ted! 

I also added tests for exploitability_win and fixed a few functional bugs that
they flushed out.

[SiyangXie]

http://breakpad.appspot.com/203001/diff/30001/31055
File src/third_party/libdisasm/x86_format.c (right):

http://breakpad.appspot.com/203001/diff/30001/31055#newcode296
Line 296: if (! (int) op->flags & 0xF00 ) {
Hey, I was trying to compiler this with blaze, but I saw a warning "suggest
parentheses ...".  Then I took a look at the condition, it got me wondering, do
you really mean:

if ( ! ((int)op->flags & 0xF00) ) ?  Operator '!' has a higher precedence than
operator '&'.