Server-side workaround to handle overlapping modules.

Server-side workaround to handle overlapping modules.

This change is resolving an issue that was caused by the combination of:
 - Android system libraries being relro packed in N+.
 - Breakpad dealing with relro packed libraries in a hack way.

This is a fix for http://crbug/611824.

I also found an use-after-free issue (bug in Minidump::SeekToStreamType).  I disallowed the MinidumpStreamInfo copy and assign constructors and the compiler detected another similar issue in Minidump::Print.  Then I disabled the copy and assign constructors for most classes in minidump.h (just in case).  There are a couple of classes where I couldn't disallow them (since assign is used).  This will require a small refactor so I left it out of this CL.

R=mark@chromium.org

Committed: https://chromium.googlesource.com/breakpad/breakpad/+/24f5931c5e0120982c0cbf1896641e3ef2bdd52f

[ivanpe, 2016-06-11 01:48:33]

ivanpe@chromium.org changed reviewers:
+ mark@chromium.org

[Mark Mentovai, 2016-06-13 14:38:10]

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

https://codereview.chromium.org/2060663002/diff/20001/src/processor/minidump....
src/processor/minidump.cc:2510: range_map_->SetEnableShrinkDown(true);
I thought that we were only going to do this for the Android release that needs
it.

[Torne, 2016-06-13 16:07:07]

It seems like we should just call this load_bias, or similar, and not
"shrunk_down_delta".

This is intended, once we have actually got the load bias information, to be the
load bias. Using the amount by which we had to shrink the module for it to be
nonoverlapping as a guess at the load bias is the "short-term" way to deal with
it, but the general changes throughout the processor to handle a load bias are
expected to remain.

What do you think, Mark?

[Torne, 2016-06-13 16:07:28]

Description was changed from

==========
Server-side workaround to handle overlapping modules.

This change is resolving an issue that was caused by the combination of:
 - Android system libraries being relro packed in N+.
 - Breakpad dealing with relro packed libraries in a hack way.

This is a fix for http://crbug/611824.
==========

to

==========
Server-side workaround to handle overlapping modules.

This change is resolving an issue that was caused by the combination of:
 - Android system libraries being relro packed in N+.
 - Breakpad dealing with relro packed libraries in a hack way.

This is a fix for http://crbug/611824.
==========

[Mark Mentovai, 2016-06-13 16:23:26]

How forward-looking. That sounds right to me.

[ivanpe, 2016-06-13 20:26:33]

On 2016/06/13 16:07:07, Torne wrote:
> It seems like we should just call this load_bias, or similar, and not
> "shrunk_down_delta".
> 
> This is intended, once we have actually got the load bias information, to be
the
> load bias. Using the amount by which we had to shrink the module for it to be
> nonoverlapping as a guess at the load bias is the "short-term" way to deal
with
> it, but the general changes throughout the processor to handle a load bias are
> expected to remain.
> 
> What do you think, Mark?

I thought about this.  Unfortunately, shrink down delta (as implemented here) is
different from load bias and if I used load bias it will only cause confusion.

Load bias is when the reported base addresses are correct (correspond to the
actual values from the crashing process) and the bias indicates a delta to be
subtracted from the module offsets during symbolization.  Implementing support
for that would be much more invasive (and we can do it when needed).

Shrink down delta is a very simple method to deal with conflicting modules.  The
base address and size for all modules remain unchanged.  Symbolization remains
unchanged.  The only things that change are:
 - Conflicting modules can co-exist.
 - The way we decide which module contains a given instruction pointer.

When load bias is properly implemented (on both client and server), shrink down
delta should always be always 0.  This is basically a "temporary" resolution of
the problem that was caused by the Android hack.

[ivanpe, 2016-06-13 21:36:00]

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

https://codereview.chromium.org/2060663002/diff/20001/src/processor/minidump....
src/processor/minidump.cc:2510: range_map_->SetEnableShrinkDown(true);
On 2016/06/13 14:38:10, Mark Mentovai wrote:
> I thought that we were only going to do this for the Android release that
needs
> it.

