Add support for Linux memory mapping stream and remove ELF header usage

Add support for Linux memory mapping stream and remove ELF header usage
when checking exploitability rating.

Linux minidumps do not support MD_MEMORY_INFO_LIST_STREAM, meaning the
processor cannot retrieve its memory mappings. However, it has its own
stream, MD_LINUX_MAPS, which contains memory mappings specific to Linux
(it contains the contents of /proc/self/maps). This CL allows the minidump
to gather information from the memory mappings for Linux minidumps.

In addition, exploitability rating for Linux dumps now use memory mappings
instead of checking the ELF headers of binaries. The basis for the change
is that checking the ELF headers requires the minidumps to store the memory
from the ELF headers, while the memory mapping data is already present,
meaning the size of a minidump will be unchanged.

As a result, of removing ELF header analysis, two unit tests have been removed.
Arguably, the cases that those unit tests check do not merit a high
exploitability rating and do not warrant a solid conclusion that was given
earlier.

R=ivanpe@chromium.org

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

[liuandrew, 2015-07-22 17:34:25]

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

[ivanpe, 2015-07-23 01:17:13]

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
File src/google_breakpad/processor/minidump.h (right):

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:893: explicit
MinidumpLinuxMaps(Minidump *minidump);
Please, document who should own the memory for the passed in pointer?

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:898: bool Read(vector<char> *data);
Is this an input parameter?  Passing as const vector<char>& is preferred.

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:900: // Instance variables.
This comment is not very useful.  Please, comment the individual members or
maybe just provide an URL to a place where these fields are already documented.

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:913: };
DISALLOW_COPY_AND_ASSIGN(MinidumpLinuxMaps);

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:923: const MinidumpLinuxMaps
*GetLinuxMapsForAddress(uint64_t address) const;
Please, document who owns the memory for the returned pointer (caller or
callee)!

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:925: const MinidumpLinuxMaps
*GetLinuxMapsAtIndex(unsigned int index) const;
Please, document who owns the memory for the returned pointer (caller or
callee)!

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:937: explicit
MinidumpLinuxMapsList(Minidump *minidump);
Again, please, describe who owns the passed in pointer.

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:941: bool Read(uint32_t expected_size);
Please, document return value?  What happens when the expected size is different
from the actual one?

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:943: // Instance variables.
Please, document the individual members instead.

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:947: };
DISALLOW_COPY_AND_ASSIGN(MinidumpLinuxMapsList);

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

https://codereview.chromium.org/1251593007/diff/40001/src/processor/exploitab...
src/processor/exploitability_linux.cc:250: this->LoadElfHeader(memory_region,
base_address, &header);
Please, make sure to cleanup all functions that are not needed anymore.

https://codereview.chromium.org/1251593007/diff/40001/src/processor/minidump.cc
File src/processor/minidump.cc (right):

