Close a window for a race with the system linker

Close a window for a race with the system linker

Lock globals at the head of Add/DelEntryImpl() callback handlers.

  The crazy linker exports updates to _r_debug pages to the UI thread
  if a callback poster function has been supplied.  At the time the
  callback is posted, the crazy linker holds the globals lock, used to
  prevent other threads from loading libraries (in particular through
  ::dlopen(), used for system libraries).  When the callback is
  received on a different thread the receiving code needs to reacquire
  the lock.

Make the globals lock reentrant.

  If there is no callback poster function then the updates to _r_debug
  pages happen immediately on the current thread.  Here the globals
  lock is already held.  Reentrancy ensures that we do not deadlock
  in that case.

Fix potential inaccuracy when looking for the page to mprotect.

  The addresses checked for writability are currently the start of the
  object being modified.  This is 12 or 16 bytes (on arm, 24 or 32
  bytes on arm64) below the address that will actually be written, and
  not guaranteed to be in the same memory page where the object
  crosses a page boundary.

Workround for races with the system linker over _r_debug protections.

  There is no way to safely and accurately sense whether _r_debug is
  in a readonly page -- we might read while the system linker is in
  the process of writing to it, and conclude that it is not readonly
  when it is.  Similarly, we may set it read/write, update pointers,
  and then set it readonly between the system linker setting it to
  read/write and updating pointers.  To work round this, we always map
  it to read/write, no matter its apparent initial state, and never
  map it back to readonly.  This may leave a small security hole, but
  it is better than the alternatives (typically, a crash).