I thought about this.  I'm sorry I didn't mentioned it in the description.  I
believe that it doesn't matter much whether this is enabled for all platforms or
just for Android.  And I actually see some benefits if this is enabled for all
platforms so I'd like to enable it for all platforms if permitted.

First of all, it is not expected to have overlapping modules on any platform, so
such issues indicate either some sort of a corruption or the presence of the
Android hack.

I decided to enable this for all platforms and examine the results.  I thought
I'll only see Android minidumps with this issue, but interestingly enough I also
saw 10s of Windows minidumps as well (out of 10s of millions - so it's a one in
a million kind of thing).

The good thing is that I found this very useful for non-Android platforms as
well.

Here are the benefits if this is enabled for all platforms:
 (1) Reports with a few corrupt module entries will get better stack traces.
 (2) Reports with a few corrupt module entries will have a list of modules.
 (3) It is much easier to spot and blame a corrupt minidump when there are
processing and/or symbolization issues due to conflicting modules.

Here is an example of a crash report: go/crash/7c7d497a00000000.  At the end of
the list of modules ('Modules' tab) you can see a modules with empty name, and
empty debug id that spans a large part of the virtual address space.  Due to
this single corruption, we cannot produce a stack trace.  However with the fix
we can produce a perfect stack trace and also we get an indication that there
were some module conflicts and how the processor dealt with them (shrunk the
address space of the offending module).

Here is what we get without the fix.  Unreadable stack trace and !!! NO list of
modules at all !!!

 0  0x6bc51595
    eip = 0x6bc51595   esp = 0x054df470   ebp = 0x054df484   ebx = 0x0fd0d2a8
    esi = 0x054df498   edi = 0x10037a0c   eax = 0x00000001   ecx = 0x054df498
    edx = 0xfffdf88d   efl = 0x00010286
    Found by: given as instruction pointer in context
 1  0x6caf80f8
    eip = 0x6caf80f8   esp = 0x054df48c   ebp = 0x054df4a0
    Found by: previous frame's frame pointer
 2  0x6bf7dfbc
    eip = 0x6bf7dfbc   esp = 0x054df4a8   ebp = 0x054df4ac
    Found by: previous frame's frame pointer
 3  0x6bff020f
    eip = 0x6bff020f   esp = 0x054df4b4   ebp = 0x054df4d0
    Found by: previous frame's frame pointer
 4  0x6bdbf737
    eip = 0x6bdbf737   esp = 0x054df4d8   ebp = 0x054df50c
    Found by: previous frame's frame pointer
 5  0x6bdbf53c
    eip = 0x6bdbf53c   esp = 0x054df514   ebp = 0x054df51c
    Found by: previous frame's frame pointer
 6  0x6bdbf421
    eip = 0x6bdbf421   esp = 0x054df524   ebp = 0x054df59c
    Found by: previous frame's frame pointer
 7  0x6bdb34ab
    eip = 0x6bdb34ab   esp = 0x054df5a4   ebp = 0x054df5b4
    Found by: previous frame's frame pointer
 8  0x6bdb344d
    eip = 0x6bdb344d   esp = 0x054df5bc   ebp = 0x054df5cc
    Found by: previous frame's frame pointer
 9  0x6bca064d
    eip = 0x6bca064d   esp = 0x054df5d4   ebp = 0x054df630
    Found by: previous frame's frame pointer
10  0x6bca0410
    eip = 0x6bca0410   esp = 0x054df638   ebp = 0x054df8ac
    Found by: previous frame's frame pointer
11  0x6bc9fd95
    eip = 0x6bc9fd95   esp = 0x054df8b4   ebp = 0x054df9b8
    Found by: previous frame's frame pointer
12  0x6bc9f961
    eip = 0x6bc9f961   esp = 0x054df9c0   ebp = 0x054df9d0
    Found by: previous frame's frame pointer