https://codereview.chromium.org/1251593007/diff/40001/src/processor/minidump....
src/processor/minidump.cc:3991: bool MinidumpLinuxMaps::Read(vector<char> *data)
{
There is already an implementation of this in Chromium code that you should
probably reuse:
 
https://code.google.com/p/chromium/codesearch#chromium/src/base/debug/proc_ma...

The Chromium version also has plenty of tests that you should reuse as well.

[liuandrew, 2015-07-23 16:14:50]

How do I reuse the Chromium code? Do I just copy the relevant files over into
this repository, or is there a specific way to import the code?

https://codereview.chromium.org/1251593007/diff/40001/src/processor/minidump.cc
File src/processor/minidump.cc (right):

https://codereview.chromium.org/1251593007/diff/40001/src/processor/minidump....
src/processor/minidump.cc:3991: bool MinidumpLinuxMaps::Read(vector<char> *data)
{
On 2015/07/23 01:17:12, ivanpe wrote:
> There is already an implementation of this in Chromium code that you should
> probably reuse:
>  
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/debug/proc_ma...
> 
> The Chromium version also has plenty of tests that you should reuse as well.

How do I reuse the Chromium code? Do I just copy the relevant files over into
this repository, or is there a specific way to import the code?

[ivanpe, 2015-07-23 17:35:46]

On 2015/07/23 16:14:50, liuandrew wrote:
> How do I reuse the Chromium code? Do I just copy the relevant files over into
> this repository, or is there a specific way to import the code?
> 
>
https://codereview.chromium.org/1251593007/diff/40001/src/processor/minidump.cc
> File src/processor/minidump.cc (right):
> 
>
https://codereview.chromium.org/1251593007/diff/40001/src/processor/minidump....
> src/processor/minidump.cc:3991: bool MinidumpLinuxMaps::Read(vector<char>
*data)
> {
> On 2015/07/23 01:17:12, ivanpe wrote:
> > There is already an implementation of this in Chromium code that you should
> > probably reuse:
> >  
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/debug/proc_ma...
> > 
> > The Chromium version also has plenty of tests that you should reuse as well.
> 
> How do I reuse the Chromium code? Do I just copy the relevant files over into
> this repository, or is there a specific way to import the code?

You may want to create src/third_party/proc_maps or
src/third_party/linux/proc_maps and copy proc_maps_linux.* there.

[liuandrew, 2015-07-23 18:19:47]

On 2015/07/23 17:35:46, ivanpe wrote:
> On 2015/07/23 16:14:50, liuandrew wrote:
> > How do I reuse the Chromium code? Do I just copy the relevant files over
into
> > this repository, or is there a specific way to import the code?
> > 
> >
>
https://codereview.chromium.org/1251593007/diff/40001/src/processor/minidump.cc
> > File src/processor/minidump.cc (right):
> > 
> >
>
https://codereview.chromium.org/1251593007/diff/40001/src/processor/minidump....
> > src/processor/minidump.cc:3991: bool MinidumpLinuxMaps::Read(vector<char>
> *data)
> > {
> > On 2015/07/23 01:17:12, ivanpe wrote:
> > > There is already an implementation of this in Chromium code that you
should
> > > probably reuse:
> > >  
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/debug/proc_ma...
> > > 
> > > The Chromium version also has plenty of tests that you should reuse as
well.
> > 
> > How do I reuse the Chromium code? Do I just copy the relevant files over
into
> > this repository, or is there a specific way to import the code?
> 
> You may want to create src/third_party/proc_maps or
> src/third_party/linux/proc_maps and copy proc_maps_linux.* there.

Would there be any merit to the "selective copy and paste" method? Adding the
proc_maps files means I will need to also add its dependencies and have it
mirror Chromium's directory structure (or modify the files themselves), and I
will need to modify the Makefile. In addition, the "third party" owning the code
and license is the same between Breakpad and Chromium.

I was thinking of copying the parsing method and stealing the proc_map test
cases and putting them into minidump.* and minidump_unittest.cc instead of
keeping them in their original files. How does that sound?

[ivanpe, 2015-07-23 18:31:18]

On 2015/07/23 18:19:47, liuandrew wrote:
> On 2015/07/23 17:35:46, ivanpe wrote:
> > On 2015/07/23 16:14:50, liuandrew wrote:
> > > How do I reuse the Chromium code? Do I just copy the relevant files over
> into
> > > this repository, or is there a specific way to import the code?
> > > 
> > >
> >
>
https://codereview.chromium.org/1251593007/diff/40001/src/processor/minidump.cc
> > > File src/processor/minidump.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1251593007/diff/40001/src/processor/minidump....
> > > src/processor/minidump.cc:3991: bool MinidumpLinuxMaps::Read(vector<char>
> > *data)
> > > {
> > > On 2015/07/23 01:17:12, ivanpe wrote:
> > > > There is already an implementation of this in Chromium code that you
> should
> > > > probably reuse:
> > > >  
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/debug/proc_ma...
> > > > 
> > > > The Chromium version also has plenty of tests that you should reuse as
> well.
> > > 
> > > How do I reuse the Chromium code? Do I just copy the relevant files over
> into
> > > this repository, or is there a specific way to import the code?
> > 
> > You may want to create src/third_party/proc_maps or
> > src/third_party/linux/proc_maps and copy proc_maps_linux.* there.
> 
> Would there be any merit to the "selective copy and paste" method? Adding the
> proc_maps files means I will need to also add its dependencies and have it
> mirror Chromium's directory structure (or modify the files themselves), and I
> will need to modify the Makefile. In addition, the "third party" owning the
code
> and license is the same between Breakpad and Chromium.
> 
> I was thinking of copying the parsing method and stealing the proc_map test
> cases and putting them into minidump.* and minidump_unittest.cc instead of
> keeping them in their original files. How does that sound?

Copying only the parsing method and putting it next the minidump.* can also
work.  I would prefer if you keep the parsing code in a separate file though.

[liuandrew, 2015-07-24 23:25:36]

Uploaded patch set 4.

Changes include:
 - stealing the Chromium code to parse proc/<pid>/maps
 - nuking my own code that re-implemented the task above
 - editing the Makefile.am to support the Chromium files
 - recompiling the Makefile.in since I changed the Makefile.am (I don't
understand why the repository has this file; shouldn't it be removed since it
can be compiled from the other Makefile?)
 - changing my patch set to support the stolen Chromium code
 - documentation about pointer ownership
 - learned what references in C++ are
 - miscellaneous cleanup

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
File src/google_breakpad/processor/minidump.h (right):

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:893: explicit
MinidumpLinuxMaps(Minidump *minidump);
On 2015/07/23 01:17:12, ivanpe wrote:
> Please, document who should own the memory for the passed in pointer?

Done.

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:898: bool Read(vector<char> *data);
On 2015/07/23 01:17:12, ivanpe wrote:
> Is this an input parameter?  Passing as const vector<char>& is preferred.

Removed method.

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:900: // Instance variables.
On 2015/07/23 01:17:12, ivanpe wrote:
> This comment is not very useful.  Please, comment the individual members or
> maybe just provide an URL to a place where these fields are already
documented.

Removed these instance variables.

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:913: };
On 2015/07/23 01:17:12, ivanpe wrote:
> DISALLOW_COPY_AND_ASSIGN(MinidumpLinuxMaps);

Why do I need this? What does it do? The other classes in this file do not have
it.

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:923: const MinidumpLinuxMaps
*GetLinuxMapsForAddress(uint64_t address) const;
On 2015/07/23 01:17:12, ivanpe wrote:
> Please, document who owns the memory for the returned pointer (caller or
> callee)!

Done.

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:925: const MinidumpLinuxMaps
*GetLinuxMapsAtIndex(unsigned int index) const;
On 2015/07/23 01:17:12, ivanpe wrote:
> Please, document who owns the memory for the returned pointer (caller or
> callee)!

Done.

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:937: explicit
MinidumpLinuxMapsList(Minidump *minidump);
On 2015/07/23 01:17:12, ivanpe wrote:
> Again, please, describe who owns the passed in pointer.

Done.

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:941: bool Read(uint32_t expected_size);
On 2015/07/23 01:17:12, ivanpe wrote:
> Please, document return value?  What happens when the expected size is
different
> from the actual one?

Done.

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:943: // Instance variables.
On 2015/07/23 01:17:12, ivanpe wrote:
> Please, document the individual members instead.

Done.

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:947: };
On 2015/07/23 01:17:12, ivanpe wrote:
> DISALLOW_COPY_AND_ASSIGN(MinidumpLinuxMapsList);

See response for similar comment.

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

https://codereview.chromium.org/1251593007/diff/40001/src/processor/exploitab...
src/processor/exploitability_linux.cc:250: this->LoadElfHeader(memory_region,
base_address, &header);
On 2015/07/23 01:17:12, ivanpe wrote:
> Please, make sure to cleanup all functions that are not needed anymore.

Done.

https://codereview.chromium.org/1251593007/diff/40001/src/processor/minidump.cc
File src/processor/minidump.cc (right):

https://codereview.chromium.org/1251593007/diff/40001/src/processor/minidump....
src/processor/minidump.cc:3991: bool MinidumpLinuxMaps::Read(vector<char> *data)
{
On 2015/07/23 16:14:50, liuandrew wrote:
> On 2015/07/23 01:17:12, ivanpe wrote:
> > There is already an implementation of this in Chromium code that you should
> > probably reuse:
> >  
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/debug/proc_ma...
> > 
> > The Chromium version also has plenty of tests that you should reuse as well.
> 
> How do I reuse the Chromium code? Do I just copy the relevant files over into
> this repository, or is there a specific way to import the code?

Done.

[liuandrew, 2015-07-24 23:29:45]

Also, I stole the /proc/<pid>/maps unittest from Chromium as well.

And I will need to edit the BUILD file in Google3 to support these changes
internally (which I will get to in the future).

[liuandrew, 2015-07-27 18:08:48]

Google3 BUILD file has been successfully modified; the Google CL now mirrors
this patchset.

[ivanpe, 2015-07-27 18:12:42]

Some minor nits.

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
File src/google_breakpad/processor/minidump.h (right):

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:913: };
On 2015/07/24 23:25:35, liuandrew wrote:
> On 2015/07/23 01:17:12, ivanpe wrote:
> > DISALLOW_COPY_AND_ASSIGN(MinidumpLinuxMaps);
> 
> Why do I need this? What does it do? The other classes in this file do not
have
> it.

If you do not want to support copy/move operations on your type, you should
explicitly disable them using = delete or DISALLOW_COPY_AND_ASSIGN.

https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Copyable_Mov...

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:947: };
On 2015/07/24 23:25:35, liuandrew wrote:
> On 2015/07/23 01:17:12, ivanpe wrote:
> > DISALLOW_COPY_AND_ASSIGN(MinidumpLinuxMapsList);
> 
> See response for similar comment.

Please, take a look at my other response.  Thanks.

https://codereview.chromium.org/1251593007/diff/60001/src/google_breakpad/pro...
File src/google_breakpad/processor/minidump.h (right):

https://codereview.chromium.org/1251593007/diff/60001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:127: };
please, remove the whitespace after the ;

https://codereview.chromium.org/1251593007/diff/60001/src/google_breakpad/pro...
File src/google_breakpad/processor/proc_maps_linux.h (right):

https://codereview.chromium.org/1251593007/diff/60001/src/google_breakpad/pro...
src/google_breakpad/processor/proc_maps_linux.h:11: #include
"google_breakpad/common/breakpad_types.h"
#include "common/using_std_string.h"

https://codereview.chromium.org/1251593007/diff/60001/src/google_breakpad/pro...
src/google_breakpad/processor/proc_maps_linux.h:13: // Describes a region of
mapped memory and the path of the file mapped.
You should probably put this into the google_breakpad namespace:

namespace google_breakpad {

https://codereview.chromium.org/1251593007/diff/60001/src/google_breakpad/pro...
src/google_breakpad/processor/proc_maps_linux.h:44: std::string path;
Please, don't use std::string in breakpad code.  Instead, declare this as
follows:

   sting path;

There is a different string type in Google ::string != std::string.  The
inclusion of common/using_std_string.h should allow you to do this.

[liuandrew, 2015-07-27 23:09:26]

Uploaded patch set 5. Changes include:
 - putting proc_maps_linux into google_breakpad's namespace
 - disallow_copy_and_assign
 - lint errors

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
File src/google_breakpad/processor/minidump.h (right):

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:913: };
On 2015/07/27 18:12:42, ivanpe wrote:
> On 2015/07/24 23:25:35, liuandrew wrote:
> > On 2015/07/23 01:17:12, ivanpe wrote:
> > > DISALLOW_COPY_AND_ASSIGN(MinidumpLinuxMaps);
> > 
> > Why do I need this? What does it do? The other classes in this file do not
> have
> > it.
> 
> If you do not want to support copy/move operations on your type, you should
> explicitly disable them using = delete or DISALLOW_COPY_AND_ASSIGN.
> 
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Copyable_Mov...

Done.

https://codereview.chromium.org/1251593007/diff/40001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:947: };
On 2015/07/27 18:12:41, ivanpe wrote:
> On 2015/07/24 23:25:35, liuandrew wrote:
> > On 2015/07/23 01:17:12, ivanpe wrote:
> > > DISALLOW_COPY_AND_ASSIGN(MinidumpLinuxMapsList);
> > 
> > See response for similar comment.
> 
> Please, take a look at my other response.  Thanks.

Done.

https://codereview.chromium.org/1251593007/diff/60001/src/google_breakpad/pro...
File src/google_breakpad/processor/minidump.h (right):

https://codereview.chromium.org/1251593007/diff/60001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:127: };
On 2015/07/27 18:12:42, ivanpe wrote:
> please, remove the whitespace after the ;

Done.

https://codereview.chromium.org/1251593007/diff/60001/src/google_breakpad/pro...
File src/google_breakpad/processor/proc_maps_linux.h (right):

https://codereview.chromium.org/1251593007/diff/60001/src/google_breakpad/pro...
src/google_breakpad/processor/proc_maps_linux.h:11: #include
"google_breakpad/common/breakpad_types.h"
On 2015/07/27 18:12:42, ivanpe wrote:
> #include "common/using_std_string.h"

Done.

https://codereview.chromium.org/1251593007/diff/60001/src/google_breakpad/pro...
src/google_breakpad/processor/proc_maps_linux.h:13: // Describes a region of
mapped memory and the path of the file mapped.
On 2015/07/27 18:12:42, ivanpe wrote:
> You should probably put this into the google_breakpad namespace:
> 
> namespace google_breakpad {

Done.

https://codereview.chromium.org/1251593007/diff/60001/src/google_breakpad/pro...
src/google_breakpad/processor/proc_maps_linux.h:44: std::string path;
On 2015/07/27 18:12:42, ivanpe wrote:
> Please, don't use std::string in breakpad code.  Instead, declare this as
> follows:
> 
>    sting path;
> 
> There is a different string type in Google ::string != std::string.  The
> inclusion of common/using_std_string.h should allow you to do this.

Done.

[ivanpe, 2015-07-28 00:18:58]

LGTM

https://codereview.chromium.org/1251593007/diff/80001/src/processor/proc_maps...
File src/processor/proc_maps_linux.cc (right):

https://codereview.chromium.org/1251593007/diff/80001/src/processor/proc_maps...
src/processor/proc_maps_linux.cc:28:
std::vector<google_breakpad::MappedMemoryRegion>
Do you really need to specify the namespace here and below (since the code is
already in namespace google_breakpad)?

[liuandrew, 2015-07-28 00:44:03]

Uploaded patch set 6. Changes include:
 - removing unnecessary google_breakpad prefix in proc_maps_linux.cc

[liuandrew, 2015-07-28 00:53:54]

Committed patchset #6 (id:100001) manually as r1476 (presubmit successful).