BUG=450659, 458346
Committed: https://crrev.com/cd5c4353b052baa4d96925cb46f47e16a697f0e1
Cr-Commit-Position: refs/heads/master@{#317314}

[simonb (inactive), 2015-02-18 19:52:45]

simonb@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[no sievers, 2015-02-19 01:46:22]

sievers@chromium.org changed reviewers:
+ klobag@chromium.org

[no sievers, 2015-02-19 01:46:31]

sievers@chromium.org changed reviewers:
+ aelias@chromium.org

[rmcilroy, 2015-02-19 11:00:26]

Seems reasonable.  One suggestion but otherwise LGTM.

https://codereview.chromium.org/937813002/diff/1/third_party/android_crazy_li...
File third_party/android_crazy_linker/src/src/crazy_linker_rdebug.cpp (right):

https://codereview.chromium.org/937813002/diff/1/third_party/android_crazy_li...
third_party/android_crazy_linker/src/src/crazy_linker_rdebug.cpp:449:
after->l_prev = entry;
nit - could we just wrap this up in a helper function to ensure we always do it
right - i.e.:

void WriteLinkMapEntry(link_map_t** entry_pointer, link_map_t* entry) {
ScopedPageMapper mapper;
  if (readonly_entries_)
    mapper.MapReadWrite(entry_pointer);
  *entry_pointer = entry;
}

...
WriteLinkMapEntry(&after->l_prev, entry);

[rmcilroy, 2015-02-19 11:07:03]

Actually one final suggestion: Could you instead aquire the lock in
RDebugRunnable::Run and thereby avoid making the lock reenterent (and ensure
that any RDebugRunnable tasks acquire the lock before doing their work)?

[simonb (inactive), 2015-02-19 14:17:34]

New patchsets have been uploaded after l-g-t-m from rmcilroy@chromium.org

[simonb (inactive), 2015-02-19 14:27:53]

https://codereview.chromium.org/937813002/diff/1/third_party/android_crazy_li...
File third_party/android_crazy_linker/src/src/crazy_linker_rdebug.cpp (right):

https://codereview.chromium.org/937813002/diff/1/third_party/android_crazy_li...
third_party/android_crazy_linker/src/src/crazy_linker_rdebug.cpp:449:
after->l_prev = entry;
On 2015/02/19 11:00:26, rmcilroy wrote:
> nit - could we just wrap this up in a helper function to ensure we always do
it
> right - i.e.:
> 
> void WriteLinkMapEntry(link_map_t** entry_pointer, link_map_t* entry) {
> ScopedPageMapper mapper;
>   if (readonly_entries_)
>     mapper.MapReadWrite(entry_pointer);
>   *entry_pointer = entry;
> }
> 
> ...
> WriteLinkMapEntry(&after->l_prev, entry);

Done.

[simonb (inactive), 2015-02-19 14:29:25]

On 2015/02/19 11:07:03, rmcilroy wrote:
> Actually one final suggestion: Could you instead aquire the lock in
> RDebugRunnable::Run and thereby avoid making the lock reenterent (and ensure
> that any RDebugRunnable tasks acquire the lock before doing their work)?

Left as is.  There is another case: if we ask Java to run the callback on the UI
thread and we happen to already be on the UI thread then we get the callback
immediately.  That's the same as if we'd run it ourselves, but here execution
will come in through RDebugRunnable::Run yet we already hold the lock, so what
you suggest could lead to deadlock.

Just moving lock acquisition to RDebugRunnable::Run is an option, but to a
casual reader it's a lot less clear that the lock is held when we get into
Add/DelEntryImpl functions; the call by pointer obscures the control flow.

(In general yeah, I'm not a fan of reentrant locks.  But because this lock is
always properly scoped a reentrant variety should be okay.)

[rmcilroy, 2015-02-19 15:57:30]

On 2015/02/19 14:29:25, simonb wrote:
> On 2015/02/19 11:07:03, rmcilroy wrote:
> > Actually one final suggestion: Could you instead aquire the lock in
> > RDebugRunnable::Run and thereby avoid making the lock reenterent (and ensure
> > that any RDebugRunnable tasks acquire the lock before doing their work)?
> 
> Left as is.  There is another case: if we ask Java to run the callback on the
UI
> thread and we happen to already be on the UI thread then we get the callback
> immediately.  That's the same as if we'd run it ourselves, but here execution
> will come in through RDebugRunnable::Run yet we already hold the lock, so what
> you suggest could lead to deadlock.
> 
> Just moving lock acquisition to RDebugRunnable::Run is an option, but to a
> casual reader it's a lot less clear that the lock is held when we get into
> Add/DelEntryImpl functions; the call by pointer obscures the control flow.
> 
> (In general yeah, I'm not a fan of reentrant locks.  But because this lock is
> always properly scoped a reentrant variety should be okay.)

Sounds good, thanks for the explanation.

[simonb (inactive), 2015-02-19 19:35:07]

Updated to reflect latest thinking.  Currently testing (overnight) on a nexus4
which has shown the problem in the past.

rmcilroy@, please take another look when convenient, thanks.

[simonb (inactive), 2015-02-20 11:12:03]

> Currently testing (overnight) ...

More than 2k invocations overnight without problems.  Before patch set 3, crash
within around 200 invocations was normal.

[rmcilroy, 2015-02-20 11:17:34]

One suggestion and a couple of nits, but otherwise lgtm.

https://codereview.chromium.org/937813002/diff/40001/third_party/android_craz...
File third_party/android_crazy_linker/src/src/crazy_linker_rdebug.cpp (right):

https://codereview.chromium.org/937813002/diff/40001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_rdebug.cpp:159: class
ScopedPageMapper {
nit - could you call this something like ScopedPageReadWriteRemapper since this
is closer to what it does now.

https://codereview.chromium.org/937813002/diff/40001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_rdebug.cpp:163: void
Release();
nit - could you add a comment to say this releases the mapped page without
unmapping it.

https://codereview.chromium.org/937813002/diff/40001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_rdebug.cpp:176:
page_address_ = 0;
Should we return an error here to avoid doing the write if this operation fails
(either by making it a function again, or ensuring the write checks whether the
mapper object is valid before doing the write)?

https://codereview.chromium.org/937813002/diff/40001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_rdebug.cpp:412: //
TODO(simonb): Revisit this.
nit - link to the bug

[simonb (inactive), 2015-02-20 12:26:32]

New patchsets have been uploaded after l-g-t-m from rmcilroy@chromium.org

[simonb (inactive), 2015-02-20 12:52:57]

https://codereview.chromium.org/937813002/diff/40001/third_party/android_craz...
File third_party/android_crazy_linker/src/src/crazy_linker_rdebug.cpp (right):

https://codereview.chromium.org/937813002/diff/40001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_rdebug.cpp:159: class
ScopedPageMapper {
On 2015/02/20 11:17:33, rmcilroy wrote:
> nit - could you call this something like ScopedPageReadWriteRemapper since
this
> is closer to what it does now.

Done.

https://codereview.chromium.org/937813002/diff/40001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_rdebug.cpp:163: void
Release();
On 2015/02/20 11:17:33, rmcilroy wrote:
> nit - could you add a comment to say this releases the mapped page without
> unmapping it.

Done.

https://codereview.chromium.org/937813002/diff/40001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_rdebug.cpp:176:
page_address_ = 0;
On 2015/02/20 11:17:33, rmcilroy wrote:
> Should we return an error here to avoid doing the write if this operation
fails
> (either by making it a function again, or ensuring the write checks whether
the
> mapper object is valid before doing the write)?

Soft failure seems unappealing here.  For example we might succeed writing
l_prev but then fail with l_next and so leave a corrupted list.  In practice
FindProtectionFlagsForAddress should not fail unless there is a coding error
somewhere (for example we corrupt the value of _r_debug that we picked up from
DT_DEBUG so that we don't find the erroneous value in /proc/self/maps).  In such
cases a hard crash is probably preferable.

Also we can do little here to propagate the error status outwards to callers; it
is too late to fail the library load, because we already said it loaded okay.

I have added a log message, so we can at least see any failure in debug builds.

https://codereview.chromium.org/937813002/diff/40001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_rdebug.cpp:412: //
TODO(simonb): Revisit this.
On 2015/02/20 11:17:33, rmcilroy wrote:
> nit - link to the bug

Done.

[rmcilroy, 2015-02-20 13:25:52]

lgtm, thanks.

https://codereview.chromium.org/937813002/diff/40001/third_party/android_craz...
File third_party/android_crazy_linker/src/src/crazy_linker_rdebug.cpp (right):

https://codereview.chromium.org/937813002/diff/40001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_rdebug.cpp:176:
page_address_ = 0;
On 2015/02/20 12:52:56, simonb wrote:
> On 2015/02/20 11:17:33, rmcilroy wrote:
> > Should we return an error here to avoid doing the write if this operation
> fails
> > (either by making it a function again, or ensuring the write checks whether
> the
> > mapper object is valid before doing the write)?
> 
> Soft failure seems unappealing here.  For example we might succeed writing
> l_prev but then fail with l_next and so leave a corrupted list.  In practice
> FindProtectionFlagsForAddress should not fail unless there is a coding error
> somewhere (for example we corrupt the value of _r_debug that we picked up from
> DT_DEBUG so that we don't find the erroneous value in /proc/self/maps).  In
such
> cases a hard crash is probably preferable.
> 
> Also we can do little here to propagate the error status outwards to callers;
it
> is too late to fail the library load, because we already said it loaded okay.
> 
> I have added a log message, so we can at least see any failure in debug
builds.

Makes sense, thanks.

[simonb (inactive), 2015-02-20 13:47:48]

The CQ bit was checked by simonb@chromium.org

[commit-bot: I haz the power, 2015-02-20 13:48:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/937813002/60001

[commit-bot: I haz the power, 2015-02-20 14:15:47]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-20 14:16:44]

Patchset 4 (id:??) landed as
https://crrev.com/cd5c4353b052baa4d96925cb46f47e16a697f0e1
Cr-Commit-Position: refs/heads/master@{#317314}