13  0x6bc9f8c8
    eip = 0x6bc9f8c8   esp = 0x054df9d8   ebp = 0x054df9ec
    Found by: previous frame's frame pointer
14  0x6bc9f70d
    eip = 0x6bc9f70d   esp = 0x054df9f4   ebp = 0x054dfa18
    Found by: previous frame's frame pointer
15  0x6bc9f601
    eip = 0x6bc9f601   esp = 0x054dfa20   ebp = 0x054dfa44
    Found by: previous frame's frame pointer
16  0x6bd7844c
    eip = 0x6bd7844c   esp = 0x054dfa4c   ebp = 0x054dfb14
    Found by: previous frame's frame pointer
17  0x6bd48906
    eip = 0x6bd48906   esp = 0x054dfb1c   ebp = 0x054dfb2c
    Found by: previous frame's frame pointer
18  0x6bc9e0a8
    eip = 0x6bc9e0a8   esp = 0x054dfb34   ebp = 0x054dfb68
    Found by: previous frame's frame pointer
19  0x6bc9ddf3
    eip = 0x6bc9ddf3   esp = 0x054dfb70   ebp = 0x054dfb8c
    Found by: previous frame's frame pointer
20  0x75727c04
    eip = 0x75727c04   esp = 0x054dfb94   ebp = 0x054dfba0
    Found by: previous frame's frame pointer
21  0x7762ab8f
    eip = 0x7762ab8f   esp = 0x054dfba8   ebp = 0x054dfbe8
    Found by: previous frame's frame pointer
22  0x7762ab5a
    eip = 0x7762ab5a   esp = 0x054dfbf0   ebp = 0x054dfbf8
    Found by: previous frame's frame pointer


Here is what we get with the fix.  Perfect stack trace and a complete list of
modules (minidump_stackwalk hasn't been updated to show the module conflicts
yet, but the data is readily available in the ProcessState):

