Add check for Linux minidump ending on bad write for exploitability rating.

Add check for Linux minidump ending on bad write for exploitability rating.

If a crash occurred as a result to a write to unwritable memory, it is reason
to suggest exploitability. The processor checks for a bad write by
disassembling the command that caused the crash by piping the raw bytes near
the instruction pointer through objdump. This allows the processor to see if
the instruction that caused the crash is a write to memory and where the
target of the address is located.

R=ivanpe@chromium.org

Committed: https://code.google.com/p/google-breakpad/source/detail?r=1497

[liuandrew, 2015-08-06 21:03:56]

liu.andrew.x@gmail.com changed reviewers:
+ ahonig@google.com, ivanpe@chromium.org

[liuandrew, 2015-08-10 18:13:13]

Add check for Linux minidump ending on bad write for exploitability rating.

If a crash occurred as a result to a write to unwritable memory, it is reason
to suggest exploitability. The processor checks for a bad write by
disassembling the command that caused the crash by piping the raw bytes near
the instruction pointer through objdump. This allows the processor to see if
the instruction that caused the crash is a write to memory and where the
target of the address is located.

[liuandrew, 2015-08-10 18:15:45]

Uploaded patch set 2.

Changes include:
 - writing up to one instruction to objdump instead of all the remaining memory
in the instruction pointer's memory region

[ivanpe, 2015-08-10 19:58:05]

https://codereview.chromium.org/1273823004/diff/20001/src/processor/exploitab...
File src/processor/exploitability_linux.cc (right):

https://codereview.chromium.org/1273823004/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.cc:162: return 0;
The return value type is bool

https://codereview.chromium.org/1273823004/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.cc:190: const uint32_t offset =
instruction_ptr - memory_region->GetBase();
Both instruction_ptr and memory_region->GetBase() are 64-bit uints. I think
offset should also be 64-bit value.  Also, you never check whether
memory_region->GetBase() <= instruction_ptr and since the underlying type
(uint64) does not support negative numbers you may end up an integer overflow.

https://codereview.chromium.org/1273823004/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.cc:198: char raw_bytes_tmpfile[] =
"/tmp/breakpad_mem_region-raw_bytes-XXXXXX";
const char?

https://codereview.chromium.org/1273823004/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.cc:199: int raw_bytes_fd =
mkstemp(raw_bytes_tmpfile);
Please, check return value

https://codereview.chromium.org/1273823004/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.cc:240: execlp("objdump", "objdump", "-D",
"-b", "binary", "-M", "intel",
This function seems to have lots of portability issues, and I'm not sure this is
a such great idea.

 - This makes breakpad processor dependent on an external tool (objdump).
 - C fork and pipe are not very portable as well.

Isn't it possible to use a dis-assembler though a portable library?

[liuandrew, 2015-08-10 22:00:40]

Uploaded patch set 3.

Changes include:
 - minor return and overflow issues
 - some error checking
 - checking the exit status of the forked process

https://codereview.chromium.org/1273823004/diff/20001/src/processor/exploitab...
File src/processor/exploitability_linux.cc (right):

https://codereview.chromium.org/1273823004/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.cc:162: return 0;
On 2015/08/10 19:58:04, ivanpe wrote:
> The return value type is bool

Done.

https://codereview.chromium.org/1273823004/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.cc:190: const uint32_t offset =
instruction_ptr - memory_region->GetBase();
On 2015/08/10 19:58:04, ivanpe wrote:
> Both instruction_ptr and memory_region->GetBase() are 64-bit uints. I think
> offset should also be 64-bit value.  Also, you never check whether
> memory_region->GetBase() <= instruction_ptr and since the underlying type
> (uint64) does not support negative numbers you may end up an integer overflow.

Done.

https://codereview.chromium.org/1273823004/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.cc:198: char raw_bytes_tmpfile[] =
"/tmp/breakpad_mem_region-raw_bytes-XXXXXX";
On 2015/08/10 19:58:04, ivanpe wrote:
> const char?

No, it should not be const. Calling mkstemp modifies this variable, and the
result is the name of the file. It is fine to allocate this on the stack because
the method changes the "X's" to different characters, so the length of the
string stays the same.

https://codereview.chromium.org/1273823004/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.cc:199: int raw_bytes_fd =
mkstemp(raw_bytes_tmpfile);
On 2015/08/10 19:58:04, ivanpe wrote:
> Please, check return value

