Add ELF header analysis when checking for instruction pointer in code.

Add ELF header analysis when checking for instruction pointer in code.

If the minidump module containing the instruction pointer has memory
containing the ELF header and program header table, when checking the
exploitability rating, the processor will use the ELF header data to determine
if the instruction pointer lies in an executable region of the module, rather
than just checking if it lies in a module.

R=ivanpe@chromium.org

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

[liuandrew, 2015-07-13 23:20:43]

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

[ivanpe, 2015-07-14 00:23:02]

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
File src/processor/exploitability_linux.cc (right):

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:145: BPLOG(INFO) << "Unsupported
architecture.";
Instead of bool, you should consider returning an enumeration with a distinct
value for "unsupported architecture".  Otherwise the caller will assume that the
unsupported architecture is 64-bit.

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:171: // If the instruction pointer isn't
in a module, we can return false.
Please, don't use "we" in comments. Comments should be written in 3rd person.
e.g:

// If the instruction pointer isn't in a module, return false.

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:198: // Set 32-bit ELF header and program
header table.
Instead of "Set" maybe is s better to say "Load" or "Get".

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:199: Elf32_Ehdr *header =
this->LoadElf32Header(memory_region, base_address);
Do we need to worry about ownership transfer here?

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:214: Elf32_Phdr program_header =
program_headers[i];
Please, use const Elf32_Phdr& to avoid copy

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:217: program_header.p_vaddr +
program_header.p_memsz < instruction_ptr) {
This check seems wrong to me.  It can only be true for negative
program_header.p_memsz

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:218: free(header);
I noticed that you free header here but you don't free it when returning on line
204.

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:221: return program_header.p_flags & 1;
Instead of the literal 1, isn't there a constant that can be used here.

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:264: void
*ExploitabilityLinux::LoadElfHeader(MinidumpMemoryRegion *memory,
I would suggest replacing this with a template method where the type of the
header will be passed as a template parameter.  This will allow you to replace
malloc for new and then the callers can use scoped_ptr (similar to
std::unique_ptr) to handle ownership and proper cleanup.

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
File src/processor/exploitability_linux.h (right):

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.h:64: bool Architecture32Bit();
Maybe a better name would be: Is32BitArchitecture()?

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.h:66: // Copies ELF header data from the
minidump's memory to a pointer.
It is a bit unclear which ones are input and which ones are output parameters. 
Can you also document the return value?  Is it a buffer?   What is its size?  Is
there a transfer of ownership?

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.h:76: Elf32_Ehdr
*LoadElf32Header(MinidumpMemoryRegion *memory,
Please, document the return value and clarify whether there is a transfer of
ownership.

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.h:82: Elf64_Ehdr
*LoadElf64Header(MinidumpMemoryRegion *memory,
Please, document the return value and clarify whether there is a transfer of
ownership.

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.h:90: Elf32_Phdr
*LoadElf32PHeader(MinidumpMemoryRegion *memory,
Please, document the return value and clarify whether there is a transfer of
ownership.

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.h:101: Elf64_Phdr
*LoadElf64PHeader(MinidumpMemoryRegion *memory,
Please, document the return value and clarify whether there is a transfer of
ownership.

[liuandrew, 2015-07-14 00:43:25]

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

[liuandrew, 2015-07-14 00:43:43]

+ahonig, srutherford

[ahonig, 2015-07-14 19:32:18]

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
File src/processor/exploitability_linux.cc (right):

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:133: // GetContextCPU() should have
already been successfully called before
Add a check to make sure this is the case.  If the context is null then you
should either have the program die gracefully or return false and log an error
message.

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:200: assert(header->e_phentsize ==
sizeof(Elf32_Phdr));
assert is compiled out in non-debug builds.  If this is important make it an if
statement.

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:218: free(header);
On 2015/07/14 00:23:01, ivanpe wrote:
> I noticed that you free header here but you don't free it when returning on
line
> 204.

The pattern of free is pretty complicated and error prone.  Is c++ 11
(std::unique_ptr) allowed in this file?

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
File src/processor/exploitability_linux.h (right):

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.h:66: // Copies ELF header data from the
minidump's memory to a pointer.
On 2015/07/14 00:23:02, ivanpe wrote:
> It is a bit unclear which ones are input and which ones are output parameters.

> Can you also document the return value?  Is it a buffer?   What is its size? 
Is
> there a transfer of ownership?

To clarify Ivan's point

It looks from the interface that this function allocates memory and returns the
allocated chunk which the caller must free.  You should explicitly state that.  

I think a much cleaner interface would be to have the caller allocate the
memory.  It's almost always cleaner to have code that deletes the memory as
close to the allocation as possible.  In this case the caller knows the size, so
I don't see why you can't have the caller allocate the memory.

[liuandrew, 2015-07-15 21:50:14]

Submitted patchset 2.

Changes include using scoped_ptrs and templated methods and other miscellaneous
fixes.

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
File src/processor/exploitability_linux.cc (right):

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:133: // GetContextCPU() should have
already been successfully called before
On 2015/07/14 19:32:18, ahonig wrote:
> Add a check to make sure this is the case.  If the context is null then you
> should either have the program die gracefully or return false and log an error
> message.

Done.

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:145: BPLOG(INFO) << "Unsupported
architecture.";
On 2015/07/14 00:23:02, ivanpe wrote:
> Instead of bool, you should consider returning an enumeration with a distinct
> value for "unsupported architecture".  Otherwise the caller will assume that
the
> unsupported architecture is 64-bit.

Done.

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:171: // If the instruction pointer isn't
in a module, we can return false.
On 2015/07/14 00:23:02, ivanpe wrote:
> Please, don't use "we" in comments. Comments should be written in 3rd person.
> e.g:
> 
> // If the instruction pointer isn't in a module, return false.
> 

Done.

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:198: // Set 32-bit ELF header and program
header table.
On 2015/07/14 00:23:02, ivanpe wrote:
> Instead of "Set" maybe is s better to say "Load" or "Get".

Done.

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:199: Elf32_Ehdr *header =
this->LoadElf32Header(memory_region, base_address);
On 2015/07/14 00:23:02, ivanpe wrote:
> Do we need to worry about ownership transfer here?

using scoped_ptr

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:200: assert(header->e_phentsize ==
sizeof(Elf32_Phdr));
On 2015/07/14 19:32:18, ahonig wrote:
> assert is compiled out in non-debug builds.  If this is important make it an
if
> statement.

Done.

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:217: program_header.p_vaddr +
program_header.p_memsz < instruction_ptr) {
On 2015/07/14 00:23:02, ivanpe wrote:
> This check seems wrong to me.  It can only be true for negative
> program_header.p_memsz

My bad. I flipped the equality signs. Fixed and added a relevant unit test.

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:218: free(header);
On 2015/07/14 00:23:01, ivanpe wrote:
> I noticed that you free header here but you don't free it when returning on
line
> 204.

Done.

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:221: return program_header.p_flags & 1;
On 2015/07/14 00:23:01, ivanpe wrote:
> Instead of the literal 1, isn't there a constant that can be used here.

Done.

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:264: void
*ExploitabilityLinux::LoadElfHeader(MinidumpMemoryRegion *memory,
On 2015/07/14 00:23:02, ivanpe wrote:
> I would suggest replacing this with a template method where the type of the
> header will be passed as a template parameter.  This will allow you to replace
> malloc for new and then the callers can use scoped_ptr (similar to
> std::unique_ptr) to handle ownership and proper cleanup. 

Done.

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
File src/processor/exploitability_linux.h (right):

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.h:64: bool Architecture32Bit();
On 2015/07/14 00:23:02, ivanpe wrote:
> Maybe a better name would be: Is32BitArchitecture()?

I changed the method name to ArchitectureType() to better match the enum that it
returns.

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.h:66: // Copies ELF header data from the
minidump's memory to a pointer.
On 2015/07/14 19:32:18, ahonig wrote:
> On 2015/07/14 00:23:02, ivanpe wrote:
> > It is a bit unclear which ones are input and which ones are output
parameters.
> 
> > Can you also document the return value?  Is it a buffer?   What is its size?

> Is
> > there a transfer of ownership?
> 
> To clarify Ivan's point
> 
> It looks from the interface that this function allocates memory and returns
the
> allocated chunk which the caller must free.  You should explicitly state that.
 
> 
> I think a much cleaner interface would be to have the caller allocate the
> memory.  It's almost always cleaner to have code that deletes the memory as
> close to the allocation as possible.  In this case the caller knows the size,
so
> I don't see why you can't have the caller allocate the memory.

Done.

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.h:76: Elf32_Ehdr
*LoadElf32Header(MinidumpMemoryRegion *memory,
On 2015/07/14 00:23:02, ivanpe wrote:
> Please, document the return value and clarify whether there is a transfer of
> ownership.

method removed

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.h:82: Elf64_Ehdr
*LoadElf64Header(MinidumpMemoryRegion *memory,
On 2015/07/14 00:23:02, ivanpe wrote:
> Please, document the return value and clarify whether there is a transfer of
> ownership.

method removed

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.h:90: Elf32_Phdr
*LoadElf32PHeader(MinidumpMemoryRegion *memory,
On 2015/07/14 00:23:02, ivanpe wrote:
> Please, document the return value and clarify whether there is a transfer of
> ownership.

method removed

https://codereview.chromium.org/1233973002/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.h:101: Elf64_Phdr
*LoadElf64PHeader(MinidumpMemoryRegion *memory,
On 2015/07/14 00:23:02, ivanpe wrote:
> Please, document the return value and clarify whether there is a transfer of
> ownership.

method removed

[liuandrew, 2015-07-16 00:23:46]

Add ELF header analysis when checking for instruction pointer in code.

If the minidump module containing the instruction pointer has memory
containing the ELF header and program header table, when checking the
exploitability rating, the processor will use the ELF header data to determine
if the instruction pointer lies in an executable region of the module, rather
than just checking if it lies in a module.

[liuandrew, 2015-07-16 00:26:43]

Uploaded patch set 3. Basically I forgot to run lint before I uploaded patch set
2, so I just did some clean up.

Changes include:
 - reinterpret_cast instead of generic C-style cast
 - making scoped_ptr const as method parameter
 - whitespace and 80 character rule fixes

[ivanpe, 2015-07-16 00:45:23]

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

https://codereview.chromium.org/1233973002/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.cc:218: scoped_ptr<Elf32_Ehdr> header(new
Elf32_Ehdr);
Is this header needed only in the scope of this method?  If yes, then you can
allocate it on the stack:

  Elf32_Ehdr header;

https://codereview.chromium.org/1233973002/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.cc:251: scoped_ptr<Elf64_Ehdr> header(new
Elf64_Ehdr);
Elf64_Ehdr header;

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

https://codereview.chromium.org/1233973002/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.h:85: void LoadElfHeader(scoped_ptr<T>&
header,
Please, move the out parameter to the end.  Also the type of the out parameter
should be T*.  No need to pass a reference to a scoped_ptr in this case.

https://codereview.chromium.org/1233973002/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.h:104: uint16_t e_phnum) {
Please, update the function signature as follows:

   template<typename T>
   void LoadElfHeaderTable(MinidumpMemoryRegion *memory,
                           uint64_t base_address,
                           uint16_t e_phnum,
                           T* table)   // or   T table[]

https://codereview.chromium.org/1233973002/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.h:111: offset++;
This is a bit scary.  I'm concerned about padding at the end of the struct (due
to the alignment requirements of T). The next element in the table[] may not
necessarily be adjacent in memory to the previous one.  I think that the n-th
table entry must be written at address &table[n].  Or we should assert that
&table[n] == table.get() + offset when starting a new table entry.

[liuandrew, 2015-07-16 17:07:02]

Uploaded patch set 4.

Changes include:
 - appropriate usage of scoped pointers (namely passing normal pointers as
method parameters)
 - usage of static memory for ELF headers
 - moving the out parameters of functions to be the last parameter
 - avoiding risk of offset padding within ELF header tables

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

https://codereview.chromium.org/1233973002/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.cc:218: scoped_ptr<Elf32_Ehdr> header(new
Elf32_Ehdr);
On 2015/07/16 00:45:23, ivanpe wrote:
> Is this header needed only in the scope of this method?  If yes, then you can
> allocate it on the stack:
> 
>   Elf32_Ehdr header;

Done.

You're right; it is only needed within the method. Having the ELF header be a
pointer was a vestige from the design of the first patch set.

https://codereview.chromium.org/1233973002/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.cc:251: scoped_ptr<Elf64_Ehdr> header(new
Elf64_Ehdr);
On 2015/07/16 00:45:23, ivanpe wrote:
> Elf64_Ehdr header;

Done.

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

https://codereview.chromium.org/1233973002/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.h:85: void LoadElfHeader(scoped_ptr<T>&
header,
On 2015/07/16 00:45:23, ivanpe wrote:
> Please, move the out parameter to the end.  Also the type of the out parameter
> should be T*.  No need to pass a reference to a scoped_ptr in this case.

Done.

https://codereview.chromium.org/1233973002/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.h:104: uint16_t e_phnum) {
On 2015/07/16 00:45:23, ivanpe wrote:
> Please, update the function signature as follows:
> 
>    template<typename T>
>    void LoadElfHeaderTable(MinidumpMemoryRegion *memory,
>                            uint64_t base_address,
>                            uint16_t e_phnum,
>                            T* table)   // or   T table[]

Done.

https://codereview.chromium.org/1233973002/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.h:111: offset++;
On 2015/07/16 00:45:23, ivanpe wrote:
> This is a bit scary.  I'm concerned about padding at the end of the struct
(due
> to the alignment requirements of T). The next element in the table[] may not
> necessarily be adjacent in memory to the previous one.  I think that the n-th
> table entry must be written at address &table[n].  Or we should assert that
> &table[n] == table.get() + offset when starting a new table entry.

Done.

Good point. It never occurred to me since it was working for the tests. But it
makes sense especially since this is a method with generic types.

[ivanpe, 2015-07-16 17:20:57]

LGTM with some minor nits.

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

https://codereview.chromium.org/1233973002/diff/60001/src/processor/exploitab...
src/processor/exploitability_linux.h:91: memcpy(reinterpret_cast<char *>(header)
+ i,
You are copying a single byte here.  Why not just do the following:

*(reinterpret_cast<uint8_t *>(header) + i) = my_byte;

https://codereview.chromium.org/1233973002/diff/60001/src/processor/exploitab...
src/processor/exploitability_linux.h:113: memcpy(reinterpret_cast<char *>(entry)
+ j,
*(reinterpret_cast<uint8_t *>(entry) + j) = my_byte;

[liuandrew, 2015-07-16 17:35:08]

Uploaded patch set 5.

Changes include:
 - removing memcpy in favor of normal dereferencing
 - removing unnecessary #includes

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

https://codereview.chromium.org/1233973002/diff/60001/src/processor/exploitab...
src/processor/exploitability_linux.h:91: memcpy(reinterpret_cast<char *>(header)
+ i,
On 2015/07/16 17:20:57, ivanpe wrote:
> You are copying a single byte here.  Why not just do the following:
> 
> *(reinterpret_cast<uint8_t *>(header) + i) = my_byte;

Done.

https://codereview.chromium.org/1233973002/diff/60001/src/processor/exploitab...
src/processor/exploitability_linux.h:113: memcpy(reinterpret_cast<char *>(entry)
+ j,
On 2015/07/16 17:20:57, ivanpe wrote:
> *(reinterpret_cast<uint8_t *>(entry) + j) = my_byte;

Done.

[ivanpe, 2015-07-16 18:20:52]

lgtm

[liuandrew, 2015-07-16 19:31:32]

Uploaded patch set 6.

Changes include:
 - casting to mitigate risk of integer overflow (see lines 222 and 256)

[ivanpe, 2015-07-16 20:26:04]

lgtm

[liuandrew, 2015-07-16 20:42:42]

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