Stack trace:
--------------
(chrome.dll -http_stream_parser.cc:392 )
base::internal::InvokeHelper<1,void,base::internal::RunnableAdapter<void (
net::MDnsConnection::*)(int)> >::MakeItSo<base::WeakPtr<net::MDnsConnection>,int
const &>(base::internal::RunnableAdapter<void (
net::MDnsConnection::*)(int)>,base::WeakPtr<net::MDnsConnection>,int const &)
base::internal::Invoker<base::IndexSequence<0>,base::internal::BindState<base::internal::RunnableAdapter<void
(
content::ServiceWorkerFetchDispatcher::*)(content::ServiceWorkerStatusCode)>,void
,base::WeakPtr<content::ServiceWorkerFetchDispatcher>
>,base::internal::InvokeHelper<1,void,base::internal::RunnableAdapter<void (
content::ServiceWorkerFetchDispatcher::*)(content::ServiceWorkerStatusCode)>
>,void >::Run(base::internal::BindStateBase *,content::ServiceWorkerStatusCode
&&)
net::TCPClientSocket::DidCompleteReadWrite(base::Callback<void ,1> const &,int)
base::internal::Invoker<base::IndexSequence<0,1>,base::internal::BindState<base::internal::RunnableAdapter<void
( net::SOCKSClientSocket::*)(base::Callback<void ,1> const &,int)>,void
,base::internal::UnretainedWrapper<net::SOCKSClientSocket>,base::Callback<void
,1> const
&>,base::internal::InvokeHelper<0,void,base::internal::RunnableAdapter<void (
net::SOCKSClientSocket::*)(base::Callback<void ,1> const &,int)> >,void
>::Run(base::internal::BindStateBase *,int &&)
net::TCPSocketWin::DidSignalRead()
net::TCPSocketWin::Core::ReadDelegate::OnObjectSignaled(void *)
base::win::ObjectWatcher::Signal(base::win::ObjectWatcher::Delegate *)
base::internal::Invoker<base::IndexSequence<0,1>,base::internal::BindState<base::internal::RunnableAdapter<void
( base::win::ObjectWatcher::*)(base::win::ObjectWatcher::Delegate *)>,void
,base::WeakPtr<base::win::ObjectWatcher>,base::win::ObjectWatcher::Delegate *
&>,base::internal::InvokeHelper<1,void,base::internal::RunnableAdapter<void (
base::win::ObjectWatcher::*)(base::win::ObjectWatcher::Delegate *)> >,void
>::Run(base::internal::BindStateBase *)
base::debug::TaskAnnotator::RunTask(char const *,base::PendingTask const &)
base::MessageLoop::RunTask(base::PendingTask const &)
base::MessageLoop::DoWork()
base::MessagePumpForIO::DoRunLoop()
base::MessagePumpWin::Run(base::MessagePump::Delegate *)
base::RunLoop::Run()
base::Thread::Run(base::MessageLoop *)
content::BrowserThreadImpl::IOThreadRun(base::MessageLoop *)
content::BrowserThreadImpl::Run(base::MessageLoop *)
base::Thread::ThreadMain()
base::`anonymous namespace'::ThreadFunc
BaseThreadInitThunk
__RtlUserThreadStart
_RtlUserThreadStart


Loaded modules:
--------------------
0x011d0000 - 0x012bbfff  chrome.exe  51.0.2704.84  (main)
0x667e0000 - 0x66948fff  DUI70.dll  6.3.9600.17415
0x6a2f0000 - 0x6a2fafff  msdmo.dll  6.6.9600.17415
...
...
0x775d0000 - 0x7773efff  ntdll.dll  6.3.9600.18233
0x4d8b52a8 - 0xec7552a5    0.0.0.0

[Mark Mentovai, 2016-06-14 16:57:46]

I’d still prefer to only enable this for Android (and Android N, at that). We
know what the module overlap means on Android N. We can’t be so sure what it
would mean on other platforms. (Should the “bigger” module win? The smaller one?
Something else?)

When the stackwalk fails for this reason, it alerts us to the fact that
something’s wrong with the module list. In this case
(go/crash/7c7d497a00000000), I’m not sure what the problem might be, but I do
see that strange module in there, and I think that we should investigate it. If
we just papered over these kinds of problems by squashing modules, we’d never be
alerted to the true underlying issue as we are being now, and we’d never have a
chance to investigate and fix correctly. +scottmg, since this is a
Crashpad-generated dump.

[Mark Mentovai, 2016-06-14 17:01:45]

https://crashpad.chromium.org/bug/117 for the weird module on Windows.

[Torne, 2016-06-15 16:51:52]

On 2016/06/13 20:26:33, ivanpe wrote:
> On 2016/06/13 16:07:07, Torne wrote:
> > It seems like we should just call this load_bias, or similar, and not
> > "shrunk_down_delta".
> > 
> > This is intended, once we have actually got the load bias information, to be
> the
> > load bias. Using the amount by which we had to shrink the module for it to
be
> > nonoverlapping as a guess at the load bias is the "short-term" way to deal
> with
> > it, but the general changes throughout the processor to handle a load bias
are
> > expected to remain.
> > 
> > What do you think, Mark?
> 
> I thought about this.  Unfortunately, shrink down delta (as implemented here)
is
> different from load bias and if I used load bias it will only cause confusion.
> 
> Load bias is when the reported base addresses are correct (correspond to the
> actual values from the crashing process) and the bias indicates a delta to be
> subtracted from the module offsets during symbolization.  Implementing support
> for that would be much more invasive (and we can do it when needed).

My point is that this *is* what we need. The goal here is to have the client
report the load bias, and the processor use the load bias. What I was expecting
we would do is that for the short term (to avoid rushing clientside breakpad
changes into M51) we would just shrink down the overlap to generate a "fake"
load bias to populate this field, and then when we'd later updated the client to
send the actual load bias and actual base address instead of "adjusting" it in
the hacky way, the processor would only need very minor changes to consume the
real load bias instead of guessing it based on overlaps.

> Shrink down delta is a very simple method to deal with conflicting modules. 
The
> base address and size for all modules remain unchanged.  Symbolization remains
> unchanged.  The only things that change are:
>  - Conflicting modules can co-exist.
>  - The way we decide which module contains a given instruction pointer.
> 
> When load bias is properly implemented (on both client and server), shrink
down
> delta should always be always 0.  This is basically a "temporary" resolution
of
> the problem that was caused by the Android hack.

Yes, that's what I expect, but my point is that it seems like we could, instead
of implementing *two* mechanisms on the server (shrink down delta and load
bias), just implement load bias now and avoid needing to make larger changes
later. I'm not that familiar with the breakpad code so maybe I'm wrong, but it
doesn't seem like doing it the way you've done it here is really any easier than
actually supporting load bias.

[ivanpe, 2016-06-15 17:41:14]

On 2016/06/15 16:51:52, Torne wrote:
> On 2016/06/13 20:26:33, ivanpe wrote:
> > On 2016/06/13 16:07:07, Torne wrote:
> > > It seems like we should just call this load_bias, or similar, and not
> > > "shrunk_down_delta".
> > > 
> > > This is intended, once we have actually got the load bias information, to
be
> > the
> > > load bias. Using the amount by which we had to shrink the module for it to
> be
> > > nonoverlapping as a guess at the load bias is the "short-term" way to deal
> > with
> > > it, but the general changes throughout the processor to handle a load bias
> are
> > > expected to remain.
> > > 
> > > What do you think, Mark?
> > 
> > I thought about this.  Unfortunately, shrink down delta (as implemented
here)
> is
> > different from load bias and if I used load bias it will only cause
confusion.
> > 
> > Load bias is when the reported base addresses are correct (correspond to the
> > actual values from the crashing process) and the bias indicates a delta to
be
> > subtracted from the module offsets during symbolization.  Implementing
support
> > for that would be much more invasive (and we can do it when needed).
> 
> My point is that this *is* what we need. The goal here is to have the client
> report the load bias, and the processor use the load bias. What I was
expecting
> we would do is that for the short term (to avoid rushing clientside breakpad
> changes into M51) we would just shrink down the overlap to generate a "fake"
> load bias to populate this field, and then when we'd later updated the client
to
> send the actual load bias and actual base address instead of "adjusting" it in
> the hacky way, the processor would only need very minor changes to consume the
> real load bias instead of guessing it based on overlaps.
> 
> > Shrink down delta is a very simple method to deal with conflicting modules. 
> The
> > base address and size for all modules remain unchanged.  Symbolization
remains
> > unchanged.  The only things that change are:
> >  - Conflicting modules can co-exist.
> >  - The way we decide which module contains a given instruction pointer.
> > 
> > When load bias is properly implemented (on both client and server), shrink
> down
> > delta should always be always 0.  This is basically a "temporary" resolution
> of
> > the problem that was caused by the Android hack.
> 
> Yes, that's what I expect, but my point is that it seems like we could,
instead
> of implementing *two* mechanisms on the server (shrink down delta and load
> bias), just implement load bias now and avoid needing to make larger changes
> later. I'm not that familiar with the breakpad code so maybe I'm wrong, but it
> doesn't seem like doing it the way you've done it here is really any easier
than
> actually supporting load bias.

Hi Torne, my takeaway from the last meeting (with everyone) was that further
client work on this is NOT needed and basically unlikely to happen so it looks
like we only need a *single* solution.  Also, if I recall correctly, this server
fix was needed urgently.  The current solution is much less invasive than the
alternative.

[ivanpe, 2016-06-20 06:16:15]

On 2016/06/13 14:38:10, Mark Mentovai wrote:
>
https://codereview.chromium.org/2060663002/diff/20001/src/processor/minidump.cc
> File src/processor/minidump.cc (right):
> 
>
https://codereview.chromium.org/2060663002/diff/20001/src/processor/minidump....
> src/processor/minidump.cc:2510: range_map_->SetEnableShrinkDown(true);
> I thought that we were only going to do this for the Android release that
needs
> it.

Agreed.  Done.

[ivanpe, 2016-06-20 06:18:21]

On 2016/06/14 16:57:46, Mark Mentovai wrote:
> I’d still prefer to only enable this for Android (and Android N, at that). We
> know what the module overlap means on Android N. We can’t be so sure what it
> would mean on other platforms. (Should the “bigger” module win? The smaller
one?
> Something else?)
> 
> When the stackwalk fails for this reason, it alerts us to the fact that
> something’s wrong with the module list. In this case
> (go/crash/7c7d497a00000000), I’m not sure what the problem might be, but I do
> see that strange module in there, and I think that we should investigate it.
If
> we just papered over these kinds of problems by squashing modules, we’d never
be
> alerted to the true underlying issue as we are being now, and we’d never have
a
> chance to investigate and fix correctly. +scottmg, since this is a
> Crashpad-generated dump.

As discussed offline, there is not enough information in the minidump to tell
what the Android family is.  Enabled for all Android.

[ivanpe, 2016-06-20 06:18:47]

Mark, PTAL.

[ivanpe, 2016-06-20 06:33:14]

Description was changed from

==========
Server-side workaround to handle overlapping modules.

This change is resolving an issue that was caused by the combination of:
 - Android system libraries being relro packed in N+.
 - Breakpad dealing with relro packed libraries in a hack way.

This is a fix for http://crbug/611824.
==========

to

==========
Server-side workaround to handle overlapping modules.

This change is resolving an issue that was caused by the combination of:
 - Android system libraries being relro packed in N+.
 - Breakpad dealing with relro packed libraries in a hack way.

This is a fix for http://crbug/611824.

I also found an use-after-free issue (bug in Minidump::SeekToStreamType).  I
disallowed the MinidumpStreamInfo copy and assign constructors and the compiler
detected another similar issue in Minidump::Print.  Then I disabled the copy and
assign constructors for most classes in minidump.h (just in case).  There are a
couple of classes where I couldn't disallow them (since assign is used).  This
will require a small refactor so I left it out of this CL.
==========

[Mark Mentovai, 2016-06-20 13:45:24]

LGTM

[ivanpe, 2016-06-20 18:14:51]

Description was changed from

==========
Server-side workaround to handle overlapping modules.

This change is resolving an issue that was caused by the combination of:
 - Android system libraries being relro packed in N+.
 - Breakpad dealing with relro packed libraries in a hack way.

This is a fix for http://crbug/611824.

I also found an use-after-free issue (bug in Minidump::SeekToStreamType).  I
disallowed the MinidumpStreamInfo copy and assign constructors and the compiler
detected another similar issue in Minidump::Print.  Then I disabled the copy and
assign constructors for most classes in minidump.h (just in case).  There are a
couple of classes where I couldn't disallow them (since assign is used).  This
will require a small refactor so I left it out of this CL.
==========

to

==========
Server-side workaround to handle overlapping modules.

This change is resolving an issue that was caused by the combination of:
 - Android system libraries being relro packed in N+.
 - Breakpad dealing with relro packed libraries in a hack way.

This is a fix for http://crbug/611824.

I also found an use-after-free issue (bug in Minidump::SeekToStreamType).  I
disallowed the MinidumpStreamInfo copy and assign constructors and the compiler
detected another similar issue in Minidump::Print.  Then I disabled the copy and
assign constructors for most classes in minidump.h (just in case).  There are a
couple of classes where I couldn't disallow them (since assign is used).  This
will require a small refactor so I left it out of this CL.

R=mark@chromium.org

Committed:
https://chromium.googlesource.com/breakpad/breakpad/+/24f5931c5e0120982c0cbf1...
==========

[ivanpe, 2016-06-20 18:14:52]

Committed patchset #4 (id:60001) manually as
24f5931c5e0120982c0cbf1896641e3ef2bdd52f (presubmit successful).