Done.

https://codereview.chromium.org/1273823004/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.cc:240: execlp("objdump", "objdump", "-D",
"-b", "binary", "-M", "intel",
On 2015/08/10 19:58:04, ivanpe wrote:
> This function seems to have lots of portability issues, and I'm not sure this
is
> a such great idea.
> 
>  - This makes breakpad processor dependent on an external tool (objdump).
>  - C fork and pipe are not very portable as well.
> 
> Isn't it possible to use a dis-assembler though a portable library?

There looked to be no good disassembler libraries to use. Beaengine had a
restrictive license, and Capstone had security issues. Libdisasm only supports
32-bit architectures.

Since objdump is exec'd in a child process, it dies gracefully if any
portability issues arise. Why would fork and pipe have a portability problem?

[ahonig, 2015-08-10 22:53:11]

https://codereview.chromium.org/1273823004/diff/40001/src/processor/exploitab...
File src/processor/exploitability_linux.cc (right):

https://codereview.chromium.org/1273823004/diff/40001/src/processor/exploitab...
src/processor/exploitability_linux.cc:144: bool
ExploitabilityLinux::EndedOnIllegalWrite(uint64_t instruction_ptr) {
This is a really long function, please break it into smaller pieces.

https://codereview.chromium.org/1273823004/diff/40001/src/processor/exploitab...
src/processor/exploitability_linux.cc:206: BPLOG(ERROR) << "Failed to create
tempfile.";
This should return rather than continue running with an invalid fd.

https://codereview.chromium.org/1273823004/diff/40001/src/processor/exploitab...
src/processor/exploitability_linux.cc:209: != MAX_INSTRUCTION_LEN) {
Add call to unlink and return.

https://codereview.chromium.org/1273823004/diff/40001/src/processor/exploitab...
src/processor/exploitability_linux.cc:276: } while (regexec(&regex,
line.c_str(), 0, NULL, 0));
What happens if this finishes prior to finding a line that matches your regex?

https://codereview.chromium.org/1273823004/diff/40001/src/processor/exploitab...
src/processor/exploitability_linux.cc:333: if (dest.at(0) == '[' &&
dest.at(dest.size() - 1) == ']' &&
Is this reasonable if dest is empty?

https://codereview.chromium.org/1273823004/diff/40001/src/processor/exploitab...
src/processor/exploitability_linux.cc:341: dest = dest.substr(1, dest.size() -
2);
What happens if dest is []

https://codereview.chromium.org/1273823004/diff/40001/src/processor/exploitab...
src/processor/exploitability_linux.cc:354: delim = dest.find('-');
There are definitely more complicated destinations that just reg+/reg-.  Some
examples include

mov [eax+4*ecx+5], edx

I'm not saying you need to handle that case in this change, but make sure you
handle it gracefully and put a TODO that it's not done yet.

https://codereview.chromium.org/1273823004/diff/40001/src/processor/exploitab...
File src/processor/exploitability_linux.h (right):

https://codereview.chromium.org/1273823004/diff/40001/src/processor/exploitab...
src/processor/exploitability_linux.h:62: // This method checks if the crash
occurred during a write to read-only
nit, read-only or completely invalid memory right?

[ivanpe, 2015-08-10 23:27:59]

https://codereview.chromium.org/1273823004/diff/20001/src/processor/exploitab...
File src/processor/exploitability_linux.cc (right):

https://codereview.chromium.org/1273823004/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.cc:240: execlp("objdump", "objdump", "-D",
"-b", "binary", "-M", "intel",
On 2015/08/10 22:00:40, liuandrew wrote:
> On 2015/08/10 19:58:04, ivanpe wrote:
> > This function seems to have lots of portability issues, and I'm not sure
this
> is
> > a such great idea.
> > 
> >  - This makes breakpad processor dependent on an external tool (objdump).
> >  - C fork and pipe are not very portable as well.
> > 
> > Isn't it possible to use a dis-assembler though a portable library?
> 
> There looked to be no good disassembler libraries to use. Beaengine had a
> restrictive license, and Capstone had security issues. Libdisasm only supports
> 32-bit architectures.
> 
> Since objdump is exec'd in a child process, it dies gracefully if any
> portability issues arise. Why would fork and pipe have a portability problem?

Fork and pipe will have portability issues if breakpad processor is run on a
non-unix based OS (I know some people were trying to run it on Windows).  I'm
also worried about performance.  We are starting a new process for each crash
report.  What if objdump hangs?  We should set a timeout so that if the external
process does not respond in, say, a half second, we bail and kill it.  Also, I'm
not sure whether this will work on Borg.

https://codereview.chromium.org/1273823004/diff/40001/src/processor/exploitab...
File src/processor/exploitability_linux.cc (right):

https://codereview.chromium.org/1273823004/diff/40001/src/processor/exploitab...
src/processor/exploitability_linux.cc:196: if (memory_region->GetSize() - offset
< MAX_INSTRUCTION_LEN) {
Can "memory_region->GetSize() - offset" become negative and underflow?  Maybe a
safer way to write this would be:

if (memory_region->GetSize() < offset + MAX_INSTRUCTION_LEN) {

[liuandrew, 2015-08-11 22:55:39]

Uploaded patch set 4.

Changes include:
 - continuing objdump discussion
 - error checks/fixes
 - miscellaneous comments

https://codereview.chromium.org/1273823004/diff/20001/src/processor/exploitab...
File src/processor/exploitability_linux.cc (right):

https://codereview.chromium.org/1273823004/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.cc:240: execlp("objdump", "objdump", "-D",
"-b", "binary", "-M", "intel",
On 2015/08/10 23:27:58, ivanpe wrote:
> On 2015/08/10 22:00:40, liuandrew wrote:
> > On 2015/08/10 19:58:04, ivanpe wrote:
> > > This function seems to have lots of portability issues, and I'm not sure
> this
> > is
> > > a such great idea.
> > > 
> > >  - This makes breakpad processor dependent on an external tool (objdump).
> > >  - C fork and pipe are not very portable as well.
> > > 
> > > Isn't it possible to use a dis-assembler though a portable library?
> > 
> > There looked to be no good disassembler libraries to use. Beaengine had a
> > restrictive license, and Capstone had security issues. Libdisasm only
supports
> > 32-bit architectures.
> > 
> > Since objdump is exec'd in a child process, it dies gracefully if any
> > portability issues arise. Why would fork and pipe have a portability
problem?
> 
> Fork and pipe will have portability issues if breakpad processor is run on a
> non-unix based OS (I know some people were trying to run it on Windows).  I'm
> also worried about performance.  We are starting a new process for each crash
> report.  What if objdump hangs?  We should set a timeout so that if the
external
> process does not respond in, say, a half second, we bail and kill it.  Also,
I'm
> not sure whether this will work on Borg.

I tested objdump, and it works in borg.

If I set a timeout to kill the child process, would it be ok to use this?

https://codereview.chromium.org/1273823004/diff/40001/src/processor/exploitab...
File src/processor/exploitability_linux.cc (right):

https://codereview.chromium.org/1273823004/diff/40001/src/processor/exploitab...
src/processor/exploitability_linux.cc:196: if (memory_region->GetSize() - offset
< MAX_INSTRUCTION_LEN) {
On 2015/08/10 23:27:58, ivanpe wrote:
> Can "memory_region->GetSize() - offset" become negative and underflow?  Maybe
a
> safer way to write this would be:
> 
> if (memory_region->GetSize() < offset + MAX_INSTRUCTION_LEN) {
> 

Done.

https://codereview.chromium.org/1273823004/diff/40001/src/processor/exploitab...
src/processor/exploitability_linux.cc:206: BPLOG(ERROR) << "Failed to create
tempfile.";
On 2015/08/10 22:53:11, ahonig wrote:
> This should return rather than continue running with an invalid fd.

Done.

https://codereview.chromium.org/1273823004/diff/40001/src/processor/exploitab...
src/processor/exploitability_linux.cc:209: != MAX_INSTRUCTION_LEN) {
On 2015/08/10 22:53:11, ahonig wrote:
> Add call to unlink and return.

Done.

https://codereview.chromium.org/1273823004/diff/40001/src/processor/exploitab...
src/processor/exploitability_linux.cc:276: } while (regexec(&regex,
line.c_str(), 0, NULL, 0));
On 2015/08/10 22:53:11, ahonig wrote:
> What happens if this finishes prior to finding a line that matches your regex?

What do you mean? The terminating condition for the loop is a regex match.
Otherwise, if there are no lines, we get the error and return false.

I added a comment to clarify this in the code.

https://codereview.chromium.org/1273823004/diff/40001/src/processor/exploitab...
src/processor/exploitability_linux.cc:333: if (dest.at(0) == '[' &&
dest.at(dest.size() - 1) == ']' &&
On 2015/08/10 22:53:11, ahonig wrote:
> Is this reasonable if dest is empty?

No this is not reasonable at all. I added a length check.

https://codereview.chromium.org/1273823004/diff/40001/src/processor/exploitab...
src/processor/exploitability_linux.cc:341: dest = dest.substr(1, dest.size() -
2);
On 2015/08/10 22:53:11, ahonig wrote:
> What happens if dest is []

This is addressed in the comment above.

https://codereview.chromium.org/1273823004/diff/40001/src/processor/exploitab...
src/processor/exploitability_linux.cc:354: delim = dest.find('-');
On 2015/08/10 22:53:11, ahonig wrote:
> There are definitely more complicated destinations that just reg+/reg-.  Some
> examples include
> 
> mov [eax+4*ecx+5], edx
> 
> I'm not saying you need to handle that case in this change, but make sure you
> handle it gracefully and put a TODO that it's not done yet.

Added TODO.

I'm not sure if that's the case. I tried writing in x86 some cases where the
destination address had some more complicated arithmetic, such as
multiplication. The only cases that would compile for me were the cases of a
single register and addition/subtraction.

Then, for those cases, after compiling and disassembling in objdump, the
resultant format would be a register +/- offset.

[ivanpe, 2015-08-11 23:29:43]

https://codereview.chromium.org/1273823004/diff/20001/src/processor/exploitab...
File src/processor/exploitability_linux.cc (right):

https://codereview.chromium.org/1273823004/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.cc:240: execlp("objdump", "objdump", "-D",
"-b", "binary", "-M", "intel",
On 2015/08/11 22:55:38, liuandrew wrote:
> On 2015/08/10 23:27:58, ivanpe wrote:
> > On 2015/08/10 22:00:40, liuandrew wrote:
> > > On 2015/08/10 19:58:04, ivanpe wrote:
> > > > This function seems to have lots of portability issues, and I'm not sure
> > this
> > > is
> > > > a such great idea.
> > > > 
> > > >  - This makes breakpad processor dependent on an external tool
(objdump).
> > > >  - C fork and pipe are not very portable as well.
> > > > 
> > > > Isn't it possible to use a dis-assembler though a portable library?
> > > 
> > > There looked to be no good disassembler libraries to use. Beaengine had a
> > > restrictive license, and Capstone had security issues. Libdisasm only
> supports
> > > 32-bit architectures.
> > > 
> > > Since objdump is exec'd in a child process, it dies gracefully if any
> > > portability issues arise. Why would fork and pipe have a portability
> problem?
> > 
> > Fork and pipe will have portability issues if breakpad processor is run on a
> > non-unix based OS (I know some people were trying to run it on Windows). 
I'm
> > also worried about performance.  We are starting a new process for each
crash
> > report.  What if objdump hangs?  We should set a timeout so that if the
> external
> > process does not respond in, say, a half second, we bail and kill it.  Also,
> I'm
> > not sure whether this will work on Borg.
> 
> I tested objdump, and it works in borg.
> 
> If I set a timeout to kill the child process, would it be ok to use this?

I guess that if someone wants to compile this on a platform that does not
support pipe, fork, and objdump, he could solve this by conditional compilation.

Yes, I think we definitely need to have a timeout here.  If for whatever reasons
objdump hangs, the processor must recover and continue processing reports.

https://codereview.chromium.org/1273823004/diff/60001/src/processor/exploitab...
File src/processor/exploitability_linux.cc (right):

https://codereview.chromium.org/1273823004/diff/60001/src/processor/exploitab...
src/processor/exploitability_linux.cc:144: bool
ExploitabilityLinux::EndedOnIllegalWrite(uint64_t instruction_ptr) {
This function is too big.  Please, break it down into smaller chunks that are
easily testable and also add unittests to test them.

https://codereview.chromium.org/1273823004/diff/60001/src/processor/exploitab...
src/processor/exploitability_linux.cc:260: exit(EX_OSERR);
This is not an acceptable outcome.

[liuandrew, 2015-08-17 21:34:46]

liu.andrew.x@gmail.com changed reviewers:
+ ted.mielczarek@gmail.com

[liuandrew, 2015-08-17 21:37:37]

+ted.mielczarek@gmail.com

Uploaded patch set 5.

Changes include:
 - breaking the write method into smaller methods
 - adding unit tests for the above mentioned methods
 - creating friend class for testing purposes
 - setting an alarm that kills the forked objdump process if it lasts for more
than a second

https://codereview.chromium.org/1273823004/diff/20001/src/processor/exploitab...
File src/processor/exploitability_linux.cc (right):

https://codereview.chromium.org/1273823004/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.cc:240: execlp("objdump", "objdump", "-D",
"-b", "binary", "-M", "intel",
On 2015/08/11 23:29:43, ivanpe wrote:
> On 2015/08/11 22:55:38, liuandrew wrote:
> > On 2015/08/10 23:27:58, ivanpe wrote:
> > > On 2015/08/10 22:00:40, liuandrew wrote:
> > > > On 2015/08/10 19:58:04, ivanpe wrote:
> > > > > This function seems to have lots of portability issues, and I'm not
sure
> > > this
> > > > is
> > > > > a such great idea.
> > > > > 
> > > > >  - This makes breakpad processor dependent on an external tool
> (objdump).
> > > > >  - C fork and pipe are not very portable as well.
> > > > > 
> > > > > Isn't it possible to use a dis-assembler though a portable library?
> > > > 
> > > > There looked to be no good disassembler libraries to use. Beaengine had
a
> > > > restrictive license, and Capstone had security issues. Libdisasm only
> > supports
> > > > 32-bit architectures.
> > > > 
> > > > Since objdump is exec'd in a child process, it dies gracefully if any
> > > > portability issues arise. Why would fork and pipe have a portability
> > problem?
> > > 
> > > Fork and pipe will have portability issues if breakpad processor is run on
a
> > > non-unix based OS (I know some people were trying to run it on Windows). 
> I'm
> > > also worried about performance.  We are starting a new process for each
> crash
> > > report.  What if objdump hangs?  We should set a timeout so that if the
> > external
> > > process does not respond in, say, a half second, we bail and kill it. 
Also,
> > I'm
> > > not sure whether this will work on Borg.
> > 
> > I tested objdump, and it works in borg.
> > 
> > If I set a timeout to kill the child process, would it be ok to use this?
> 
> I guess that if someone wants to compile this on a platform that does not
> support pipe, fork, and objdump, he could solve this by conditional
compilation.
> 
> Yes, I think we definitely need to have a timeout here.  If for whatever
reasons
> objdump hangs, the processor must recover and continue processing reports.

I set an alarm to kill the child process after one second.

https://codereview.chromium.org/1273823004/diff/40001/src/processor/exploitab...
File src/processor/exploitability_linux.cc (right):

https://codereview.chromium.org/1273823004/diff/40001/src/processor/exploitab...
src/processor/exploitability_linux.cc:144: bool
ExploitabilityLinux::EndedOnIllegalWrite(uint64_t instruction_ptr) {
On 2015/08/10 22:53:11, ahonig wrote:
> This is a really long function, please break it into smaller pieces.

Done.

https://codereview.chromium.org/1273823004/diff/40001/src/processor/exploitab...
File src/processor/exploitability_linux.h (right):

https://codereview.chromium.org/1273823004/diff/40001/src/processor/exploitab...
src/processor/exploitability_linux.h:62: // This method checks if the crash
occurred during a write to read-only
On 2015/08/10 22:53:11, ahonig wrote:
> nit, read-only or completely invalid memory right?

Done.

Yes, both read-only and invalid.

https://codereview.chromium.org/1273823004/diff/60001/src/processor/exploitab...
File src/processor/exploitability_linux.cc (right):

https://codereview.chromium.org/1273823004/diff/60001/src/processor/exploitab...
src/processor/exploitability_linux.cc:144: bool
ExploitabilityLinux::EndedOnIllegalWrite(uint64_t instruction_ptr) {
On 2015/08/11 23:29:43, ivanpe wrote:
> This function is too big.  Please, break it down into smaller chunks that are
> easily testable and also add unittests to test them.

Done.

https://codereview.chromium.org/1273823004/diff/60001/src/processor/exploitab...
src/processor/exploitability_linux.cc:260: exit(EX_OSERR);
On 2015/08/11 23:29:43, ivanpe wrote:
> This is not an acceptable outcome.

How so? The child process exits with a non-zero exit code, which the parent
process will catch (so it can fail the method).

[Ted Mielczarek, 2015-08-18 12:59:12]

I'm not wild about the idea of shelling out to objdump here. I understand there
aren't many good options! There is an in-tree libdasm for x86, but that doesn't
get you x86-64 support which is unfortunate. The exploitability_win code is
using that currently:
https://code.google.com/p/google-breakpad/source/browse/trunk/src/processor/e...

I could live with this if it solves your use case and your colleagues are
willing to sign off on it as long as you can try to wrap the non-portable bits
of it. I am building a copy of minidump_stackwalk with mingw for use on our
Windows test machines in CI, so I'd appreciate not having that broken. If you go
that route a simple #ifdef _WIN32 around the fork/exec bits is sufficient.

[Ted Mielczarek, 2015-08-18 13:01:02]

Just FYI, I've also experimented with shelling out to objdump to disassemble
instructions from a minidump, I wrote a special tool to do it:
https://github.com/mozilla/socorro/blob/master/minidump-stackwalk/get-minidum...

You might find some of that code informative if you decide to continue with
this.

[liuandrew, 2015-08-18 17:50:33]

On 2015/08/18 12:59:12, Ted Mielczarek wrote:
> I'm not wild about the idea of shelling out to objdump here. I understand
there
> aren't many good options! There is an in-tree libdasm for x86, but that
doesn't
> get you x86-64 support which is unfortunate. The exploitability_win code is
> using that currently:
>
https://code.google.com/p/google-breakpad/source/browse/trunk/src/processor/e...
> 
> I could live with this if it solves your use case and your colleagues are
> willing to sign off on it as long as you can try to wrap the non-portable bits
> of it. I am building a copy of minidump_stackwalk with mingw for use on our
> Windows test machines in CI, so I'd appreciate not having that broken. If you
go
> that route a simple #ifdef _WIN32 around the fork/exec bits is sufficient.

We tried looking at disassembler libraries, but as you guessed, nothing was
sufficient. Support x86-64 is important, which is why libdasm was not used.

I added preprocessor directives to route around any potential portability
issues. Please let me know if the it builds properly. I will get up a Windows
instance and try the same.

Also, does MinGW support FILE *popen(const char *command, const char *type);? If
so, I suppose we could replace fork/exec with popen.

[liuandrew, 2015-08-18 17:51:52]

Uploaded patch set 6.

Changes include:
 - proprocessor directives to prevent portability issues
 - miscellanous lint errors

[liuandrew, 2015-08-18 21:21:43]

Uploaded patch set 7.

I tried using popen on MinGW and as far as I can tell, it works. (I used a
32-bit version of Windows Server 2003.)

I replaced fork, pipe, and exec with popen and pclose. I also removed the
preprocessor directives, since the code should theoretically work for Windows
now. Can you verify that this it works?

Also, I was unable to compile Breakpad on Windows via MinGW. I had the exact
same problems as in issue 659
(https://code.google.com/p/google-breakpad/issues/detail?id=659), so I am unable
to test if patch sets 6 and 7 actually work.

[liuandrew, 2015-08-18 23:24:38]

Uploaded patch set 8.

Changes include:
 - usage of preprocessor directives (again)
 - blocking out unused/incompatible includes in Windows
 - moving constants to anonymous namespace to avoid namespace pollution

I hacked around Breakpad in Windows to get it to compile. It turns out popen
works, but MinGW doesn't support regex. As a result, I have gone back to using
preprocessor directives to route around portability concerns.

With this patch set, Breakpad compiles fine on Windows as far as I can tell.

[ivanpe, 2015-08-20 05:37:44]

https://codereview.chromium.org/1273823004/diff/100001/src/processor/exploita...
File src/processor/exploitability_linux.cc (right):

https://codereview.chromium.org/1273823004/diff/100001/src/processor/exploita...
src/processor/exploitability_linux.cc:164: bool
ExploitabilityLinux::EndedOnIllegalWrite(uint64_t instruction_ptr) {
I think that this functionality should be disabled by default.  You should add a
method to MinidumpProcessor to enable the disassemble functionality, if the user
of MinidumpProcessor desires.

https://codereview.chromium.org/1273823004/diff/100001/src/processor/exploita...
src/processor/exploitability_linux.cc:174: BPLOG(ERROR) << "No memory region
around instruction pointer.";
Please, change this to INFO.  In general we want to use ERROR only in cases
where we can file a bug as soon as we see such a message.  No memory region
around instruction seems like a valid scenario - not a bug in code.

https://codereview.chromium.org/1273823004/diff/100001/src/processor/exploita...
src/processor/exploitability_linux.cc:220: BPLOG(ERROR) << "Not enough bytes
left to guarantee complete instruction.";
Please, replace with INFO

https://codereview.chromium.org/1273823004/diff/100001/src/processor/exploita...
src/processor/exploitability_linux.cc:234: regcomp(&regex, "0:", REG_EXTENDED |
REG_NOSUB);
1. You are not checking the return value.

2. Isn't there a more lightweight way to find the first instruction, like
string::find ?

https://codereview.chromium.org/1273823004/diff/100001/src/processor/exploita...
src/processor/exploitability_linux.cc:246: BPLOG(ERROR) << "Objdump instructions
not found";
please change this to LOG

https://codereview.chromium.org/1273823004/diff/100001/src/processor/exploita...
src/processor/exploitability_linux.cc:247: return false;
Don't you need to free regex?

https://codereview.chromium.org/1273823004/diff/100001/src/processor/exploita...
src/processor/exploitability_linux.cc:257: 
Please use a single empty line here.

https://codereview.chromium.org/1273823004/diff/100001/src/processor/exploita...
src/processor/exploitability_linux.cc:409: std::cout << "LINE: " << line <<
std::endl;
Please, do not output to console.

https://codereview.chromium.org/1273823004/diff/140001/src/processor/exploita...
File src/processor/exploitability_unittest.cc (right):

https://codereview.chromium.org/1273823004/diff/140001/src/processor/exploita...
src/processor/exploitability_unittest.cc:58: context)
Please, move the parameter to a new line (+4 indent)

https://codereview.chromium.org/1273823004/diff/140001/src/processor/exploita...
src/processor/exploitability_unittest.cc:193: regcomp(&regex, "0:", REG_EXTENDED
| REG_NOSUB);
I don't see where you free the regex

[liuandrew, 2015-08-20 17:11:51]

Uploaded patch set 9.

Changes include:
 - miscellaneous changes
 - making the illegal write check an optional feature that the user specifies
before checking exploitability

This patch should not break the build through MinGW. Also, this is my last week
at Google, so it would be nice to have this resolved by tomorrow.

https://codereview.chromium.org/1273823004/diff/100001/src/processor/exploita...
File src/processor/exploitability_linux.cc (right):

https://codereview.chromium.org/1273823004/diff/100001/src/processor/exploita...
src/processor/exploitability_linux.cc:164: bool
ExploitabilityLinux::EndedOnIllegalWrite(uint64_t instruction_ptr) {
On 2015/08/20 05:37:44, ivanpe wrote:
> I think that this functionality should be disabled by default.  You should add
a
> method to MinidumpProcessor to enable the disassemble functionality, if the
user
> of MinidumpProcessor desires.

Done.

https://codereview.chromium.org/1273823004/diff/100001/src/processor/exploita...
src/processor/exploitability_linux.cc:174: BPLOG(ERROR) << "No memory region
around instruction pointer.";
On 2015/08/20 05:37:44, ivanpe wrote:
> Please, change this to INFO.  In general we want to use ERROR only in cases
> where we can file a bug as soon as we see such a message.  No memory region
> around instruction seems like a valid scenario - not a bug in code.

Done.

https://codereview.chromium.org/1273823004/diff/100001/src/processor/exploita...
src/processor/exploitability_linux.cc:220: BPLOG(ERROR) << "Not enough bytes
left to guarantee complete instruction.";
On 2015/08/20 05:37:43, ivanpe wrote:
> Please, replace with INFO

Done.

https://codereview.chromium.org/1273823004/diff/100001/src/processor/exploita...
src/processor/exploitability_linux.cc:234: regcomp(&regex, "0:", REG_EXTENDED |
REG_NOSUB);
On 2015/08/20 05:37:44, ivanpe wrote:
> 1. You are not checking the return value.
> 
> 2. Isn't there a more lightweight way to find the first instruction, like
> string::find ?

Done.

https://codereview.chromium.org/1273823004/diff/100001/src/processor/exploita...
src/processor/exploitability_linux.cc:246: BPLOG(ERROR) << "Objdump instructions
not found";
On 2015/08/20 05:37:44, ivanpe wrote:
> please change this to LOG

Done.

https://codereview.chromium.org/1273823004/diff/100001/src/processor/exploita...
src/processor/exploitability_linux.cc:247: return false;
On 2015/08/20 05:37:44, ivanpe wrote:
> Don't you need to free regex?

Removed since I'm now using string::find.

https://codereview.chromium.org/1273823004/diff/100001/src/processor/exploita...
src/processor/exploitability_linux.cc:257: 
On 2015/08/20 05:37:44, ivanpe wrote:
> Please use a single empty line here.

Done.

https://codereview.chromium.org/1273823004/diff/100001/src/processor/exploita...
src/processor/exploitability_linux.cc:409: std::cout << "LINE: " << line <<
std::endl;
On 2015/08/20 05:37:44, ivanpe wrote:
> Please, do not output to console.

Done.

My bad, I accidentally left it here while debugging.

https://codereview.chromium.org/1273823004/diff/140001/src/processor/exploita...
File src/processor/exploitability_unittest.cc (right):

https://codereview.chromium.org/1273823004/diff/140001/src/processor/exploita...
src/processor/exploitability_unittest.cc:58: context)
On 2015/08/20 05:37:44, ivanpe wrote:
> Please, move the parameter to a new line (+4 indent)

Done.

https://codereview.chromium.org/1273823004/diff/140001/src/processor/exploita...
src/processor/exploitability_unittest.cc:193: regcomp(&regex, "0:", REG_EXTENDED
| REG_NOSUB);
On 2015/08/20 05:37:44, ivanpe wrote:
> I don't see where you free the regex

I'm now using string::npos instead of regex here.

[ivanpe, 2015-08-20 21:17:51]

This is better. A few more nits.

https://codereview.chromium.org/1273823004/diff/180001/src/google_breakpad/pr...
File src/google_breakpad/processor/exploitability.h (right):

https://codereview.chromium.org/1273823004/diff/180001/src/google_breakpad/pr...
src/google_breakpad/processor/exploitability.h:56: static Exploitability
*ExploitabilityForPlatform(Minidump *dump,
Can you please, describe the parameters.  More specifically, people need to know
what would be consequences of enabling objdump.

https://codereview.chromium.org/1273823004/diff/180001/src/processor/exploita...
File src/processor/exploitability_linux.cc (right):

https://codereview.chromium.org/1273823004/diff/180001/src/processor/exploita...
src/processor/exploitability_linux.cc:241: // Loop until the line matches the
regex or there are no lines left.
Please, update the comment.

[liuandrew, 2015-08-20 21:59:57]

Uploaded patch set 11.

Changes include:
 - addressing/resolving all nits

https://codereview.chromium.org/1273823004/diff/180001/src/google_breakpad/pr...
File src/google_breakpad/processor/exploitability.h (right):

https://codereview.chromium.org/1273823004/diff/180001/src/google_breakpad/pr...
src/google_breakpad/processor/exploitability.h:56: static Exploitability
*ExploitabilityForPlatform(Minidump *dump,
On 2015/08/20 21:17:51, ivanpe wrote:
> Can you please, describe the parameters.  More specifically, people need to
know
> what would be consequences of enabling objdump.

Done.

https://codereview.chromium.org/1273823004/diff/180001/src/processor/exploita...
File src/processor/exploitability_linux.cc (right):

https://codereview.chromium.org/1273823004/diff/180001/src/processor/exploita...
src/processor/exploitability_linux.cc:241: // Loop until the line matches the
regex or there are no lines left.
On 2015/08/20 21:17:51, ivanpe wrote:
> Please, update the comment.

Done.

[ivanpe, 2015-08-21 00:01:02]

For some reason I don't see your latest patch.  I'll approve, assuming you'll
fix the nits.

LGTM

[liuandrew, 2015-08-21 16:22:32]

Committed patchset #12 (id:220001) manually as r1497 (presubmit successful).